Add logic for reflecting IDREF (single or FrozenArray) attributes

[alice]

Add logic for reflecting Element? or FrozenArray<Element>? attributes.

---

/common-dom-interfaces.html ( diff )
/infrastructure.html ( diff )

[domenic]

Overall this looks to work pretty great. The algorithms seem to work out well.

Most of the review is editorial; the main content is asking to have an explicit concept for the backing value, instead of using "last set to" and such.

[domenic]

s/is/must be/

[alice]

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.

[domenic]

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.

[alice]

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

[domenic]

I'm trying to say "same tree, or same tree as shadow host" - or maybe "same tree as any shadow-including inclusive ancestor"?

Hmm, I'm going to summon @annevk to help with this one.

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

Right, OK, but we still are defining two IDL attributes in terms of a HTML attribute. I guess this'll be clearer when we define the element-reflect concept; let's defer until then.

[annevk]

I couldn't immediately find context for the question, but I'd compare the https://dom.spec.whatwg.org/#concept-tree-root of the objects in question instead of talking about trees. There's also https://dom.spec.whatwg.org/#concept-shadow-including-root that will effectively "ignore" ShadowRoot as a root.

[domenic]

Issue backreferences: #3219 #3515

[annevk]

Hey Alice, thanks for working on this! I reviewed the getter/setter of reflecting an HTMLElement for now. I suspect the advice might also apply to the sequence variant. I have a couple style nits, but we can sort those out later on (the main thing is that we always use <li><p> and not a sole <li> to hold contents).

[alice]

Actually updated the PR now... sorry for spam!

[domenic]

Pushed some editorial tweaks.

The comments I left were the bigger things I wasn't sure on the intent of. Especially the "If the content attribute is set directly" bullet is troubling; I hope that can be fixed somehow.

I also haven't walked through the behavior fully; the setter for sequences in particular is complicated, and the tree constraints are tricky. But they seem good on the surface, and I hope the tests and implementation will shake out any errors. I can also try to do a more thorough pass after the "If the content attribute is set directly" thing get straightened out, as right now that's throwing some doubt on how things work for me.

Finally, to @annevk, I'll note that we could be doing a lot more cross-referencing to DOM concept's here (e.g. attribute's value, set an attribute, has an attribute, attribute's owner element, etc.) but the HTML spec generally hasn't done that so far, so I don't think we should require it for this PR. If one of us feels ambitious we could add the cross-links before merging, I suppose.

[alice]

Oh no: "Sequences must not be used as the type of an attribute or constant. ... Instead of a writable attribute of a sequence type, it is suggested that a pair of operations to get and set the sequence is used."

https://heycam.github.io/webidl/#idl-sequence

[domenic]

Ah, right, glad Web IDL caught that! We actually want to use FrozenArray<>, not sequence<>.

This will be a little annoying as we can no longer return a new list every time in the getter (i.e. "Return elements.") But that's actually a good fix to make. The spec right now implies that el.attrElements !== el.attrElements, because each call of the getter would return a fresh sequence. That's pretty bad behavior.

The most straightforward thing I can think of is to just have a separate slot, call it [[attrElementsCache]] or similar, which stores the last result of the computation logic ("On getting" steps 3-5). Then, still keep those steps to re-compute the elements list, but do a comparison between the elements list and the value of [[attrElementsCache]] before returning. If they're the same, return the cached value (throwing away elements). If they're different, reset the cached value to the new elements, and return it.

[alice]

@domenic @annevk PTAL?

I reworked it in terms of FrozenArray. Rather than add a new slot, I have instead modified the attribute change steps in each case to compute the new value for the existing slot. Hope that's ok.

I tried using FrozenArray<Element> in the ARIA AriaAttributes IDL and the parser didn't complain, so that's a start...

(Oh - the fooElement naming there is just a preference of @cookiecrook, not necessary for other users of this mechanism.)

[annevk]

It makes sense for the getter to return a FrozenArray (I'm still not sure I like the freezing bit), but the setter presumably still needs to take a sequence<>? (Unfortunately not supported by IDL, a known issue of sorts.) Or would you expect users of this API to create an array, freeze it, and pass it in? Does IDL even do the necessary validation for that?

[domenic]

When you use the FrozenArray<> syntax, IDL automatically converts any incoming JS iterable into an IDL frozen array. Just like when you use the sequence<> syntax, IDL automatically converts any incoming JS iterable into an IDL sequence.

[alice]

Does the rest of the change look good to the two of you?

[annevk]

Mostly looks solid to me, but I'm not entirely convinced the tree traversal is correct for shadow trees. Also found a few other nits.

(Also, I'm guessing the plan is that legacy attributes which simply reflect the ID string instead of an element might be tackled separately later on somehow?)

[annevk]

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?

[alice]

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).

[annevk]

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

[alice]

Right, that was after I rewrote it again :)

[annevk]

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?

[alice]

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.

[alice]

Some examples which may be turned into WPTs at the appropriate time:

[Reflect] attribute Element? apricot;
[Reflect] attribute FrozenArray<Element>? grapes;

<fruit-apricot id="apricotOnFloor"></fruit-apricot>
<fruit-grape id="grapeOnFloor"></fruit-grape>
<dining-table>
  #shadow-root (open)
  | <fruit-apricot id="apricotOnTable"></fruit-apricot>
  | <fruit-grape id="grapeOnTable"></fruit-grape>
  | <centre-piece>
  |   #shadow-root (open)
  |   | <fruit-bowl id="bowl" apricot="apricot" grapes="grape1 grape2">
  |   |   <fruit-apricot></fruit-apricot>
  |   |   <fruit-apricot id="apricot"></fruit-apricot>
  |   |   <fruit-grape id="grape1"></fruit-grape>
  |   |   <fruit-grape id="grape2"></fruit-grape>
  |   |   <fruit-grape></fruit-grape>
  |   | </fruit-bowl>
  |   | <fruit-basket id="basket">
  |   |   #shadow-root (open)
  |   |   | <fruit-apricot id="apricotInBasket"></fruit-apricot>
  |   |   | <fruit-grape id="grapeInBasket"></fruit-grape>
  |   | </fruit-basket>
  | </centre-piece>
</dining-table>

// Let's assume we can magically get elements by their ID in any context

console.log(bowl.apricot);  // logs reference to <fruit-apricot id="apricot">
console.log(bowl.grapes);   // logs [<fruit-grape id="grape1">, <fruit-grape id="grape2">]

// ~~ No ID ~~
bowl.apricot = bowl.firstElementChild;
console.log(bowl.getAttribute("apricot"));  // logs ""

bowl.grapes = [grape1, grape2, bowl.lastElementChild];
console.log(bowl.getAttribute("grapes"));   // logs ""

// ~~ Elements in shadow host's tree ~~
bowl.apricot = apricotOnTable;
console.log(bowl.getAttribute("apricot"));  // logs ""
console.log(bowl.apricot);                  // logs reference to <fruit-apricot id="apricotOnTable"></fruit-apricot>

bowl.grapes = [grape1, grape2, grapeOnTable];
console.log(bowl.getAttribute("grapes"));   // logs ""
console.log(bowl.grapes);                   // logs [<fruit-grape id="grape1">, <fruit-grape id="grape2">, <fruit-grape id="grapeOnTable">]

// ~~ Elements in shadow host's shadow host's tree ~~
bowl.apricot = apricotOnFloor;
console.log(bowl.getAttribute("apricot"));  // logs ""
console.log(bowl.apricot);                  // logs reference to <fruit-apricot id="apricotOnFloor"></fruit-apricot>

bowl.grapes = [grape1, grape2, grapeOnFloor];
console.log(bowl.getAttribute("grapes"));   // logs ""
console.log(bowl.grapes);                   // logs [<fruit-grape id="grape1">, <fruit-grape id="grape2">, <fruit-grape id="grapeOnFloor">]

// ~~ Elements in deeper shadow tree ~~
// Should this be an error, or fail silently? Currently fails silently.
bowl.apricot = apricotInBasket;
console.log(bowl.getAttribute("apricot"));  // logs null
console.log(bowl.apricot);                  // logs null

bowl.grapes = [grape1, grape2, grapeInBasket];
console.log(bowl.getAttribute("grapes"));   // logs "grape1 grape2"
console.log(bowl.grapes);                   // logs [<fruit-grape id="grape1">, <fruit-grape id="grape2">]

// Doesn't matter if the element is the shadow host
basket.apricot = apricotInBasket;
console.log(basket.getAttribute("apricot"));  // logs null
console.log(basket.apricot);                // logs null

basket.grapes = [grape1, grape2, grapeInBasket];
console.log(basket.getAttribute("grapes"));  // logs "grape1 grape2"
console.log(basket.grapes);                 // logs [<fruit-grape id="grape1">, <fruit-grape id="grape2">]

// ~~ Using setAttribute ~~
bowl.setAttribute("apricot", null);
console.log(bowl.apricot);                  // logs null

bowl.setAttribute("grapes", null);
console.log(bowl.grapes);                   // logs null

bowl.setAttribute("apricot", "");
console.log(bowl.apricot);                  // logs null

bowl.setAttribute("grapes", "");
console.log(bowl.grapes);                   // logs []

bowl.setAttribute("apricot", "apricot");
console.log(bowl.apricot);                  // logs reference to <fruit-apricot id="apricot">

bowl.setAttribute("grapes", "grape1 grape2");
console.log(bowl.grapes);                   // logs [<fruit-grape id="grape1">, <fruit-grape id="grape2">]

bowl.setAttribute("grapes", "grape1 grape2 grapeOnTable");
console.log(bowl.getAttribute("grapes"));   // logs "grape1 grape2 grapeOnTable"
console.log(bowl.grapes);                   // logs [<fruit-grape id="grape1">, <fruit-grape id="grape2">]

[alice]

After talking with @robdodson, changed it to allow setting a reference to any element in a less-deeply-nested shadow tree (and changed the examples above to reflect the same).

[alice]

@domenic @annevk Does this make sense?

[annevk]

I think it'd be better to have some kind of flag (probably in the form of an internal slot) that you set before you set the content attribute, this bails out if that flag is set, and then unset it after you've set the content attribute.

For new algorithms we try to make all state an implementation needs explicit and have less "hidden complexity", if you will.

[alice]

Ok, I'll do that.

I'd considered that initially but was thrown off by the idea of adding two internal slots for each of these things, and also that it seemed overly prescriptive when implementors may choose to implement it any number of different ways.

[alice]

Done :)

[domenic]

This has gotten quite complicated, but the spec looks solid, so I'll LGTM this. I trust our deliberative process enough to believe that all the complexity is warranted.

I'm really happy to see those potential future test cases; that's setting us up for great success.

We should probably hold off on merging this until we have a usage of it (either in HTML or in an ARIA-related spec), to validate the design. That will also allow us to write web platform tests.

We also should get explicit multi-implementer support for this. I believe probably it's supported as part of a larger AOM push, but it'd be ideal to get explicit signoff on this particular piece.

But as far as actual spec work on this PR, this seems good to go. Yay!

[alice]

@cookiecrook FYI

[cookiecrook]

@rniwa FYI

[rniwa]

Hm... do we want to remove duplicates from FrozenArray upon assignment (setter) here?

I'm not sure I follow. IDREF-list attributes don't disallow duplicate values as far as I know; unless there's a specific reason to disallow them here, I don't understand why we would add that constraint. (I also can't see any reason why authors would want to be able to use duplicate values, but I'm not that imaginative.)

DOMTokenList de-duplicates the entires. e.g.

span = document.createElement('span');
span.className = 'foo bar foo'
span.classList.length // 2
span.classList[0] // foo
span.classList[1] // bar
span.classList.value // foo bar foo

[alice]

Hm... do we want to remove duplicates from FrozenArray upon assignment (setter) here?

I'm not sure I follow. IDREF-list attributes don't disallow duplicate values as far as I know; unless there's a specific reason to disallow them here, I don't understand why we would add that constraint. (I also can't see any reason why authors would want to be able to use duplicate values, but I'm not that imaginative.)

DOMTokenList de-duplicates the entires. e.g.

span = document.createElement('span');
span.className = 'foo bar foo'
span.classList.length // 2
span.classList[0] // foo
span.classList[1] // bar
span.classList.value // foo bar foo

Thanks for that!

I think in that case de-duplicating makes sense, because the classlist is really more of a class set - adding another instance of the same class, or changing the order, wouldn't make a difference.

With an IDREF list, the order is significant, and adding a duplicate entry would change the behaviour.

For example, if I had something like aria-labelelledby="foo foo", that should cause the contents of the "foo" element to be included twice in the name of the element with the aria-labelledby attribute on it.

Edited to clarify.

[rniwa]

DOMTokenList de-duplicates the entires. e.g.

span = document.createElement('span');
span.className = 'foo bar foo'
span.classList.length // 2
span.classList[0] // foo
span.classList[1] // bar
span.classList.value // foo bar foo

I think in that case de-duplicating makes sense, because the classlist is really more of a class set - adding another instance of the same class, or changing the order, wouldn't make a difference.

With an IDREF list, the order is significant, and adding a duplicate entry would change the behaviour.

For example, if I had something like aria-labelelledby="foo foo", that should cause the contents of the "foo" element to be included twice in the name of the element with the aria-labelledby attribute on it.

Hm... that's interesting. In the case of slot assignment, we need to remove duplicates but that doesn't really have a setter so it works fine there, and I guess there aren't really any other DOM API that uses ID ref list than ARIA?

[annevk]

There is: https://html.spec.whatwg.org/#attr-tdth-headers. That's currently defined to take an unordered set and there's no API for it that would expose order or some such. I guess this would be the first ordered list of IDs. (Note that DOMTokenList is ordered, but it's indeed a set despite the name, not a list.)

[mrego]

This PR has been on hold for more than a year now, so let's do an update.

Chromium and WebKit have now an implementation of this PR under a runtime flag (AOMAriaRelationshipProperties flag in Chromium and AriaReflectionForElementReferencesEnabled flag in WebKit).
Both implementations allow to refer to descendants of your shadow-including ancestors (#6063).

There's a WPT test covering this PR too: https://github.com/web-platform-tests/wpt/blob/master/dom/nodes/aria-element-reflection.tentative.html (Chromium fails one case but that's being currently fixed).

Does anyone remember what's blocking this PR? What else should be done before being ready to merge? Thanks!

[annevk]

I think a fresh (rebased) PR with a filled out PR template (that wasn't part of our setup back in 2018) would be good at this point, properly acknowledging @alice for her contributions toward this feature.

[mrego]

Thanks @annevk, I've just done a rebase and created a new PR: #7934
If I should have updated this PR instead, please let me know how.

[mrego]

This has been moved to #7934, which is about to land; so I guess we can close this one now.