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

DOM: Make moveBefore() throw for all pre-move validity checks #48642

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Oct 16, 2024

This CL makes moveBefore() match the spec PR 1, with regard to the
agreed-upon error-throwing behavior for all pre-moving validity and
hierarchy conditions. This means throwing an exception for:

  • Disconnected parent destination or move target
  • Cross-document Nodes
  • Destination parent that is not an Element node
  • Move target that is not an Element or character data

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: Iaf5243fb2762e21ede068a222600bd158859fe92
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5935350
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369303}

This CL makes moveBefore() match the spec PR [1], with regard to the
agreed-upon error-throwing behavior for all pre-moving validity and
hierarchy conditions. This means throwing an exception for:
 - Disconnected parent destination or move target
 - Cross-document Nodes
 - Destination parent that is not an Element node
 - Move target that is not an Element or character data

[1]: whatwg/dom#1307

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: Iaf5243fb2762e21ede068a222600bd158859fe92
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5935350
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369303}
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

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

The review process for this patch is being conducted in the Chromium project.

@dev-ansung
Copy link
Contributor

WPT Command: python3 ./wpt run --channel=nightly --verify --verify-no-chaos-mode --verify-repeat-loop=0 --verify-repeat-restart=10 --github-checks-text-file=/home/test/artifacts/checkrun.md --affected base_head --log-mach-level=info --log-mach=- -y --no-pause --no-restart-on-unexpected --install-fonts --no-headless --verify-log-full --binary=/home/test/build/firefox/firefox firefox

Some affected tests had inconsistent (flaky) results:

Unstable results

Test Subtest Results Messages
/dom/nodes/moveBefore/tentative/css-transition-cross-document.html OK: 1/10, TIMEOUT: 9/10
/dom/nodes/moveBefore/tentative/css-transition-cross-document.html Moving a transition across documents should reset its state FAIL: 1/10, TIMEOUT: 9/10 Test timed out;assert_throws_dom: function "() => {\n document.querySelector("#new-parent").moveBefore(item, null);\n }" threw object "TypeError: document.querySelector(...).moveBefore is not a function" that is not a DOMException HIERARCHY_REQUEST_ERR: property "code" is equal to undefined, expected 3

These may be pre-existing or new flakes. Please try to reproduce (see the above WPT command, though some flags may not be needed when running locally) and determine if your change introduced the flake. If you are unable to reproduce the problem, please tag @web-platform-tests/wpt-core-team in a comment for help.

@dev-ansung
Copy link
Contributor

Created crbug.com/374788499
@KyleJu @jcscottiii can you help admin merge, thanks!

@KyleJu KyleJu merged commit 06b6ff6 into master Oct 21, 2024
17 of 20 checks passed
@KyleJu KyleJu deleted the chromium-export-cl-5935350 branch October 21, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants