layout: Ensure empty list items are at least as tall as outside markers #32152
Conversation
This file contains 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
|
🔨 Triggering try run (#8853657889) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#8853657889): Flaky unexpected result (13)
Stable unexpected results that are known to be intermittent (16)
Stable unexpected results (5)
|
|
|
Loirooriol
reviewed
Apr 26, 2024
| @@ -257,20 +260,26 @@ impl OutsideMarker { | |||
| }, | |||
| ); | |||
|
|
|||
| // Position the marker beyond the inline start of the border box list item. This needs to | |||
| // take into account the border of the item. | |||
| let pbm_of_list_item = self.list_item_style.padding_border_margin(containing_block); | |||
There was a problem hiding this comment.
But containing_block is the one of the marker, and this should get the one of the list item, right?
There was a problem hiding this comment.
So this is the containing block passed in when laying out the children of the list item. We don't have access to the writing mode of the list item's parent here -- though this will be affected also by an implementation of marker-side. I've modified this to only call border_width() making it explicit that only the writing mode is important here.
What do you recommend?
components/layout_2020/flow/mod.rs
Outdated
Comment on lines
1792
to
1795
| let mut collapsed_through = self.next_in_flow_margin_collapses_with_parent_start_margin; | ||
| if self.marker_block_size.is_some() { | ||
| collapsed_through = false; | ||
| } |
There was a problem hiding this comment.
Doesn't need to be mutable with
Suggested change
| let mut collapsed_through = self.next_in_flow_margin_collapses_with_parent_start_margin; | |
| if self.marker_block_size.is_some() { | |
| collapsed_through = false; | |
| } | |
| let collapsed_through = match self.marker_block_size { | |
| Some(_) => false, | |
| None => self.next_in_flow_margin_collapses_with_parent_start_margin, | |
| }; |
There was a problem hiding this comment.
I've made this change.
mrobinson
force-pushed
the
empty-list-item
branch
from
ee98d8c
to
e786346
Compare
It turns out that the offset from |
|
🔨 Triggering try run (#8858507380) for Linux WPT |
|
|
mrobinson
force-pushed
the
empty-list-item
branch
from
e786346
to
89f9ae5
Compare
|
🔨 Triggering try run (#8859043012) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#8859043012): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (13)
|
|
✨ Try run (#8859043012) succeeded. |
Loirooriol
approved these changes
Apr 29, 2024
components/layout_2020/flow/mod.rs
Outdated
| @@ -257,20 +260,28 @@ impl OutsideMarker { | |||
| }, | |||
| ); | |||
|
|
|||
| // Position the marker beyond the inline start of the border box list item. This needs to | |||
| // take into account the border of the item. | |||
There was a problem hiding this comment.
But also the padding, right? I guess it's fine for now since it's tricky to resolve the percentage, but the comment should mention it
There was a problem hiding this comment.
Oh, right, of course. I've gone back to using PaddingBorderMargin here and taking into account padding. I've also left a comment explaining that the containing block used here is the wrong one and what is wrong about the results.
components/layout_2020/flow/mod.rs
Outdated
Comment on lines
1788
to
1803
| // `self.current_block_direction_position` can be negative here, so we cannot max with zero. | ||
| let block_size_from_marker = self | ||
| .marker_block_size | ||
| .unwrap_or(self.current_block_direction_position); | ||
| let total_block_size = self | ||
| .current_block_direction_position | ||
| .max(block_size_from_marker) | ||
| .into(); | ||
|
|
||
| // If this is a list item (even empty) with an outside marker, then it | ||
| // should not collapse through. | ||
| let collapsed_through = match self.marker_block_size { | ||
| Some(_) => false, | ||
| None => self.next_in_flow_margin_collapses_with_parent_start_margin, | ||
| }; |
There was a problem hiding this comment.
Seems a bit strange to take the maximum of the same value when there is no marker. The logic can bemoved into the match:
Suggested change
| // `self.current_block_direction_position` can be negative here, so we cannot max with zero. | |
| let block_size_from_marker = self | |
| .marker_block_size | |
| .unwrap_or(self.current_block_direction_position); | |
| let total_block_size = self | |
| .current_block_direction_position | |
| .max(block_size_from_marker) | |
| .into(); | |
| // If this is a list item (even empty) with an outside marker, then it | |
| // should not collapse through. | |
| let collapsed_through = match self.marker_block_size { | |
| Some(_) => false, | |
| None => self.next_in_flow_margin_collapses_with_parent_start_margin, | |
| }; | |
| let (total_block_size, collapsed_through) = match self.marker_block_size { | |
| Some(marker_block_size) => ( | |
| self.current_block_direction_position.max(marker_block_size), | |
| false, | |
| ), | |
| None => ( | |
| self.current_block_direction_position, | |
| self.next_in_flow_margin_collapses_with_parent_start_margin, | |
| ), | |
| }; |
mrobinson
force-pushed
the
empty-list-item
branch
from
89f9ae5
to
120b2c2
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
While <https://drafts.csswg.org/css-lists/#list-style-position-property> says: > The size or contents of the marker box may affect the height of the > principal block box and/or the height of its first line box, and in some > cases may cause the creation of a new line box; this interaction is also > not defined. All other browsers ensure that the first line of list item content is the same block size as the marker. Doing this is complicated, but we can ensure that the entire list item is at least as tall as the marker. This should handle the majority of cases and we can make refinements later for stranger situations, such as when the marker is very tall. Co-authored-by: Oriol Brufau <obrufau@igalia.com>
mrobinson
force-pushed
the
empty-list-item
branch
from
120b2c2
to
38b26f1
Compare
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 https://drafts.csswg.org/css-lists/#list-style-position-property says:
All other browsers ensure that the first line of list item content is
the same block size as the marker. Doing this is complicated, but we can
ensure that the entire list item is at least as tall as the marker. This
should handle the majority of cases and we can make refinements later
for stranger situations, such as when the marker is very tall.
Co-authored-by: Oriol Brufau obrufau@igalia.com
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors