layout: Add initial support for row height distribution #31421
Merged
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 (#8024844911) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#8024844911): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (13)
Stable unexpected results (11)
|
|
|
|
I'm converting this one back into a draft. I think we should simply implement the LayoutNG distribution algorithm outright to avoid the code churn. |
mrobinson
force-pushed
the
table-height-determination
branch
from
0acbd7d
to
5bb19f4
Compare
|
🔨 Triggering try run (#8050682843) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#8050682843): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (11)
|
|
✨ Try run (#8050682843) succeeded. |
Loirooriol
approved these changes
Feb 28, 2024
| if let Some(percentage_resolution_size) = percentage_resolution_size { | ||
| let get_percent_block_size_deficit = |row_index: usize, track_size: Au| { | ||
| let size_needed_for_percent = percentage_resolution_size | ||
| .scale_by(self.rows[row_index].percent.0 as f32 / 100.); |
There was a problem hiding this comment.
Already a f32
Suggested change
| .scale_by(self.rows[row_index].percent.0 as f32 / 100.); | |
| .scale_by(self.rows[row_index].percent.0 / 100.); |
Comment on lines
1275
to
1279
| let non_empty_rows_all_constrained = non_empty_rows | ||
| .iter() | ||
| .filter(|index| is_unconstrained(*index)) | ||
| .next() | ||
| .is_none(); |
There was a problem hiding this comment.
Suggested change
| let non_empty_rows_all_constrained = non_empty_rows | |
| .iter() | |
| .filter(|index| is_unconstrained(*index)) | |
| .next() | |
| .is_none(); | |
| let non_empty_rows_all_constrained = !non_empty_rows.iter().any(is_unconstrained); |
| let mut rows_to_grow = &empty_rows; | ||
| let unconstrained_empty_rows: Vec<usize> = rows_to_grow | ||
| .iter() | ||
| .map(|track| *track) |
There was a problem hiding this comment.
Suggested change
| .map(|track| *track) | |
| .copied() |
| let get_percent_block_size_deficit = |row_index: usize, track_size: Au| { | ||
| let size_needed_for_percent = percentage_resolution_size | ||
| .scale_by(self.rows[row_index].percent.0 as f32 / 100.); | ||
| size_needed_for_percent - track_size.max(Au::zero()) |
There was a problem hiding this comment.
Should this be?
Suggested change
| size_needed_for_percent - track_size.max(Au::zero()) | |
| (size_needed_for_percent - track_size).max(Au::zero()) |
Or possibly leave it to the callers, since one is already checking > Au::zero() and doesn't need .max(Au::zero())
There was a problem hiding this comment.
I fixed this one. I think we do need this in case the sum of the percentages of the rows is greater than 100%.
| row_deficit.to_f32_px() / percent_block_size_deficit.to_f32_px(); | ||
| let size = percent_distributable_block_size.scale_by(ratio); | ||
| track_sizes[track_index] += size; | ||
| excess_size -= size; |
There was a problem hiding this comment.
Can this make excess_size negative?
There was a problem hiding this comment.
I don't think so. We calculate the ratio via (row_deficit / total_deficit), but this ratio is multiplied by total_deficit.min(excess_size), so we always take a ratio of excess size.
Comment on lines
1329
to
1338
| .auto_is(Length::zero) | ||
| .block |
There was a problem hiding this comment.
No need to resolve both dimensions:
Suggested change
| .auto_is(Length::zero) | |
| .block | |
| .block | |
| .auto_is(Length::zero) |
Also this is not taking min/max-height into account, but we can leave it for the follow-up.
| @@ -306,11 +399,11 @@ impl<'a> TableLayout<'a> { | |||
| for column_index in 0..self.table.size.width { | |||
| let column_measure = &mut column_measures[column_index]; | |||
| let final_intrinsic_percentage_width = column_measure | |||
| .percentage_width | |||
| .percentage | |||
| .0 | |||
| .min(100. - total_intrinsic_percentage_width); | |||
There was a problem hiding this comment.
Just realized that Percentage is actually 1-based, but we can fix them in a follow-up.
mrobinson
force-pushed
the
table-height-determination
branch
from
5bb19f4
to
a18cdab
Compare
mrobinson
commented
Feb 29, 2024
There was a problem hiding this comment.
Thanks for the review @Loirooriol. I think I've addressed all the issues you raised.
| row_deficit.to_f32_px() / percent_block_size_deficit.to_f32_px(); | ||
| let size = percent_distributable_block_size.scale_by(ratio); | ||
| track_sizes[track_index] += size; | ||
| excess_size -= size; |
There was a problem hiding this comment.
I don't think so. We calculate the ratio via (row_deficit / total_deficit), but this ratio is multiplied by total_deficit.min(excess_size), so we always take a ratio of excess size.
This change adds a version of row height distribution that follows the distribtuion algorithm used for tables in Blink's LayoutNG. This is just an intermediate step toward implementing a distribution algorithm for both rows and columns more similar to Layout NG. The CSS Table 3 specification is often wrong with regard to web compatability, which is why we have abandoned it in favor of the Layout NG algorithm for row height distribution. this work. Co-authored-by: Oriol Brufau <obrufau@igalia.com>
mrobinson
force-pushed
the
table-height-determination
branch
from
a18cdab
to
b4f605e
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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 change adds a version of row height distribution that follows the
distribution algorithm used for tables in Blink's LayoutNG. This is just
an intermediate step toward implementing a distribution algorithm for
both rows and columns more similar to Layout NG.
The CSS Table 3 specification is often wrong with regard to web
compatibility, which is why we have abandoned it in favor of the Layout
NG algorithm for row height distribution.
Co-authored-by: Oriol Brufau obrufau@igalia.com
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors