script: Complete basic implementation of delete command#43082
Merged
Conversation
|
🔨 Triggering try run (#22799697773) for Linux (WPT) |
804a73a to
1168f1a
Compare
|
Test results for linux-wpt from try job (#22799697773): Flaky unexpected result (26)
Stable unexpected results that are known to be intermittent (14)
Stable unexpected results (24)
|
|
|
While we are not there yet, this is the majority of the implementation of the delete command. What's missing: 1. Handling of style attributes (most of the test failures) 2. Handling of lists Since these are quite complicated on their own, deferring that to their own PR. Also added more assertions regarding parent nodes, which should always exist in the spec. By adding those, discovered that the logic for `is_visible` was wrong where its display value wasn't checked for `"none"`. Part of servo#25005 Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
1168f1a to
cf9b1c2
Compare
jdm
approved these changes
Mar 11, 2026
Comment on lines
+237
to
+248
| if offset == 0 && | ||
| (start_offset > 0 && | ||
| start_node | ||
| .children() | ||
| .nth(start_offset as usize - 1) | ||
| .is_some_and(|child| { | ||
| child.is::<HTMLHRElement>() || | ||
| (child.is::<HTMLBRElement>() && | ||
| child.GetPreviousSibling().is_some_and(|previous| { | ||
| previous.is::<HTMLBRElement>() || !previous.is_inline_node() | ||
| })) | ||
| })) |
Contributor
Author
There was a problem hiding this comment.
Yeah, this spec ain't pretty
| }) { | ||
| if self.is::<Element>() && | ||
| self.resolved_display_value().is_some_and(|display| { | ||
| display != DisplayOutside::Inline && display != DisplayOutside::None |
Member
There was a problem hiding this comment.
FYI, we should be checking the inside value too:
- inline -> inline & flow
- inline-block -> inline & flow-root
- inline-table -> inline & table
- none -> none
Contributor
Author
There was a problem hiding this comment.
Hm I am not sure. Since there are two possibilities (it's block or it's inline), I think the intention of the spec is to check for the latter. The spec is old and predates flex/grid, hence I don't think it knew about it back then. Other browsers also only check the outside value as far as I know.
I will keep this as-is for now and once I implement the other missing pieces, revisit this to see if there's test coverage. If there is and we are passing, we are good to go. Otherwise I fix it.
offline-ant
pushed a commit
to offline-ant/havi
that referenced
this pull request
Jun 4, 2026
While we are not there yet, this is the majority of the implementation of the delete command. What's missing: 1. Handling of style attributes (most of the test failures) 2. Handling of lists Since these are quite complicated on their own, deferring that to their own PR. Also added more assertions regarding parent nodes, which should always exist in the spec. By adding those, discovered that the logic for `is_visible` was wrong where its display value wasn't checked for `"none"`. Part of servo#25005 Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com> (cherry picked from commit c1c5e93)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While we are not there yet, this is the majority of the implementation of the delete command.
What's missing:
Since these are quite complicated on their own, deferring that to their own PR.
Also added more assertions regarding parent nodes, which should always exist in the spec. By adding those, discovered that the logic for
is_visiblewas wrong where its display value wasn't checked for"none".Part of #25005