Handle floats in BlockContainer::inline_content_sizes #29887
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
|
@bors-servo try=wpt-2020 |
bors-servo
added a commit
that referenced
this pull request
Jun 17, 2023
Handle floats in BlockContainer::inline_content_sizes Typically, block-level contents are stacked vertically, so this was just taking the maximum size among all contents. However, floats can be stacked horizontally, so we need to sum their sizes. <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #29874 <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
Test results for linux-wpt-layout-2020 from try job (#5295492779): Stable unexpected results that are known to be intermittent (1)
|
|
☀️ Test successful - checks-github |
|
The tests changes are kinda by chance since |
mrobinson
approved these changes
Jun 19, 2023
There was a problem hiding this comment.
This is a really elegant solution. I only have a few small nits, but otherwise I think it looks good.
There's enough going on here that this should probably move to a free function like calculate_inline_content_size_for_block_level_boxes which gives a short rustdoc description of the approach taken along the lines of:
Find the maximum inline size of all the block level boxes and the summation of floated boxes, respecting the clear property of their siblings.
Loirooriol
force-pushed
the
float-inline-size-complete
branch
2 times, most recently
from
53b871a
to
9d11f6f
Compare
|
@bors-servo try=wpt-2020 |
bors-servo
added a commit
that referenced
this pull request
Jun 19, 2023
Handle floats in BlockContainer::inline_content_sizes Typically, block-level contents are stacked vertically, so this was just taking the maximum size among all contents. However, floats can be stacked horizontally, so we need to sum their sizes. <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #29874 <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
💔 Test failed - checks-github |
Typically, block-level contents are stacked vertically, so this was just taking the maximum size among all contents. However, floats can be stacked horizontally, so we need to sum their sizes.
Loirooriol
force-pushed
the
float-inline-size-complete
branch
from
9d11f6f
to
4ec6dd1
Compare
|
@bors-servo try=wpt-2020 |
bors-servo
added a commit
that referenced
this pull request
Jun 19, 2023
Handle floats in BlockContainer::inline_content_sizes Typically, block-level contents are stacked vertically, so this was just taking the maximum size among all contents. However, floats can be stacked horizontally, so we need to sum their sizes. <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #29874 <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
☀️ Test successful - checks-github |
|
@bors-servo r=mrobinson |
|
📌 Commit 4ec6dd1 has been approved by |
|
Test results for linux-wpt-layout-2020 from try job (#5313368453): Flaky unexpected result (2)
|
|
Test results for linux-wpt-layout-2013 from try job (#5313368453): Flaky unexpected result (15)
Stable unexpected results that are known to be intermittent (18)
|
|
☀️ Test successful - checks-github |
3 tasks
Loirooriol
added a commit
to Loirooriol/servo
that referenced
this pull request
Jul 19, 2023
calculate_inline_content_size_for_block_level_boxes was relying on inline_content_sizes to get the size of each block-level box child. For absolutely positioned boxes, this was 0x0. That was no-op before servo#29887, but then it prevented adding the sizes of a sequence of floats. Abspos should just be ignored instead of treated as 0x0. This patch removes inline_content_sizes, moves the logic into calculate_inline_content_size_for_block_level_boxes (the only caller), and handles abspos correctly.
5 tasks
bors-servo
added a commit
that referenced
this pull request
Jul 19, 2023
… r=<try> Totally ignore abspos children for intrinsic sizing calculate_inline_content_size_for_block_level_boxes was relying on inline_content_sizes to get the size of each block-level box child. For absolutely positioned boxes, this was 0x0. That was no-op before #29887, but then it prevented adding the sizes of a sequence of floats. Abspos should just be ignored instead of treated as 0x0. This patch removes inline_content_sizes, moves the logic into calculate_inline_content_size_for_block_level_boxes (the only caller), and handles abspos correctly. <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #30009 <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 19, 2023
calculate_inline_content_size_for_block_level_boxes was relying on inline_content_sizes to get the size of each block-level box child. For absolutely positioned boxes, this was 0x0. That was no-op before #29887, but then it prevented adding the sizes of a sequence of floats. Abspos should just be ignored instead of treated as 0x0. This patch removes inline_content_sizes, moves the logic into calculate_inline_content_size_for_block_level_boxes (the only caller), and handles abspos correctly.
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.
Typically, block-level contents are stacked vertically, so this was just taking the maximum size among all contents. However, floats can be stacked horizontally, so we need to sum their sizes.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors