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

Reflection of IDREF attributes will leave stale ID value #8306

Closed
rniwa opened this issue Sep 22, 2022 · 16 comments · Fixed by #8352
Closed

Reflection of IDREF attributes will leave stale ID value #8306

rniwa opened this issue Sep 22, 2022 · 16 comments · Fixed by #8352
Labels
a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response.

Comments

@rniwa
Copy link

rniwa commented Sep 22, 2022

In the following example, document.write will log "label-id" in all three cases even though the target element had moved to be inside a shadow tree:

const element = document.createElement('div');
const label = document.createElement('div');
label.id = 'label-id';
document.body.appendChild(element);
document.body.appendChild(label);
element.ariaLabelledByElements = [label];
document.write(element.getAttribute('aria-labelledby') + '<br>');

const newLabel = document.createElement('div');
newLabel.id = 'label-id'
document.body.insertBefore(newLabel, document.body.firstChild);

document.write(element.getAttribute('aria-labelledby') + '<br>');

const host = document.createElement('div');
document.body.appendChild(host);
host.attachShadow({mode: 'closed'}).appendChild(label);

document.write(element.getAttribute('aria-labelledby') + '<br>');

This doesn't seem right. It's really confusing that setting id-ref leaves id values that are stale and no longer representative of the actually referenced element.

@rniwa
Copy link
Author

rniwa commented Sep 22, 2022

@smaug---- @domenic @annevk

@mrego
Copy link
Member

mrego commented Sep 22, 2022

This seems to be on purpose from the ARIA reflection explainer: https://github.com/WICG/aom/blob/gh-pages/aria-reflection-explainer.md#relationships-are-validated-on-get

@mrego
Copy link
Member

mrego commented Sep 22, 2022

What happens if the element already has the attribute set, and it was not set by attribute reflection, should we remove it when it's moved?

Example:

const element = document.createElement('div');
const label = document.createElement('div');
label.id = 'label-id';
document.body.appendChild(element);
document.body.appendChild(label);
element.setAttribute('aria-labelledby', 'label-id');
document.write(element.getAttribute('aria-labelledby') + '<br>');

const host = document.createElement('div');
document.body.appendChild(host);
host.attachShadow({mode: 'closed'}).appendChild(label);

document.write(element.getAttribute('aria-labelledby') + '<br>');

@annevk
Copy link
Member

annevk commented Sep 22, 2022

Hmm. I hadn't appreciated this when commenting on #8307.

Do we know why we need to set a content attribute at all? Perhaps not doing that is preferable when there's no actual serialization (such as when you have a pointer to an element).

@annevk annevk added the a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response. label Sep 22, 2022
@annevk
Copy link
Member

annevk commented Sep 22, 2022

cc @whatwg/a11y

@alice
Copy link
Contributor

alice commented Sep 27, 2022

I explained my original intent around "sprouting" content attributes here: WICG/aom#181 (comment)

Essentially: having at least an empty string as the content attribute means you can reliably know when to check the IDL attribute to understand element associations.

For example, if you wanted to find all the elements which have an aria-owns association, you could do a querySelectorAll("[aria-owns]") to find all elements with an aria-owns attribute, and then query the ariaOwns IDL attribute for each to get the owned elements (since the IDL attribute is the source of truth). If the content attribute isn't always set, you'd have to write a tree walk instead, checking the ariaOwns IDL attribute for every element in the page.

As for specifically sprouting the IDs, it's nice to have for serialization, as @annevk alludes to - the idea being that if authors have bothered to set IDs on the element/s in the first place, they may as well get serializability for free (assuming all relevant elements have IDs which are in the right scope, etc).

As we were originally designing this, it seemed unlikely enough that IDs would be changing or elements would be moving in and out of scope to design around that edge case and lose the benefit of the feature, rather than just warning authors in documentation about when the values may become stale ("play stupid games, win stupid prizes").

If it is indeed a show stopper, then hopefully we can fall back to at least having empty content attributes reliably, for the reasons I outlined above.

I certainly don't think we want to maintain the reverse mapping in order to update the content attributes when the DOM changes.

The WPT tests do a fairly good job of covering the edge cases around ID values becoming stale, I think.

Perhaps not doing that is preferable when there's no actual serialization (such as when you have a pointer to an element).

I'm not sure I understand this comment, could you possibly rephrase it?

@annevk
Copy link
Member

annevk commented Sep 27, 2022

The problem I see is that best-effort clashes a bit with how APIs normally behave, in that they work consistently and "correctly", even for contrived cases. Here there's a number of contrived cases where things get out-of-sync, which might be understandable to some and confusing for others. That's why I think that maybe we shouldn't try to go for best-effort and instead make it clear that when you use an element pointer there is no serialization. (Or empty string, as you suggest.)

E.g., if a website uses duplicate id attribute values, that mainly works fine. It's just that getElementById() will only return the first. However, with this API you can target the second element, but if you verify yourself using getElementById() you'd still get the first. I suspect it might be easier to explain that this feature doesn't have reflection through ID attributes than that sometimes ID attribute reflection ends up with poor results.

@patrickhlauke
Copy link
Member

not thought through all the implications, but my naive take is that this reminds me of things like <input type="checkbox" checked> ... and how then later testing for hasAttribute('checked') won't reflect the actual "checkedness" for the input once it hits reality/user interactions. having said that, getElementById() is near-ubiquitous in its use, so the possibility that the id may be modified in other ways may be surprising to devs and should definitely be explicitly called out/explained, as well as the "don't mix and match approaches/APIs" (though it may not always be under the dev's direct control ... if they pull in 3rd party JS they may not even be aware that this is happening)

@annevk
Copy link
Member

annevk commented Sep 27, 2022

(Checkedness is reflected by the checked IDL attribute. The checked content attribute is reflected by the defaultChecked IDL attribute. The content attribute should probably have been named defaultchecked or the checked IDL attribute should have been named liveChecked or some such, but that mistake was made a long time ago.)

@rniwa
Copy link
Author

rniwa commented Sep 28, 2022

I think setting empty string seems like a good compromising ground. That would not confuse authors with stale ID values and still indicate that the attribute is in use.

@alice
Copy link
Contributor

alice commented Sep 28, 2022

Making the compromise on the conservative side is ok with me. I do agree that it can feel a bit precarious even just knowing that these cases exist. I can also see that, even if it seems highly unlikely that any given developer would stumble into one of these cases, just having to explain how the content attribute can get out of sync means the documentation necessarily has to be longer and more complicated than it would otherwise.

@annevk
Copy link
Member

annevk commented Sep 30, 2022

@mrego would you be willing to drive the PR for this?

@mrego
Copy link
Member

mrego commented Oct 3, 2022

@mrego would you be willing to drive the PR for this?

If there's agreement yeah, I can prepare a PR, update tests and implementations. :)

mrego added a commit to mrego/html that referenced this issue Oct 4, 2022
To avoid issues with wrong IDs in IDREF attributes
we set the content attribute's value to the empty string
in the setter steps for Element and FrozenArray<Element>.

Fixes whatwg#8306
@cookiecrook
Copy link

Probably need to make sure other ARIA stakeholders are okay with this, since it would mean element reflection doesn't actually reflect anything. @jnurthen @spectranaut

@cookiecrook
Copy link

But I do like seeing spec simplification, so @mrego's linked diff is gorgeous…

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 4, 2022
This implements the agreement on the following HTML issue:
whatwg/html#8306

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 4, 2022
This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 4, 2022
This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 4, 2022
This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978
@alice
Copy link
Contributor

alice commented Oct 4, 2022

Probably need to make sure other ARIA stakeholders are okay with this, since it would mean element reflection doesn't actually reflect anything. @jnurthen @spectranaut

I don't know that I'd think of it as not reflecting anything - if the content attribute is set, the IDL attribute will indeed reflect the computed attr-associated elements based on that attribute, and if the IDL attribute is sent to null then the content attribute is removed.

But I agree, it would be nice to understand what the impact might be of never setting the content attribute based on the IDs of the attr-associated elements, compared to the impact of having the content attribute becoming stale under certain circumstances; even if it's decided (as it seems to be) that the latter is a deal breaker regardless.

mrego added a commit to mrego/html that referenced this issue Oct 5, 2022
To avoid issues with wrong IDs in IDREF attributes
we set the content attribute's value to the empty string
in the setter steps for Element and FrozenArray<Element>.

Fixes whatwg#8306
mrego added a commit to mrego/html that referenced this issue Oct 5, 2022
To avoid issues with wrong IDs in IDREF attributes
we set the content attribute's value to the empty string
in the setter steps for Element and FrozenArray<Element>.

Fixes whatwg#8306
annevk pushed a commit that referenced this issue Oct 7, 2022
Using the ID of the element led to surprising results in a number of edge cases.

Tests: web-platform-tests/wpt#36253.

Fixes #8306
webkit-early-warning-system pushed a commit to mrego/WebKit that referenced this issue Oct 7, 2022
https://bugs.webkit.org/show_bug.cgi?id=245299

This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::setElementAttribute() and Element::setElementsArrayAttribute().

The tests are updated accordingly to the new behavior.

Reviewed by Ryosuke Niwa.

* LayoutTests/accessibility/ARIA-reflection-expected.txt:
* LayoutTests/accessibility/ARIA-reflection.html:
* LayoutTests/fast/custom-elements/reactions-for-aria-element-attributes.html:
* LayoutTests/imported/w3c/web-platform-tests/html/dom/aria-element-reflection-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/dom/aria-element-reflection.html:
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::setElementAttribute):
(WebCore::Element::setElementsArrayAttribute):

Canonical link: https://commits.webkit.org/255267@main
aarongable pushed a commit to chromium/chromium that referenced this issue Oct 7, 2022
This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3934330
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Joanmarie Diggs <jdiggs@igalia.com>
Commit-Queue: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1056288}
mrego added a commit to web-platform-tests/wpt that referenced this issue Oct 7, 2022
This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978

Co-authored-by: Manuel Rego Casasnovas <rego@igalia.com>
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3934330
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Joanmarie Diggs <jdiggs@igalia.com>
Commit-Queue: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1056288}
NOKEYCHECK=True
GitOrigin-RevId: f3d72d87337ce676ab43947a574c8c8ea6bfe998
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 21, 2022
… attributes, a=testonly

Automatic update from web-platform-tests
Set empty string for reflection of IDREF attributes (#36253)

This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978

Co-authored-by: Manuel Rego Casasnovas <rego@igalia.com>
--

wpt-commits: 18ef9afe94b628d25548754216b9636cebada489
wpt-pr: 36253
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 26, 2022
… attributes, a=testonly

Automatic update from web-platform-tests
Set empty string for reflection of IDREF attributes (#36253)

This implements the agreement on the following HTML issue:
whatwg/html#8306

HTML PR is available at:
whatwg/html#8352

It just always set the content attribute to the empty string in
Element::SetElementAttribute() and Element::SetElementArrayAttribute().

The WPT test is updated accordingly to the new behavior.

Bug: 1370977
Test: external/wpt/html/dom/aria-element-reflection.html
Change-Id: I55e64df6ac4c3eb50667c1d0a5ba360a9369c978

Co-authored-by: Manuel Rego Casasnovas <rego@igalia.com>
--

wpt-commits: 18ef9afe94b628d25548754216b9636cebada489
wpt-pr: 36253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response.
Development

Successfully merging a pull request may close this issue.

7 participants
@mrego @cookiecrook @alice @rniwa @patrickhlauke @annevk and others