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

Fix source insertion/moving/removing steps #11137

Merged
merged 3 commits into from
Mar 19, 2025
Merged

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Mar 16, 2025

This PR closes #11113. While standardizing the moveBefore() integration in #10657, we discovered that no browser's removing or moving steps rely on the 'next sibling' pointer associated with the old element's position. After writing a test, we also discovered that the same goes for the insertion steps. In all cases, when the parent (old parent, for removing/moving steps) is a <picture> element, a relevant mutation is triggered for all <img> children of the parent. This PR updates the spec to reflect this.

(See WHATWG Working Mode: Changes for more details.)


/embedded-content.html ( diff )

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM, not sure if @annevk wants to take a look since he raised the issue, so maybe give him a day or two in case.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 17, 2025
See whatwg/html#11113. Since the test added
here passes in all browsers, we can remove the spec text indicating
that the source element's removing and moving steps somehow keep track
of the previous position's "next sibling" pointer.

See also whatwg/html#11137.

R=domenic

Bug: N/A
Change-Id: I572939ee5f7acb0c8980707a46aa581c449eb96d
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 17, 2025
See whatwg/html#11113. Since the test added
here passes in all browsers, we can remove the spec text indicating
that the source element's removing and moving steps somehow keep track
of the previous position's "next sibling" pointer.

See also whatwg/html#11137.

R=domenic

Bug: N/A
Change-Id: I572939ee5f7acb0c8980707a46aa581c449eb96d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6357453
Reviewed-by: Domenic Denicola <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1433512}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 17, 2025
See whatwg/html#11113. Since the test added
here passes in all browsers, we can remove the spec text indicating
that the source element's removing and moving steps somehow keep track
of the previous position's "next sibling" pointer.

See also whatwg/html#11137.

R=domenic

Bug: N/A
Change-Id: I572939ee5f7acb0c8980707a46aa581c449eb96d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6357453
Reviewed-by: Domenic Denicola <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1433512}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 17, 2025
See whatwg/html#11113. Since the test added
here passes in all browsers, we can remove the spec text indicating
that the source element's removing and moving steps somehow keep track
of the previous position's "next sibling" pointer.

See also whatwg/html#11137.

R=domenic

Bug: N/A
Change-Id: I572939ee5f7acb0c8980707a46aa581c449eb96d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6357453
Reviewed-by: Domenic Denicola <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1433512}
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.

This looks good to me. @smaug---- spotted the original issue and maybe wants to take a look at this fix.

@smaug----
Copy link

is triggered for all <img> children of the parent.

Is that true? Are there tests for this? I'm just looking at https://searchfox.org/mozilla-central/rev/f07a6b1e84a609fbf9746f67be9edd43b3ed3362/dom/html/HTMLPictureElement.cpp#63-70

@domfarolino
Copy link
Member Author

Is that true? Are there tests for this?

See the linked tests in the OP, where I test two img elements inside the picture element.

@smaug----
Copy link

smaug---- commented Mar 17, 2025

That test relies on moveBefore. How do browsers behave when moving nodes without moveBefore?

And if I'm reading wubkat correctly, the PR doesn't match webkit exactly either https://searchfox.org/wubkat/rev/d8448d088fe3ea650b82638e4aacbbcd38b078e5/Source/WebCore/html/HTMLSourceElement.cpp#92-102
But I didn't check if that is in practice just an optimization.

@domfarolino
Copy link
Member Author

The test also uses remove(), ensuring that the "removing steps" and "moving steps" behave the same. I have run the test in all implementations with moveBefore() converted to remove() (so moveBefore() doesn't appear at all) and the tests pass in all implementations, so this spec change is definitely right. I think it's fine for the tests to reflect the latest spec, but if you want me to separate the tests, I can.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 18, 2025
…ion test, a=testonly

Automatic update from web-platform-tests
Add 'next sibling' source relevant mutation test

See whatwg/html#11113. Since the test added
here passes in all browsers, we can remove the spec text indicating
that the source element's removing and moving steps somehow keep track
of the previous position's "next sibling" pointer.

See also whatwg/html#11137.

R=domenic

Bug: N/A
Change-Id: I572939ee5f7acb0c8980707a46aa581c449eb96d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6357453
Reviewed-by: Domenic Denicola <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1433512}

--

wpt-commits: c29189937f2794b23486f6d7b3bdf8326fb05ddb
wpt-pr: 51381
@domenic domenic merged commit be5f269 into main Mar 19, 2025
2 checks passed
@domenic domenic deleted the source-relevant-mutations branch March 19, 2025 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

source element move and removing steps are wrong
4 participants