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.

Sign up for GitHub

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

Jump to bottom

Fix error (resulting from confusing text) in auto directionality traversal #9853

Closed
wants to merge 1 commit into from
Closed

Fix error (resulting from confusing text) in auto directionality traversal #9853

wants to merge 1 commit into from

Conversation

dbaron
Copy link
Member

@dbaron dbaron commented Oct 13, 2023 •
edited by pr-preview bot
Loading

This fixes an unintended behavior in the auto directionality traversal that I wrote in 11dc4c7 (PR #9796, fixing #3699) by failing to edit existing text enough.

This section specifies an algorithm that implementors are expected to turn into a traversal of a subtree that skips parts of the subtree. However, by describing the skipping test as examining ancestors rather than ancestors-or-self, it introduces a subtle difference between the algorithm that an implementor is likely to write and what the spec describes. This change changes the test to ancestors-or-self.

My intent when writing this was that a element in a shadow tree with a dir attribute would be skipped by the traversal, but what I actually wrote was the opposite, specifying that it would provide the result of its dir attribute as the first-strong directionality. (That was my intent because it was slightly easier to implement, I needed to pick one way or the other, and I didn't see any other reasons to pick one or the other.)

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

/dom.html ( diff )

@dbaron
Fix error (resulting from confusing text) in auto directionality trav…
a62348b 
…ersal.

This fixes an unintended behavior in the auto directionality traversal
that I wrote in 11dc4c7 (PR whatwg#9796,
fixing whatwg#3699) by failing to edit existing text enough.

This section specifies an algorithm that implementors are expected to
turn into a traversal of a subtree that skips parts of the subtree.
However, by describing the skipping test as examining ancestors rather
than ancestors-or-self, it introduces a subtle difference between the
algorithm that an implementor is likely to write and what the spec
describes.  This change changes the test to ancestors-or-self.

My intent when writing this was that a <slot> element in a shadow tree
with a dir attribute would be skipped by the traversal, but what I
actually wrote was the opposite, specifying that it would provide the
result of its dir attribute as the first-strong directionality.  (That
was my intent because it was slightly easier to implement, I needed to
pick one way or the other, and I didn't see any other reasons to pick
one or the other.)
@dbaron dbaron requested review from annevk and smaug---- October 13, 2023 13:41
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Oct 18, 2023
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 18, 2023
@dbaron @chromium-wpt-export-bot
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidati…
291b001 
…on case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 18, 2023
@dbaron
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidati…
b6de6df 
…on case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4952754
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Di Zhang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1211624}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 18, 2023
@dbaron @chromium-wpt-export-bot
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidati…
a29ecd7 
…on case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4952754
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Di Zhang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1211624}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 18, 2023
@dbaron @chromium-wpt-export-bot
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidati…
b73abec 
…on case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4952754
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Di Zhang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1211624}
@annevk annevk mentioned this pull request Oct 19, 2023
Closed
5 tasks
@dbaron dbaron mentioned this pull request Oct 19, 2023
Merged
5 tasks
@dbaron
Copy link
Member Author

dbaron commented Oct 19, 2023

Closing, since I've merged this PR and closely related #9863 into a single PR at #9868.

@dbaron dbaron closed this Oct 19, 2023
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 1, 2023
@dbaron @moz-wptsync-bot
Bug 1859855 [wpt PR 42600] - Test <slot dir> inside dir=auto, and fix…
2f0ec82 
… a <slot> dir=auto invalidation case., a=testonly

Automatic update from web-platform-tests
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidation case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4952754
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Di Zhang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1211624}

--

wpt-commits: b73abec846dc498ef0f725c61d1aa2cb52d00748
wpt-pr: 42600
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Nov 2, 2023
@dbaron @moz-wptsync-bot
Bug 1859855 [wpt PR 42600] - Test <slot dir> inside dir=auto, and fix…
3a69b0b 
… a <slot> dir=auto invalidation case., a=testonly

Automatic update from web-platform-tests
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidation case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4952754
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Di Zhang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1211624}

--

wpt-commits: b73abec846dc498ef0f725c61d1aa2cb52d00748
wpt-pr: 42600
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 8, 2023
@marco-c
Bug 1859855 [wpt PR 42600] - Test <slot dir> inside dir=auto, and fix…
f286f97 
… a <slot> dir=auto invalidation case., a=testonly

Automatic update from web-platform-tests
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidation case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4952754
Commit-Queue: David Baron <dbaronchromium.org>
Reviewed-by: Di Zhang <dizhanggchromium.org>
Cr-Commit-Position: refs/heads/main{#1211624}

--

wpt-commits: b73abec846dc498ef0f725c61d1aa2cb52d00748
wpt-pr: 42600

UltraBlame original commit: 31eddf6a0ce5ea01d39c0900710c115826d1be4b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 8, 2023
@marco-c
Bug 1859855 [wpt PR 42600] - Test <slot dir> inside dir=auto, and fix…
f299227 
… a <slot> dir=auto invalidation case., a=testonly

Automatic update from web-platform-tests
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidation case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4952754
Commit-Queue: David Baron <dbaronchromium.org>
Reviewed-by: Di Zhang <dizhanggchromium.org>
Cr-Commit-Position: refs/heads/main{#1211624}

--

wpt-commits: b73abec846dc498ef0f725c61d1aa2cb52d00748
wpt-pr: 42600

UltraBlame original commit: 31eddf6a0ce5ea01d39c0900710c115826d1be4b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 8, 2023
@marco-c
Bug 1859855 [wpt PR 42600] - Test <slot dir> inside dir=auto, and fix…
13a3244 
… a <slot> dir=auto invalidation case., a=testonly

Automatic update from web-platform-tests
Test <slot dir> inside dir=auto, and fix a <slot> dir=auto invalidation case.

This fixes a TODO, and tests the spec editing fix proposed in
whatwg/html#9853.   It then fixes an
invalidation case for insertion/removal of <slot> elements that shows up
in that added test, and also adjusts one existing test's result to match
that fix.

Bug: 576815
Change-Id: I5ce68b1cc9b9b8684a9abaf494c5624601f601bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4952754
Commit-Queue: David Baron <dbaronchromium.org>
Reviewed-by: Di Zhang <dizhanggchromium.org>
Cr-Commit-Position: refs/heads/main{#1211624}

--

wpt-commits: b73abec846dc498ef0f725c61d1aa2cb52d00748
wpt-pr: 42600

UltraBlame original commit: 31eddf6a0ce5ea01d39c0900710c115826d1be4b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk approved these changes

@smaug---- smaug---- Awaiting requested review from smaug----

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dbaron @annevk