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

Add logic for reflecting IDREF (single or FrozenArray) attributes #3917

Closed
wants to merge 20 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
271 changes: 270 additions & 1 deletion source
Original file line number Diff line number Diff line change
Expand Up @@ -2622,6 +2622,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li><dfn data-x-href="https://heycam.github.io/webidl/#include">include</dfn></li>
<li><dfn data-x-href="https://heycam.github.io/webidl/#dfn-inherit">inherit</dfn></li>
<li><dfn data-x-href="https://heycam.github.io/webidl/#dfn-interface-prototype-object">interface prototype object</dfn></li>
<li><dfn data-x-href="https://heycam.github.io/webidl/#implements">implements</dfn></li>
<li><dfn data-x-href="https://heycam.github.io/webidl/#es-platform-objects">[[Realm]] field of a platform object</dfn></li>
<li><dfn data-x-href="https://heycam.github.io/webidl/#dfn-callback-context">callback context</dfn></li>
<li><dfn data-x-href="https://heycam.github.io/webidl/#dfn-frozen-array-type">frozen array</dfn> and
Expand Down Expand Up @@ -2940,6 +2941,8 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li>The <dfn data-x-href="https://dom.spec.whatwg.org/#concept-tree-child" data-x="concept-tree-child">child</dfn> concept</li>
<li>The <dfn data-x-href="https://dom.spec.whatwg.org/#concept-tree-root">root</dfn> and <dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-root">shadow-including root</dfn> concepts</li>
<li>The <dfn data-x-href="https://dom.spec.whatwg.org/#concept-tree-inclusive-ancestor">inclusive ancestor</dfn>,
<dfn data-x-href="https://dom.spec.whatwg.org/#concept-tree-descendant">descendant</dfn>,
<dfn data-x="concept-shadow-including-ancestor" data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-ancestor">shadow-including ancestor</dfn>,
<dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-descendant">shadow-including descendant</dfn>,
<dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-descendant">shadow-including inclusive descendant</dfn>, and
<dfn data-x-href="https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-ancestor">shadow-including inclusive ancestor</dfn> concepts</li>
Expand Down Expand Up @@ -7218,8 +7221,274 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
return a <code>DOMTokenList</code> object whose associated element is the element in question and
whose associated attribute's local name is the name of the attribute in question.</p>

</div>
<p>If a reflecting IDL attribute <var>attr</var> has the type <code
data-x=""><var>T</var>?</code>, where <var>T</var> is either <code>Element</code> or an
interface that inherits from <code>Element</code>, then:

<ul>
<li><p>Elements of the type this IDL attribute appears on have an <dfn>explicitly set
<var>attr</var>-element</dfn>, which is an element or null. It is initially null.</p></li>

<li>
<p>Elements of the type this IDL attribute appears on have an <dfn data-export=""><var>attr</var>-associated
element</dfn>. To compute the <span><var>attr</var>-associated element</span> for such an
element <var>element</var>:</p>

<ol>
<li>
<p>If <var>element</var>'s <span>explicitly set <var>attr</var>-element</span> is not null:</p>
<ul>
<li><p>If <var>element</var>'s <span>explicitly set <var>attr</var>-element</span> is a
<span>descendant</span> of any of <var>element</var>'s <span
data-x="concept-shadow-including-ancestor">shadow-including ancestors</span>, then return
<var>element</var>'s <span>explicitly set <var>attr</var>-element</span>.</p></li>
<li><p>Otherwise, return null.</p></li>
</ul>
</li>

<li>
<p>Otherwise, if the content attribute is present on <var>element</var>, then return the first
element <var>candidate</var>, in <span>tree order</span>, that meets the following
criteria:</p>

<ul class="brief">
<li><var>candidate</var>'s <span>root</span> is the same as <var>element</var>'s
<span>root</span>,</li>
<li><var>candidate</var>'s <span data-x="concept-ID">ID</span> is the value of the content
attribute, and</li>
<li><var>candidate</var> <span>implements</span> <var>T</var>.</li>
</ul>

<p>If no such element exists, then return null.</p>
</li>

<li><p>Return null.</p></li>
</ol>

<p class="note">Other parts of this specification, or other specifications using attribute
reflection, are generally expected to consult the <span><var>attr</var>-associated
element</span>. The <span>explicitly set <var>attr</var>-element</span> is an internal
implementation detail of the <span><var>attr</var>-associated element</span>, and is not to be
used directly.</p>
</li>

<li><p>On getting, the IDL attribute must return this element's <span><var>attr</var>-associated
element</span>.</p></li>

<li>
<p>On setting, the IDL attribute must perform the following steps:</p>
<ol>
<li><p>Let <var>id</var> be the empty string.</p></li>

<li><p>If the given value:</p>
<ul class="brief">
<li>has the same <span>root</span> as this element, and</li>
<li>has an <code data-x="attr-id">id</code> attribute, and</li>
<li>is the first element in this element's <span>node tree</span> whose <span
data-x="concept-ID">ID</span> is the value of that <code data-x="attr-id">id</code>
attribute,</li>
</ul>
<p>then set <var>id</var> to the given value's <span data-x="concept-ID">ID</span>.</p>

<li><p>Set the content attribute's value for this element to <var>id</var>.</p></li>

<li><p>Set this element's <span>explicitly set <var>attr</var>-element</span> to the given
value.</p></li>
</ol>
</li>

<li>
<p>The following <span data-x="concept-element-attributes-change-ext">attribute change
steps</span> are used to synchronize between the content attribute and the IDL attribute:</p>
<ol>
<li><p>If <var>localName</var> is not the content attribute's local name, or
<var>namespace</var> is not null, then return.</p></li>

<li><p>Set <var>element</var>'s <span>explicitly set <var>attr</var>-element</span> to
null.</p></li>
</ol>
</li>
</ul>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need a slightly new paradigm, since conventional reflection is more of a 1:1 relationship. Maybe something like

In addition to simple [reflection], a pair of IDL attributes can be said to element-reflect a content attribute. This means that (brief summary description of the behavior, if possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring this for now.

<p>If a reflecting IDL attribute <var>attr</var> has the type <code
domenic marked this conversation as resolved.
Show resolved Hide resolved
data-x="">FrozenArray&lt;<var>T</var>&gt;?</code>, where <var>T</var> is either
<code>Element</code> or an interface that inherits from <code>Element</code>, then:</p>

<ul>
<li><p>Elements of the type this IDL attribute appears on have <dfn>explicitly set
domenic marked this conversation as resolved.
Show resolved Hide resolved
<var>attr</var>-elements</dfn>, which is either a <span>list</span> of elements or null. It is
domenic marked this conversation as resolved.
Show resolved Hide resolved
initially null.</p></li>
domenic marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/is/must be/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm trying to define attrElement in terms of attr, so it's kind of tautological to say attr must be named attr, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're defining two things here: the attr IDL element, and the attrElement IDL element. Both of them are in terms of the attr content attribute. In particular I think of this bullet point as explaining the preconditions for using the element-reflect concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content attribute may be named something other than attr, though - case in point being for vs htmlFor and htmlForElement.

<li><p>Elements of the type this IDL attribute appears on have <dfn>cached
<var>attr</var>-associated elements</dfn>, which is a <code
data-x="">FrozenArray&lt;<code>Element</code>&gt;?</code>. It is initially null.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "On getting attr attribute, the IDL attribute must return...", say "On getting the attr IDL attribute, return..."

Similar phrasing tweaks for the rest of them. It should always be "On [get|set]ing the X IDL attribute, [imperative phrasing]".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

domenic marked this conversation as resolved.
Show resolved Hide resolved

<li>
<p>Elements of the type this IDL attribute appears on have <dfn
data-export=""><var>attr</var>-associated elements</dfn>. To compute the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not great. The better model is to say something in the intro to this element-reflect concept like "The element to which the content attribute belongs has an attr-associated element", and then operate on that concept. (Better names appreciated for that concept.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring this for now.

<span><var>attr</var>-associated elements</span> for such an element <var>element</var>:</p>

<ol>
<li><p>Let <var>elements</var> be an empty <span>list</span>.</p></li>

domenic marked this conversation as resolved.
Show resolved Hide resolved
<li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make this a bit nicer by saying "On setting the attr attribute to newValue" and then using newValue instead of "the specified value"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<p>If <var>element</var>'s <span>explicitly set <var>attr</var>-elements</span> is not
null, then:</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not previous, current.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<ol>
<li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the "attr-associated element" concept, this can be made more precise. Probably setting it to null.

Also, instead of saying "should", you need to say "must", or better yet, just use the imperative form ("delete" or "set"). In general all the below steps should be rephrased into imperative form.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased as imperative; deferred the attr-associated element concept part.

<p><span data-x="list iterate">For each</span> <var>attrElement</var> in the <var>element</var>'s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the same phrasing as below: "remove the content attribute and return"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<span>explicitly set <var>attr</var>-elements</span>:</p>
<ol>
<li><p>If <var>attrElement</var> is not a <span>descendant</span> of any of <var>element</var>'s <span
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should include this step. "valid value" would need to be defined. Probably it's better to just let the next step find nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

data-x="concept-shadow-including-ancestor">shadow-including ancestors</span>, then
<span>continue</span>.</p></li>
<li><p>Add <var>attrElement</var> to <var>elements</var>.</p></li>
</ol>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to rephrase this not to call the public API later. But for now it'd make things clearer to remove the document., because the root may be a shadow root, in particular.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworded the text from the definition of getElementById.

</li>
</ol>
</li>
<li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/it is type-compatible with the/it implements the interface given by the type of the/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<p>Otherwise:</p>
<ol>
<li><p>If the content attribute is not present on <var>element</var>, return null.</p></li>

<li><p>Let <var>tokens</var> be the content attribute's value, <span data-x="split a string on
ASCII whitespace">split on ASCII whitespace</span>.

<li>
<p><span data-x="list iterate">For each</span> <var>id</var> in <var>tokens</var>:</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again you can use newValue and then use that in place of "the given element".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (used newElement).

<ol>
<li>
<p>Let <var>candidate</var> be the first element, in <span>tree order</span>, that meets the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"light tree" is not a defined concept these days. I think the first clause is probably enough?

Here and below, it appears the canonical phrasing is "X and Y are [not] in the same tree".

Copy link
Contributor Author

@alice alice Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

following criteria:</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type-compatibility is guaranteed here by the IDL layer, so this is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


<ul class="brief">
<li><var>candidate</var>'s <span>root</span> is the same as <var>element</var>'s
<span>root</span>,</li>
<li><var>candidate</var>'s <span data-x="concept-ID">ID</span> is <var>id</var>, and</li>
<li><var>candidate</var> <span>implements</span> <var>T</var>.</li>
</ul>

<p>If no such element exists, then <span>continue</span>.</p>
</li>
annevk marked this conversation as resolved.
Show resolved Hide resolved
domenic marked this conversation as resolved.
Show resolved Hide resolved

<li><p><span data-x="list append">Append</span> <var>candidate</var> to
domenic marked this conversation as resolved.
Show resolved Hide resolved
<var>elements</var>.</p></li>
</ol>
domenic marked this conversation as resolved.
Show resolved Hide resolved
</li>
</ol>
</li>

<li><p>Return <var>elements</var>.</p></li>
</ol>
</li>

<li>
<p class="note">Other parts of this specification, or other specifications using attribute
domenic marked this conversation as resolved.
Show resolved Hide resolved
reflection, are generally expected to consult the <span><var>attr</var>-associated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if you're two shadow trees deep, the shadow tree in the middle would not be considered here. That's wrong, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I've tried to rewrite it so it can be in the element's tree or the host's tree - I'm not sure if we want/need to allow arbitrary levels of nesting here (e.g. host's host's host's tree).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text that there's there, "descendant of a shadow-including ancestor", would allow that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that was after I rewrote it again :)

elements</span>. The <span>explicitly set <var>attr</var>-elements</span> are an internal
domenic marked this conversation as resolved.
Show resolved Hide resolved
implementation detail of the <span><var>attr</var>-associated elements</span>, and are not to be
used directly. Similarly, the <span>cached <var>attr</var>-associated elements</span> are an
internal implementation detail of the IDL attribute's getter.</p>
</li>

<li>
annevk marked this conversation as resolved.
Show resolved Hide resolved
<p>On getting, the IDL attribute must perform the following steps:</p>

domenic marked this conversation as resolved.
Show resolved Hide resolved
<ol>
<li><p>Let <var>elements</var> be this element's <span><var>attr</var>-associated
annevk marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this happen? IDL would throw, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. This could not be reached, and should be removed. We should add tests that it just throws without resetting any internal state.

elements</span>.</p></li>

<li><p>If the contents of <var>elements</var> is equal to the contents of this element's
domenic marked this conversation as resolved.
Show resolved Hide resolved
<span>cached <var>attr</var>-associated elements</span>, then return this element's
<span>cached <var>attr</var>-associated elements</span>.</p></li>

<li><p>Let <var>elementsAsFrozenArray</var> be <var>elements</var>, <span
data-x="concept-idl-convert">converted</span> to a <code
data-x="">FrozenArray&lt;<var>T</var>&gt;?</code>.</p></li>
domenic marked this conversation as resolved.
Show resolved Hide resolved

<li><p>Set this element's <span>cached <var>attr</var>-associated elements</span> to
domenic marked this conversation as resolved.
Show resolved Hide resolved
<var>elementsAsFrozenArray</var>.</p></li>

domenic marked this conversation as resolved.
Show resolved Hide resolved
<li><p>Return <var>elementsAsFrozenArray</var>.</p></li>
</ol>

<p class="note">This extra caching layer is necessary to preserve the invariant that <code
data-x="">element.reflectedElements === element.reflectedElements</code>.</p>
annevk marked this conversation as resolved.
Show resolved Hide resolved
</li>

<li>
<p>On setting, the IDL attribute must perform the following steps:</p>
<ol>
<li>
<p>If the given value is null:</p>
annevk marked this conversation as resolved.
Show resolved Hide resolved
<ol>
<li><p>Set this element's <span>explicitly set <var>attr</var>-elements</span> to
null.</p></li>

domenic marked this conversation as resolved.
Show resolved Hide resolved
<li><p>Remove the content attribute from this element.</p></li>

<li><p>Return.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this bit makes it unlike other reflecting attributes. Perhaps this is better introduced as a separate macro-mechanism. E.g., element x supports element-pointer attribute y or some such. As reflecting IDL attributes generally modify the content attribute and the behavior is then defined in terms of those. Though I guess @domenic saw this differently so maybe it's okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reworked this a bit now and the attribute change steps are much more minimal. The same objection may well still apply, though.

</ol>
annevk marked this conversation as resolved.
Show resolved Hide resolved
</li>

<li><p>Let <var>value</var> be an empty string.</p></li>

<li>
<p><span data-x="list iterate">For each</span> <var>element</var> in the given value:</p>
<ol>
<li><p>If <var>value</var> is empty and <var>elements</var> is non-empty, then <span
data-x="list append">append</span> <var>element</var> to <var>elements</var> and
<span>continue</span>.</p></li>

<li><p><span data-x="list append">Append</span> <var>element</var> to
domenic marked this conversation as resolved.
Show resolved Hide resolved
<var>elements</var>.</p></li>

<li>
<p>If <var>element</var>:</p>

<ul class="brief">
<li>does not have the same <span>root</span> as this element, or</li>
<li>has no <code data-x="attr-id">id</code> attribute, or</li>
domenic marked this conversation as resolved.
Show resolved Hide resolved
<li>is not the first element in this element's <span>node tree</span> whose <span
data-x="concept-ID">ID</span> is the value of that <code data-x="attr-id">id</code>
attribute,</li>
</ul>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be used for the empty list? Perhaps it's better to not make this sequence thing nullable and use the empty list as a default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think we probably do want to be able to disambiguate between "attribute not set/unset" and "attribute is empty" - for example, there is a big distinction between <img alt=""> and <img> with no alt. So, I think nullable is correct.

Currently the empty list is handled - the for loop just runs zero times.


<p>then set <var>value</var> to the empty string, and <span>continue</span>.</p>
</li>

<li><p>Let <var>id</var> be <var>element</var>'s <span
data-x="concept-ID">ID</span>.</p></li>

<li><p>If <var>value</var> is not the empty string, then append U+0020 SPACE to
<var>value</var>.</p></li>

<li><p>Append <var>id</var> to <var>value</var>.</p></li>
</ol>
</li>

<li><p>Set the content attribute's value for this element to <var>value</var>.</p></li>

<li><p>Set this element's <span>explicitly set <var>attr</var>-elements</span> to
<var>elements</var>.</p></li>
domenic marked this conversation as resolved.
Show resolved Hide resolved
</ol>
</li>

<li>
<p>The following <span data-x="concept-element-attributes-change-ext">attribute change
steps</span> are used to synchronize between the content attribute and the IDL attribute:</p>
<ol>
<li><p>If <var>localName</var> is not the content attribute's local name, or
<var>namespace</var> is not null, then return.</p></li>

<li><p>Set <var>element</var>'s <span>explicitly set <var>attr</var>-elements</span> to
null.</p></li>
</ol>
</li>
</ul>

</div>
domenic marked this conversation as resolved.
Show resolved Hide resolved

<h4>Collections</h4>

Expand Down