layout: Add support for table rows, columns, rowgroups and colgroups #31341
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 (#7898358409) for Linux WPT layout-2020 |
|
Test results for linux-wpt-layout-2020 from try job (#7898358409): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (16)
Stable unexpected results (5)
|
|
|
mrobinson
force-pushed
the
table-column-and-colgroup
branch
from
53c5d5e
to
601bcfa
Compare
|
🔨 Triggering try run (#7904250249) for Linux WPT layout-2020 |
|
Test results for linux-wpt-layout-2020 from try job (#7904250249): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (14)
|
|
✨ Try run (#7904250249) succeeded. |
Loirooriol
reviewed
Feb 14, 2024
| .translate(containing_block.origin.to_vector()); | ||
| self.maybe_push_hit_test_for_style_and_tag( | ||
| builder, | ||
| &style, |
There was a problem hiding this comment.
Suggested change
| &style, | |
| style, |
Comment on lines
631
to
634
| fn build_background_image<'b>( | ||
| &mut self, | ||
| builder: &mut DisplayListBuilder, | ||
| source: background::Source<'a>, | ||
| painter: BackgroundPainter<'b>, |
There was a problem hiding this comment.
- fn build_background_image<'b>(
+ fn build_background_image(
&mut self,
builder: &mut DisplayListBuilder,
- painter: BackgroundPainter<'b>,
+ painter: BackgroundPainter<'_>,
Comment on lines
577
to
579
| fn build_background_for_painter<'b>( | ||
| &mut self, | ||
| painter: BackgroundPainter<'b>, | ||
| builder: &mut DisplayListBuilder, |
There was a problem hiding this comment.
Suggested change
| fn build_background_for_painter<'b>( | |
| &mut self, | |
| painter: BackgroundPainter<'b>, | |
| builder: &mut DisplayListBuilder, | |
| fn build_background_for_painter( | |
| &mut self, | |
| painter: BackgroundPainter<_>, | |
| builder: &mut DisplayListBuilder, |
| rect: LogicalRect<Length>, | ||
| style: ServoArc<ComputedValues>, | ||
| ) -> Self { | ||
| let writing_mode = style.writing_mode.clone(); |
There was a problem hiding this comment.
Suggested change
| let writing_mode = style.writing_mode.clone(); | |
| let writing_mode = style.writing_mode; |
Comment on lines
171
to
172
| outer_min_content_width += pbm.padding_border_sums.inline.into(); | ||
| outer_max_content_width += pbm.padding_border_sums.inline.into(); |
There was a problem hiding this comment.
Suggested change
| outer_min_content_width += pbm.padding_border_sums.inline.into(); | |
| outer_max_content_width += pbm.padding_border_sums.inline.into(); | |
| outer_min_content_width += pbm.padding_border_sums.inline; | |
| outer_max_content_width += pbm.padding_border_sums.inline; |
| if cell.effective_vertical_align() == VerticalAlignKeyword::Baseline { | ||
| let baseline = Au::from(cell_rect.start_corner.block) + row_baseline; |
There was a problem hiding this comment.
Suggested change
| let baseline = Au::from(cell_rect.start_corner.block) + row_baseline; | |
| let baseline = cell_rect.start_corner.block + row_baseline; |
mrobinson
force-pushed
the
table-column-and-colgroup
branch
from
601bcfa
to
baf3f75
Compare
|
Thanks @Loirooriol, I think I've fixed those clippy warnings, but there are so many in Layout 2020 it's a bit hard to tell. It would be nice to clean them all up. |
Loirooriol
reviewed
Feb 19, 2024
Comment on lines
157
to
160
| let display = &fragment.style.get_box().display; | ||
| if display.inside() == style::values::specified::box_::DisplayInside::Flow && | ||
| display.outside() == | ||
| style::values::specified::box_::DisplayOutside::Inline |
There was a problem hiding this comment.
This could be
Suggested change
| let display = &fragment.style.get_box().display; | |
| if display.inside() == style::values::specified::box_::DisplayInside::Flow && | |
| display.outside() == | |
| style::values::specified::box_::DisplayOutside::Inline | |
| use style::properties::longhands::display::computed_value::T as Display; | |
| if fragment.style.get_box().display == Display::Inline { |
Comment on lines
+232
to
+237
| for row_group in self.table.row_groups.iter() { | ||
| if row_group.track_range.start > n { | ||
| return row_group.track_range.start - 1; |
There was a problem hiding this comment.
Should this use a binary search?
| } | ||
|
|
||
| if let Some(remove_from) = remove_from { | ||
| self.table.column_groups.truncate(remove_from - 1); |
There was a problem hiding this comment.
Shouldn't this use remove_from?
| } | ||
| } | ||
|
|
||
| fn regenerate_track_ranges(&mut self) { |
There was a problem hiding this comment.
This can be called twice, it could be better to remove the calls from move_row_group_to_front/end, make them return a bool if something changes, and call regenerate_track_ranges from reorder_first_thead_and_tfoot depending on these bools.
Comment on lines
360
to
365
| let removed_row_group: Vec<TableTrackGroup> = self | ||
| .table | ||
| .row_groups | ||
| .splice(index_to_move..index_to_move + 1, std::iter::empty()) | ||
| .collect(); | ||
| self.table.row_groups.splice(0..0, removed_row_group); |
There was a problem hiding this comment.
Seems simpler to do this without splice():
Suggested change
| let removed_row_group: Vec<TableTrackGroup> = self | |
| .table | |
| .row_groups | |
| .splice(index_to_move..index_to_move + 1, std::iter::empty()) | |
| .collect(); | |
| self.table.row_groups.splice(0..0, removed_row_group); | |
| let removed_row_group = self.table.row_groups.remove(index_to_move); | |
| self.table.row_groups.insert(0, removed_row_group); |
Comment on lines
402
to
407
| let removed_row_group: Vec<TableTrackGroup> = self | ||
| .table | ||
| .row_groups | ||
| .splice(index_to_move..index_to_move + 1, std::iter::empty()) | ||
| .collect(); | ||
| self.table.row_groups.extend(removed_row_group); |
There was a problem hiding this comment.
Ditto
Suggested change
| let removed_row_group: Vec<TableTrackGroup> = self | |
| .table | |
| .row_groups | |
| .splice(index_to_move..index_to_move + 1, std::iter::empty()) | |
| .collect(); | |
| self.table.row_groups.extend(removed_row_group); | |
| let removed_row_group = self.table.row_groups.remove(index_to_move); | |
| self.table.row_groups.push(removed_row_group); |
Comment on lines
352
to
365
| let removed_rows: Vec<TableTrack> = self | ||
| .table | ||
| .rows | ||
| .splice(row_range, std::iter::empty()) | ||
| .collect(); | ||
| self.table.rows.splice(0..0, removed_rows); | ||
|
|
||
| // Move the group itself. | ||
| let removed_row_group: Vec<TableTrackGroup> = self | ||
| .table | ||
| .row_groups | ||
| .splice(index_to_move..index_to_move + 1, std::iter::empty()) | ||
| .collect(); | ||
| self.table.row_groups.splice(0..0, removed_row_group); |
There was a problem hiding this comment.
In fact the entire thing may be simpler without splice() at all?
Suggested change
| let removed_rows: Vec<TableTrack> = self | |
| .table | |
| .rows | |
| .splice(row_range, std::iter::empty()) | |
| .collect(); | |
| self.table.rows.splice(0..0, removed_rows); | |
| // Move the group itself. | |
| let removed_row_group: Vec<TableTrackGroup> = self | |
| .table | |
| .row_groups | |
| .splice(index_to_move..index_to_move + 1, std::iter::empty()) | |
| .collect(); | |
| self.table.row_groups.splice(0..0, removed_row_group); | |
| let mut removed_rows = self.table.rows.split_off(row_range.end + 1); | |
| self.table.rows.rotate_right(row_range.len()); | |
| self.table.rows.append(&mut removed_rows); |
Comment on lines
315
to
339
| let mut current_row_group = None; | ||
| for (row_index, row) in self.table.rows.iter().enumerate() { | ||
| if current_row_group == row.group_index { | ||
| continue; | ||
| } | ||
|
|
||
| if let Some(new_group_index) = row.group_index { | ||
| if let Some(new_group) = self.table.row_groups.get_mut(new_group_index) { | ||
| new_group.track_range = row_index..self.table.rows.len(); | ||
| } | ||
| } | ||
|
|
||
| if let Some(current_group_index) = current_row_group { | ||
| if let Some(current_group) = self.table.row_groups.get_mut(current_group_index) { | ||
| current_group.track_range.end = row_index; | ||
| } | ||
| } | ||
|
|
||
| current_row_group = row.group_index; | ||
| } |
There was a problem hiding this comment.
This seems clearer to me like this:
Suggested change
| let mut current_row_group = None; | |
| for (row_index, row) in self.table.rows.iter().enumerate() { | |
| if current_row_group == row.group_index { | |
| continue; | |
| } | |
| if let Some(new_group_index) = row.group_index { | |
| if let Some(new_group) = self.table.row_groups.get_mut(new_group_index) { | |
| new_group.track_range = row_index..self.table.rows.len(); | |
| } | |
| } | |
| if let Some(current_group_index) = current_row_group { | |
| if let Some(current_group) = self.table.row_groups.get_mut(current_group_index) { | |
| current_group.track_range.end = row_index; | |
| } | |
| } | |
| current_row_group = row.group_index; | |
| } | |
| let mut current_row_group_index = None; | |
| for (row_index, row) in self.table.rows.iter().enumerate() { | |
| if current_row_group_index == row.group_index { | |
| continue; | |
| } | |
| if let Some(current_group_index) = current_row_group_index { | |
| self.table.row_groups[current_group_index].track_range.end = row_index; | |
| } | |
| current_row_group_index = row.group_index; | |
| if let Some(current_group_index) = current_row_group_index { | |
| self.table.row_groups[current_group_index].track_range.start = row_index; | |
| } | |
| } | |
| if let Some(current_group_index) = current_row_group_index { | |
| self.table.row_groups[current_group_index].track_range.end = self.table.rows.len(); | |
| } |
| if let AttrValue::UInt(ref mut s, ref mut val) = attr { | ||
| if *val == 0 { | ||
| *val = 1; | ||
| *s = "1".into(); |
There was a problem hiding this comment.
If value isn't even a number, AttrValue::UInt will preserve the string even though the number will default to DEFAULT_SPAN.
So for consistency I think the string shouldn't be changed to "1" here?
mrobinson
force-pushed
the
table-column-and-colgroup
branch
from
baf3f75
to
a1da22a
Compare
|
🔨 Triggering try run (#7961737364) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#7961737364): Flaky unexpected result (13)
Stable unexpected results that are known to be intermittent (10)
Stable unexpected results (2)
|
|
|
mrobinson
force-pushed
the
table-column-and-colgroup
branch
from
a1da22a
to
f56f5fc
Compare
|
🔨 Triggering try run (#7962736101) for Linux WPT |
mrobinson
force-pushed
the
table-column-and-colgroup
branch
from
f56f5fc
to
d915b03
Compare
|
🔨 Triggering try run (#7962757948) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#7962757948): Flaky unexpected result (12)
Stable unexpected results that are known to be intermittent (14)
|
|
✨ Try run (#7962757948) succeeded. |
Loirooriol
approved these changes
Feb 19, 2024
mrobinson
force-pushed
the
table-column-and-colgroup
branch
from
d915b03
to
828765f
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
This adds support for table rows, columns, rowgroups and colgroups. There are few additions here: 1. The createion of fragments, which allows script queries and hit testing to work properly. These fragments are empty as all cells are still direct descendants of the table fragment. 2. Properly handling size information from tracks and track groups as well as frustrating rules about reordering rowgroups. 3. Painting a background seemlessly across track groups and groups. This is a thing that isn't done in legacy layout (nor WebKit)! Co-authored-by: Oriol Brufau <obrufau@igalia.com>
mrobinson
force-pushed
the
table-column-and-colgroup
branch
from
828765f
to
a6a9786
Compare
5 tasks
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.
This adds support for table rows, columns, rowgroups and colgroups.
There are few additions here:
testing to work properly. These fragments are empty as all cells are
still direct descendants of the table fragment.
well as frustrating rules about reordering rowgroups.
is a thing that isn't done in legacy layout (nor WebKit)!
This causes some tests to start failing, because it exposes our lack of
support for collapse in tables.
Co-authored-by: Oriol Brufau obrufau@igalia.com
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors