-
Notifications
You must be signed in to change notification settings - Fork 3k
Add reference target #10995
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
base: main
Are you sure you want to change the base?
Add reference target #10995
Conversation
… to shadow creation
…eference-target-attr-associated-element
Refactoring, and adding output-for and popover
…d make some tweaks in the Reflection section
Collapse most of the 'reset the form owner' checks into one that just watches for changes to the form(attr)-associated-element.
@annevk, @domenic, this isn't quite ready to land yet as there are still a few open questions to sort out (at least WICG/webcomponents#1087 and WICG/webcomponents#1093), plus missing implementor positions. But I don't expect most of the mechanics in this PR to change, so I think it would be great to get feedback on whether the approach we've used here is agreeable. See also whatwg/dom#1353 Thanks! |
There's also a WIP PR against ARIA to account for these changes: w3c/aria#2474 There are some open questions being discussed there on exactly how to refer to the HTML concepts, since ARIA needs to be language-independent, but it will certainly refer to these spec concepts in some form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be updated to make use of the new [Reflect] syntax.
Thanks @lukewarlow! Fixed now (it wouldn't let me merge the suggestions directly due to merge conflicts). |
I'm not sure how GitHub actually lets you dismiss change suggestions but consider this me dropping my change suggestions as they've been resolved :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done an initial review pass, got about half way, but I thought I'd provide my review commentary so far because I think if I continue I may end up repeating themes.
Some meta commentary:
- I think another formatting pass needs to be done.
- I am a little worried about the retargeting steps, I wonder if this can be simplified to avoid retargeting and to resolve a reference target where necessary.
|
||
<li><p>If no such element exists, return null.</p></li> | ||
|
||
<li><p>Return the result of <span data-x="resolve-the-reference-target">resolving the reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand how this set of steps avoids exposing elements in the shadow root to code calling the IDL bindings?
It seems as though there's nothing in the spec that says "when script calls this, it should not resolve the reference target".
The way I would interpret this is that IDL should return the resolved reference target, which is obviously wrong as it would expose ShadowDOM elements to script.
I'm surprised this has is written this way, I would have imagined we would not change reflection rules, and instead wherever we look at an element reflecting attribute, we would have an additional step to resolve the reference target there.
<span>shadow root</span>'s <span>reference target</span> is null, return <var>element</var>.</p> | ||
</li> | ||
|
||
<li><p>Let <var>referenceTarget</var> be the value of <var>element</var>'s <span>shadow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a little confused here about what referenceTarget
represents. A non-binding suggestion: rename this to perhaps referenceTargetId
or just targetId
or similar?
<li><p>Let <var>candidate</var> be the first element in <var>element</var>'s <span>shadow | ||
root</span> whose <span data-x="concept-id">ID</span> matches <var>referenceTarget</var>.</p> | ||
</li> | ||
|
||
<li><p>If no such element exists, return null.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These steps being disjoint were confusing to me, as step 4 read in isolation makes no reference to variables or some such. I think they'd be better joined:
<li><p>Let <var>candidate</var> be the first element in <var>element</var>'s <span>shadow | |
root</span> whose <span data-x="concept-id">ID</span> matches <var>referenceTarget</var>.</p> | |
</li> | |
<li><p>If no such element exists, return null.</p></li> | |
<li><p>Let <var>candidate</var> be the first element in <var>element</var>'s <span>shadow | |
root</span> whose <span data-x="concept-id">ID</span> matches <var>referenceTarget</var>. If no | |
such element exists, then return null.</p></li> |
Or alternatively:
<li><p>Let <var>candidate</var> be the first element in <var>element</var>'s <span>shadow | |
root</span> whose <span data-x="concept-id">ID</span> matches <var>referenceTarget</var>.</p> | |
</li> | |
<li><p>If no such element exists, return null.</p></li> | |
<li><p>Let <var>candidate</var> be the first element in <var>element</var>'s <span>shadow | |
root</span> whose <span data-x="concept-id">ID</span> matches <var>referenceTarget</var>. If no | |
such element exists, then instead let <var>element</var> be null.</p></li> | |
<li><p>If <var>element</var> is null, then return null</p></li> | |
<li><p>Return the result of <span data-x="resolve-the-reference-target">resolving the reference | ||
target</span> on <var>candidate</var>. | ||
</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting:
<li><p>Return the result of <span data-x="resolve-the-reference-target">resolving the reference | |
target</span> on <var>candidate</var>. | |
</p></li> | |
<li><p>Return the result of <span data-x="resolve-the-reference-target">resolving the reference | |
target</span> on <var>candidate</var>.</p></li> |
<p><dfn data-x="element reference attribute" data-lt="element reference" export> | ||
Element reference attributes</dfn> refer to one or more Elements in the document. When specified | ||
as content attributes, <span | ||
data-x="element reference attribute">element reference attributes</span> refer to other Elements | ||
via their <span data-x="concept-ID">ID</span>s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting is not right here, I think, but also it's missing a </p>
:
<p><dfn data-x="element reference attribute" data-lt="element reference" export> | |
Element reference attributes</dfn> refer to one or more Elements in the document. When specified | |
as content attributes, <span | |
data-x="element reference attribute">element reference attributes</span> refer to other Elements | |
via their <span data-x="concept-ID">ID</span>s. | |
<p><dfn data-x="element reference attribute" data-lt="element reference" export>Element reference | |
attributes</dfn> refer to one or more Elements in the document. When specified as content | |
attributes, <span data-x="element reference attribute">element reference attributes</span> refer | |
to other Elements via their <span data-x="concept-ID">ID</span>s.</p> |
</ol> | ||
</li> | ||
</ol> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double line break:
<p class="note">An element will only have | ||
<span data-x="explicitly set attr-elements">explicitly-set <var>attr</var>-elements</span> if it | ||
has an IDL attribute with type <code | ||
data-x="">FrozenArray<<var>T</var>>?</code>, where <var>T</var> is either | ||
<code>Element</code> or an interface that inherits from <code>Element</code>, which | ||
<span data-x="reflect">reflects</span> <var>attr</var>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting seems wonky here, maybe this is better:
<p class="note">An element will only have | |
<span data-x="explicitly set attr-elements">explicitly-set <var>attr</var>-elements</span> if it | |
has an IDL attribute with type <code | |
data-x="">FrozenArray<<var>T</var>>?</code>, where <var>T</var> is either | |
<code>Element</code> or an interface that inherits from <code>Element</code>, which | |
<span data-x="reflect">reflects</span> <var>attr</var>.</p> | |
<p class="note">An element will only have | |
<span data-x="explicitly set attr-elements">explicitly-set <var>attr</var>-elements</span> if it | |
has an IDL attribute with type <code data-x="">FrozenArray<<var>T</var>>?</code>, where | |
<var>T</var> is either <code>Element</code> or an interface that inherits from | |
<code>Element</code>, which <span data-x="reflect">reflects</span> <var>attr</var>.</p> |
<li> | ||
<p><span data-x="list iterate">For each</span> <var>candidate</var> in <var>candidates</var>: | ||
</p> | ||
|
||
<ol> | ||
<li><p>Let <var>resolvedCandidate</var> be the result of | ||
<span data-x="resolve-the-reference-target">resolving the reference target</span> on | ||
<var>candidate</var>.</p></li> | ||
|
||
<li><p>If <var>resolvedCandidate</var> is not null, append <var>resolvedCandidate</var> to | ||
<var>resolvedCandidates</var>.</p></li> | ||
</ol> | ||
</li> | ||
|
||
<li><p>Return <var>resolvedCandidates</var>.</p></li> | ||
</ol> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we make a list of candidates to then re-iterate the list and resolve them? Would it not be easier to resolve each candidate during the for each steps so that candidates
is a list of resolvedCandidate
s?
<li><p>Let <var>candidate</var> be the result of running <span>this</span>'s | ||
<span data-x="get the attr-associated element">get the <var>attr</var>-associated | ||
element</span>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. IDL reflections take the attr-resolved reference target and then retarget against element. This seems a little like extra work for no payoff. Are there concrete reasons to do this, rather than having attr-resolved elements return the element, and then in each spec resolve the reference target?
I'm also not sure retargeting against element is always going to consistently give the same result as the old way of handling things. This feels awkward to me.
readonly attribute <span>HTMLFormElement</span>? <span data-x="dom-label-form">form</span>; | ||
readonly attribute <span>HTMLElement</span>? <span data-x="dom-label-form">form</span>; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be observable? You'd still end up with a form element returned in all existing code right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current code expects a form element or null. This change would mean it could get some other thing, which may be surprising.
An example of a potential surprise: if (foo.form) new FormData(foo.form)
would now throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but you'd only get that if you start using this new referencetarget? So it's not really a compat issue as I understand it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess "web compatible" is the wrong term here. Old sites wouldn't break, but the change here might cause a surprise for actively maintained websites.
I guess I'll mark as resolved.
<ol> | ||
<li> | ||
<p><span data-x="list iterate">For each</span> <var>attrElement</var> in | ||
<var>reflectedTarget</var>'s <span>explicitly set <var>attr</var>-elements</span>:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is reflectedTarget here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<var>reflectedTarget</var>'s <span>explicitly set <var>attr</var>-elements</span>:</p> | |
<var>element</var>'s <span>explicitly set <var>attr</var>-elements</span>:</p> |
<span>tree</span>.</p> | ||
a valid <span>single element reference</span> attribute | ||
<span data-x="single-element-reference-refers-to">referring to</span> a <code>form</code> | ||
element.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't perhaps super clear. I think this means that the final element from single element reference
should be a form element
<li><p>Let <var>candidates</var> be an empty <span>list</span>.</p></li> | ||
|
||
<li> | ||
<p>If <var>element</var>'s has an <span>explicitly set <var>attr</var>-elements</span> which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't grammatically correct. Based on the which I'm guessing the has is correct so drop the 's
<p>If <var>element</var>'s has an <span>explicitly set <var>attr</var>-elements</span> which | |
<p>If <var>element</var> has an <span>explicitly set <var>attr</var>-elements</span> which |
order</span> is a <span data-x="category-label">labelable element</span>, then that element is the | ||
<code>label</code> element's <span>labeled control</span>.</span></p> | ||
attribute is specified, the attribute's value must be the a valid <span>single element | ||
reference</span> attribute.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the final result of single element reference
doesn't need to be labelable element?
<var>descendant</var>.</p></li> | ||
|
||
<li><p>If <var>candidate</var> is a <span data-x="category-label">labelable element</span>, | ||
return <var>candidate</var>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handling of descendants feels a bit surprising, but I don't have strong opinions, and the behavior is probably fine.
data-x="attr-popover">popover</code> attribute in the same <span>tree</span> as the <span | ||
data-x="concept-button">button</span> with the <code | ||
be a valid <span>single element reference</span> attribute | ||
<span data-x="single-element-reference-refers-to">referring to</span> an element with the <code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? The old text requires an element with popover attribute.
@@ -65213,6 +65362,7 @@ interface <dfn interface>HTMLTemplateElement</dfn> : <span>HTMLElement</span> { | |||
[<span>CEReactions</span>, <span data-x="xattr-Reflect">Reflect</span>] attribute boolean <dfn attribute for="HTMLTemplateElement" data-x="dom-template-shadowrootclonable">shadowRootClonable</dfn>; | |||
[<span>CEReactions</span>, <span data-x="xattr-Reflect">Reflect</span>] attribute boolean <dfn attribute for="HTMLTemplateElement" data-x="dom-template-shadowRootSerializable">shadowRootSerializable</dfn>; | |||
[<span>CEReactions</span>, <span data-x="xattr-Reflect">Reflect</span>] attribute DOMString <dfn attribute for="HTMLTemplateElement" data-x="dom-template-shadowRootCustomElementRegistry">shadowRootCustomElementRegistry</dfn>; | |||
[<span>CEReactions</span>, <span data-x="xattr-Reflect">Reflect</span>] attribute DOMString? <dfn attribute for="HTMLTemplateElement" data-x="dom-template-shadowRootReferenceTarget">shadowRootReferenceTarget</dfn>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is shadowRootReferenceTarget nullable?
Reference Target allows authors to specify an element inside a shadow root to be the target of any ID references referring to the host element. This would enable IDREF attributes such as for and aria-labelledby to refer to elements inside a component's shadow DOM while maintaining encapsulation of the internal details of the shadow DOM.
See the reference target explainer.
At a high level, the spec change consists of these parts:
get-the-attr-associated-element
algorithm (and its array-of-elements form) to follow reference target.get-the-attr-associated-element
.See also the corresponding whatwg/dom#1353 which adds the definition of reference target used in this PR.
(See WHATWG Working Mode: Changes for more details.)
/common-dom-interfaces.html ( diff )
/common-microsyntaxes.html ( diff )
/dom.html ( diff )
/form-control-infrastructure.html ( diff )
/form-elements.html ( diff )
/forms.html ( diff )
/iframe-embed-object.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/input.html ( diff )
/microdata.html ( diff )
/parsing.html ( diff )
/popover.html ( diff )
/scripting.html ( diff )
/tables.html ( diff )