-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Comments
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 |
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>'); |
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). |
cc @whatwg/a11y |
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 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.
I'm not sure I understand this comment, could you possibly rephrase it? |
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 |
not thought through all the implications, but my naive take is that this reminds me of things like |
(Checkedness is reflected by the |
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. |
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. |
@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. :) |
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
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 |
But I do like seeing spec simplification, so @mrego's linked diff is gorgeous… |
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
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
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
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
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. |
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
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
Using the ID of the element led to surprising results in a number of edge cases. Tests: web-platform-tests/wpt#36253. Fixes #8306
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
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 <[email protected]> Reviewed-by: Joanmarie Diggs <[email protected]> Commit-Queue: Manuel Rego <[email protected]> Cr-Commit-Position: refs/heads/main@{#1056288}
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 <[email protected]>
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 <[email protected]> Reviewed-by: Joanmarie Diggs <[email protected]> Commit-Queue: Manuel Rego <[email protected]> Cr-Commit-Position: refs/heads/main@{#1056288} NOKEYCHECK=True GitOrigin-RevId: f3d72d87337ce676ab43947a574c8c8ea6bfe998
… 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 <[email protected]> -- wpt-commits: 18ef9afe94b628d25548754216b9636cebada489 wpt-pr: 36253
… 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 <[email protected]> -- wpt-commits: 18ef9afe94b628d25548754216b9636cebada489 wpt-pr: 36253
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:
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.
The text was updated successfully, but these errors were encountered: