Layout floats as children of their inline ancestors #30130
Merged
Conversation
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
mrobinson
force-pushed
the
layout-float-inline
branch
from
32bd2a0 to
0de5c89
Compare
mukilan
reviewed
Aug 22, 2023
| } | ||
| } | ||
|
|
||
| /// Given the amount of whitespace trimmed from the start of the line and taking into consideration |
There was a problem hiding this comment.
The comment here says 'amount of whitespace trimmed from the start of the line' but in code, the value for whitespace_trimmed is set by the 'trim_whitespace_at_end` method. Is the comment here wrong or am I misunderstanding it?
There was a problem hiding this comment.
Oh, that's a good point. I've updated this to simply say "the amount of whitespace trimmed from the line."
mukilan
reviewed
Aug 22, 2023
| match text_align { | ||
| TextAlign::Start => line_start_position_from_indent, | ||
| TextAlign::End => { | ||
| containing_block.inline_size - real_line_length_without_indent + |
There was a problem hiding this comment.
Is the calculation here correct?
For this test html, both layout 2020 master and legacy-layout have the same output as Firefox and Chrome, but this PR doesn't match those.
There was a problem hiding this comment.
The calculation was incorrect! I have updated this in a way that I think corrects the case you mentioned and also have put a specification link and quote explaining why it's correct now.
There was a problem hiding this comment.
The text-align: end case doesn't seem to work similar to the other layouts & master for this case.
IIUC, the spec says 'text-indent' is treated as margin at the 'start' edge of the linebox and 'text-align: end' will align the contents to the 'end' edge of the box, so the margin cannot not influence the end edge unless the margin exceeds the available width.
mrobinson
force-pushed
the
layout-float-inline
branch
from
0de5c89 to
ed5f8a7
Compare
|
@bors-servo try=wpt-2020 |
|
🔨 Triggering try run (#5938918429) with platform=linux and layout=2020 |
|
Test results for linux-wpt-layout-2020 from try job (#5938918429): Flaky unexpected result (7)
Stable unexpected results that are known to be intermittent (30)
|
|
✨ Try run (#5938918429) succeeded. |
|
@mukilan Thanks for the review. I think this is ready to look at again, when you have a moment. |
|
Thanks @mrobinson for updating the PR with the spec and comments! I've found one more case where 'text-end' doesn't work as other browsers. Please let me know if that is not in scope for this PR and needs to fixed separately as part of 'text-indent' stuff. |
mrobinson
force-pushed
the
layout-float-inline
branch
from
ed5f8a7 to
1b4800e
Compare
Thanks for pointing out this issue. I believe I've fixed it and have also added two WPT tests corresponding to the issues your raised above. |
|
🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#41585) with upstreamable changes. |
mrobinson
force-pushed
the
layout-float-inline
branch
from
1b4800e to
c8fb806
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#41585). |
mrobinson
force-pushed
the
layout-float-inline
branch
from
c8fb806 to
59c187e
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#41585). |
mukilan
approved these changes
Aug 22, 2023
mrobinson
force-pushed
the
layout-float-inline
branch
from
59c187e to
ef93c51
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#41585). |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
When layout was split into two phases, floats were laid out as direct children of the inline formatting context. This meant that they were positioned properly, but not properly made children of their inline ancestors' stacking contexts. This change maintains the proper positioning of floats, but positions them relatively to their inline ancestors. The big change here is that `text-align` needs to be taken into account before actually laying out LineItems. This has the added benefit of setting inline layout for the implementation of `text-align: full`. Now all line items are laid out at the real final position and we can adjust the `start_corner` property of float `BoxFragments` when their ancestors are laid out.
mrobinson
force-pushed
the
layout-float-inline
branch
from
ef93c51 to
0c00c1a
Compare
|
It seems that Layout 2013 does not pass this test...it looks like due to line breaking issues. All pipelines are passing in upstream WPT issue. I've updated the result and will send it back to the MQ. |
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#41585). |
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.
When layout was split into two phases, floats were laid out as direct
children of the inline formatting context. This meant that they were
positioned properly, but not properly made children of their inline
ancestors' stacking contexts. This change maintains the proper
positioning of floats, but positions them relatively to their inline
ancestors.
The big change here is that
text-alignneeds to be taken into accountbefore actually laying out LineItems. This has the added benefit of
setting inline layout for the implementation of
text-align: full. Nowall line items are laid out at the real final position and we can adjust
the
start_cornerproperty of floatBoxFragmentswhen their ancestorsare laid out.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors