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

Set empty string for reflection of IDREF attributes #8352

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

mrego
Copy link
Member

@mrego mrego commented 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.

Fixes #8306


/common-dom-interfaces.html ( diff )

@mrego
Copy link
Member Author

mrego commented Oct 4, 2022

I guess it'd be nice to wait for ARIA folks to review this too as @cookiecrook suggested on the issue (CC @jnurthen @spectranaut).

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request 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 pull request 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 pull request 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
@annevk annevk added normative change do not merge yet Pull request must not be merged per rationale in comment a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response. labels Oct 4, 2022
source Show resolved Hide resolved
source Show resolved Hide resolved
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 mrego force-pushed the idref-reflection-empty-string branch from d28581c to 9bacd9d Compare October 5, 2022 07:23
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks! I think we're good with respect to implementer support, but would be good to have an explicit okay from the a11y experts you pinged as well.

@mrego
Copy link
Member Author

mrego commented Oct 5, 2022

Thanks! I think we're good with respect to implementer support, but would be good to have an explicit okay from the a11y experts you pinged as well.

Thanks for the quick reviews.

Both Chromium and WebKit patches have been approved, so it looks we're good regarding that.

We'll wait for the accessibility folks to chime in here before merging anything.

@mrego
Copy link
Member Author

mrego commented Oct 7, 2022

Yesterday this was discussed in the AOM meeting and people there were fine with the proposal.

@cyns was wondering how things would look like in DevTools DOM tree view, but it looks like things could be improved there (I've reported an issue on Chromium about that: https://crbug.com/1372365), and it's something that is already happening when you reference something in a shadow-including ancestor.

@annevk annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Oct 7, 2022
@annevk
Copy link
Member

annevk commented Oct 7, 2022

Thanks for the quick turnaround!

@annevk annevk merged commit 30c2607 into whatwg:main Oct 7, 2022
@mrego mrego deleted the idref-reflection-empty-string branch October 7, 2022 12:54
webkit-early-warning-system pushed a commit to mrego/WebKit that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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
This pull request was closed.
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. normative change
Development

Successfully merging this pull request may close these issues.

Reflection of IDREF attributes will leave stale ID value
2 participants