Add logic for reflecting IDREF attributes

[mrego]

This defines IDL reflection for single and FrozenArray attributes.
This work was done mostly by @alice.

This is just a rebase of the following PR: #3917

• At least two implementers are interested (and none opposed):
  • Chromium has an implementation under AOMAriaRelationshipProperties flag.
  • WebKit has an implementation under AriaReflectionForElementReferencesEnabled flag.
• Tests are written and can be reviewed and commented upon at:
  • https://github.com/web-platform-tests/wpt/blob/master/dom/nodes/aria-element-reflection.tentative.html
• Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=981423
  • Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1769586
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=196843

---

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

[domenic]

This overall looks great. I'm excited to see implementations have happened since I last was reviewing this. I found a few editorial nits but I'd be happy to add this to the spec.

I am not really familiar with the shadow tree details so if you, or another reviewer, wants to confirm that the tests you linked match the spec draft here, that would be helpful.

I don't see tests for the caching invariant, e.g. testing that assert_equals(input1.ariaLabelledByElements, input1.ariaLabelledByElements) (in the array case, not just the trivial null case). Could you be sure those are added?

I will note that since this was originally written, ObservableArray<T> has become a thing, and using it instead of FrozenArray<T> would probably be more friendly to web developers. And I suspect that if we merge this now people won't necessarily put in the work to upgrade it over time, which is sad. But I don't want to stand in the way of people unflagging a feature with two implementations... Let us know if you have any thoughts on that.

[mrego]

Thanks for the quick review. I've applied the suggested changes.

I am not really familiar with the shadow tree details so if you, or another reviewer, wants to confirm that the tests you linked match the spec draft here, that would be helpful.

I'm not a Shadow DOM expert, but the tests seem to cover what this PR mentions. Maybe others could confirm that.

I don't see tests for the caching invariant, e.g. testing that assert_equals(input1.ariaLabelledByElements, input1.ariaLabelledByElements) (in the array case, not just the trivial null case). Could you be sure those are added?

I'm adding them in web-platform-tests/wpt#34095.

I will note that since this was originally written, ObservableArray<T> has become a thing, and using it instead of FrozenArray<T> would probably be more friendly to web developers. And I suspect that if we merge this now people won't necessarily put in the work to upgrade it over time, which is sad. But I don't want to stand in the way of people unflagging a feature with two implementations... Let us know if you have any thoughts on that.

Looks like WebKit doesn't have ObservableArray yet, so that might be a reason why this is still using FrozenArray. Not really sure though.
This has also been discussed in some AOM meetings in the past, but I don't know what was the final reason to go with FrozenArray
Maybe @rniwa has some thoughts on this.

[mrego]

I don't see tests for the caching invariant, e.g. testing that assert_equals(input1.ariaLabelledByElements, input1.ariaLabelledByElements) (in the array case, not just the trivial null case). Could you be sure those are added?

I'm adding them in web-platform-tests/wpt#34095.

On top of that I've added more tests to check this in different situations at web-platform-tests/wpt#34194

And I've also fixed the WebKit implementation so now all the tests pass on WebKit, and the caching invariant works as expected with FrozenArray: https://bugs.webkit.org/show_bug.cgi?id=240563

In addition, Chromium has a TODO to fix this in the code (see this comment).

Giving the above information, is this PR blocked on something else?

[annevk]

I think we need to point out that an element's explicitly set attr-elements are weak references, no? The idea being that if the elements they point to can be collected, this shouldn't hold them alive as the corresponding attr-associated element will already have started returning null.

That also makes it clearer you really cannot depend on explicitly set attr-element. Perhaps we should stress that even more though by prefixing it with "internal" or some such?

I'm concerned as in WICG/aom#192 (comment) there's already a suggestion other things could build on top of these internal details, which I don't think is workable.

cc @smaug----

[domenic]

Great work @mrego!

+1 to @annevk's concerns, especially making sure we're all on the same page that the explicitly set attr-element is an internal implementation detail and not something any other specs or parts of the implementation (including the accessibility tree) should build on.

[mrego]

I think we need to point out that an element's explicitly set attr-elements are weak references, no? The idea being that if the elements they point to can be collected, this shouldn't hold them alive as the corresponding attr-associated element will already have started returning null.

Good idea, also both implementations use weak references. I've uploaded a new commit adding some text about that.

That also makes it clearer you really cannot depend on explicitly set attr-element. Perhaps we should stress that even more though by prefixing it with "internal" or some such?

I'm concerned as in WICG/aom#192 (comment) there's already a suggestion other things could build on top of these internal details, which I don't think is workable.

I've already mentioned there that the idea should be discarded, it was just an idea on the last AOM meeting and people were not very aware of the this spec text.

I have the feeling that the spec text is pretty clear with the 2 notes:

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

   <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
   elements</span>. The <span>explicitly set <var>attr</var>-elements</span> are an internal
   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>

TBH, I'm not sure if naming them internal would have encouraged not to think on the possibility that was discussed in WICG/aom#192 (comment). Anyway if you think that it's going to help I guess we can just add "internal" to both the explicitly set and the cached one.

WDYT?

[annevk]

Thanks @mrego! I think you're correct that the notes should suffice. I have a number of tiny nits, but overall this looks great.

[annevk]

@domenic I was hoping you could do a final pass and land this.

[domenic]

Only one substantive comment, which is that I believe we forgot the null handling for the T? case. Everything else is minor style updates (which I would have fixed myself before landing if it weren't for the more substantive issue.)

[domenic]

I think this would be slightly nicer as two nested steps. On first glance I missed the "continue" which made the next step quite confusing.

[mrego]

I've tried to do this, but not totally sure if that was what you were looking for, please check.

[domenic]

LGTM! Will merge tomorrow.