Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference Target: what type should the referenceTarget attribute be? #1093

Open
alice opened this issue Jan 29, 2025 · 11 comments
Open

Reference Target: what type should the referenceTarget attribute be? #1093

alice opened this issue Jan 29, 2025 · 11 comments

Comments

@alice
Copy link
Member

alice commented Jan 29, 2025

@alice To make sure I understand you, are you thinking that the referenceTarget attribute's type should be a nullable DOMString?

partial interface ShadowRoot {
  attribute DOMString? referenceTarget;
}

I had been assuming the attribute's type would just be DOMString, and that's how it's prototyped today. But looking at the explainer again I guess it's not specific on this point.

Assuming I'm reading the specs correctly, keeping it as a non-nullable DOMString triggers this string conversion when trying to set the attribute to null, and this conversion happens before the implementation of the property "sees" the value:

document.querySelector("#fancy-listbox").shadowRoot.referenceTarget = null;
document.querySelector("#fancy-listbox").shadowRoot.referenceTarget; // logs 'null'

But if we make it DOMString? then we'd get the behavior you suggest. That's definitely seems like it'd avoid a pitfall when someone wants to clear the referenceTarget value. An inconsistency it introduces though is that the Element.id property is itself a non-nullable DOMString. So you get this today:

const div = document.createElement("div");
div.id = null;
div.id; // logs 'null'

It's tempting to treat the referenceTarget property as kind of equivalent to setting an ID elsewhere, so keeping the behavior consistent could be nice. What do you think? (Maybe this question should be its own thread.)

Originally posted by @dandclark in #1089

@alice
Copy link
Member Author

alice commented Jan 30, 2025

It's tempting to treat the referenceTarget property as kind of equivalent to setting an ID elsewhere

I think of referenceTarget less like an ID, and more like an ID reference - it's not identifying the shadow root, but referring to another element's ID.

Most attributes on Elements which use ID references (and don't use Element reflection) are non-nullable DOMStrings, I think for the reason that nullable DOMStrings are supposed to be limited to enumerated values (which we messed up with ARIAMixin, unfortunately). So, I think you could still make a similar analogy.

However, I think it depends on how we want to think about un-setting a referenceTarget property, which is what the original comment was in reference to. I was surprised by this example code:

document.querySelector("#fancy-listbox").shadowRoot.referenceTarget = "";
console.log(input.ariaControlsElements);  // logs `listbox` since there is no reference target; aria-controls now applies to the shadow host

The original issue was concerning invalid values for referenceTarget; we determined that an invalid value should work as though any reference to the host was an invalid ID reference. Meanwhile, a lack of a referenceTarget property means that any reference to the host is a valid reference to the host. My surprise resulted from referenceTarget being set to an empty string being considered synonymous with the referenceTarget property not being present at all; I had expected that it would be similar to setting any ID reference property to an empty string (i.e., straightforwardly an invalid reference, since no element can have an empty string as an ID).

I can see three options here:

  1. referenceTarget is a nullable DOMString, and setting it to null is equivalent to not having a referenceTarget property
  2. referenceTarget is a non-nullable DOMString, and setting it to empty string is equivalent to not having a referenceTarget property
  3. referenceTarget is a non-nullable DOMString, setting it to empty string is equivalent to setting it to any other invalid ID value, and it can be un-set using delete.

My preference would be (1), but I'd honestly be ok with any of these.

@dandclark
Copy link
Contributor

I definitely see your point here. I'd be fine with either option (1) or (2); I could live with (3) but prefer it less than either of the others.

@Westbrook
Copy link
Collaborator

I'd vote against no. 3, because delete 🤮

I have a hard time really understanding the difference between nos. 1 & 2.

  • In no. 1, how would setting to "" be different than setting to null?
  • In no. 2, what would setting to null do?

@alice
Copy link
Member Author

alice commented Jan 30, 2025

I left out an important part of (1); I should have made a table:

Option Type "" is null is
1 DOMString? equivalent to invalid ID equivalent to un-set
2 DOMString equivalent to un-set converted to "null", likely invalid
3 DOMString equivalent to invalid ID converted to "null", likely invalid

So, to answer @Westbrook's questions:

  • In no. 1, how would setting to "" be different than setting to null?

"" is not a valid ID, so it's treated the same as any other invalid ID value.

  • In no. 2, what would setting to null do?

It's converted to a string, "null".

@Westbrook
Copy link
Collaborator

Thanks @alice!

All this points me towards no. 1 as well. I’ll make sure the WCCG has a think on this when we meet Friday to see if there’s any other feedback in the options.

@smaug----
Copy link

This depends on whether we want to have two attributes, or just one which deals both referencetarget and referencetargetmap (I'd still prefer to merge those two).
And even if there are two attributes, perhaps for consistency they should be reflected somehow similarly to webidl.

@annevk
Copy link
Collaborator

annevk commented Feb 14, 2025

If this actually points to an element through an ID, wouldn't it be referenceTargetElement in IDL? We went through this with popovertarget and ARIA. Not sure why this would get to decide a new shape.

@alice
Copy link
Member Author

alice commented Feb 17, 2025

@annevk The explainer did include some comments on this, e.g.:

  • [A referenceTargetElement attribute] does not unlock any net-new functionality. Since referenceTarget only works with elements inside the shadow root, every element that could be a target is accessible by a string ID reference.

Note: This is in contrast to the ARIAMixin attributes like ariaLabelledByElements, which do unlock the new functionality of referring out of the shadow DOM. In that case, the complexity is necessary to include in the ARIAMixin design.

  • At a basic level, Reference Target is augmenting the existing functionality of referring to elements by ID string. It seems in line with the design to require using ID strings.

I'm not sure I agree with all of the arguments listed in the explainer, but I do think the design goals and constraints are slightly different here than for something like popovertarget or aria-labelledby:

  • Since a shadow root is not an Element, there is no option to use setAttribute() to use an ID string if that's what the author prefers (for example, if the intent is for the shadow root to be serializable)
  • There is some extra complexity compared with a regular Element, because there are three interacting interfaces:
    • the template element
    • the ShadowRootInit dictionary
    • the ShadowRoot object.

I think it makes sense to at least offer the option of having a referenceTarget attribute which takes a DOMString, and matches the referenceTarget property in ShadowRootInit. As the explainer points out, offering a referenceTargetElement attribute as well offers no additional functionality in this context other than convenience, so I think it can at least be deferred.

@annevk
Copy link
Collaborator

annevk commented Feb 18, 2025

That's a good point. From the earlier options above, I don't see how 3 would work with delete. That doesn't end up invoking the getter or setter, iirc. Both 1 and 2 seem workable. I have a preference for 2 as that more closely matches the semantics of the id getter/setter and getElementById().

@alice
Copy link
Member Author

alice commented Feb 18, 2025

I don't see how 3 would work with delete. That doesn't end up invoking the getter or setter, iirc.

Hm, yeah, I didn't really think about that. Definitely a non-starter!

I have a preference for 2 as that more closely matches the semantics of the id getter/setter and getElementById().

It's slightly different from the id setter in that if you do el.id = '', el will sprout an empty id attribute, which will result in a different result when using getAttribute() or hasAttribute() or attribute selector matching. Obviously it won't do you much good for any ID-based APIs, since it's an invalid ID, but it does change the semantics of the element.

Implementation-wise, it looks like both existing implementations currently assume a non-null value for referenceTarget is meaningful, even if it's an empty string: i.e. any non-null value (including empty string) for referenceTarget means that the host will not be the result of resolving the reference target.

If we went with option 2, I think we'd have to update the implementations and write the spec such that the value defaults to empty string, and we consider empty string to be a "missing value" meaning that the host should be the result of resolving the reference target in that case.

@annevk
Copy link
Collaborator

annevk commented Feb 19, 2025

I think it's okay that it's different from id in that regard. It's also not on an element after all. Handling null in the same way seems preferable though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants