[LayoutNG] Fix vertical-align: top and bottom
#18956
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
wpt-pr-bot
approved these changes
Sep 10, 2019
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1792396
branch
from
aa031d1
to
57dd7f3
Compare
This patch fixes `vertical-align: top` and `bottom` when they are applied to a box that has children with `text-top` or `text-bottom`. In LayoutNG, `vertical-align` is handled in 3 different timings. 1. `top` and `bottom` are added to the pending list of the line box, or to the nearest ancestor box that has `top` or `bottom`. 2. `text-bop` and `text-bottom` are added to the pending list of the parent box. 3. Other values are computed when the box is closed. When a box has both 1 and 2, this patch changes to compute 1 (`top` and `bottom`) after 2. Before this patch, both 1 and 2 are computed at the same time for each box. 3 test cases out of 20 in this test fails without this patch. Bug: 1001664 Change-Id: I0e7746447f5e401837c2c28e308171f0987eba35 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1792396 Reviewed-by: Emil A Eklund <eae@chromium.org> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Commit-Queue: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#695194}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1792396
branch
from
57dd7f3
to
697c4b8
Compare
|
I came here to check on the Taskcluster failure, which is because of the test being flaky in Chrome Dev. But it looks like something has also gone quite wrong here, the PR doesn't match https://chromium.googlesource.com/chromium/src/+/cf88186a260efa4875553eb99f79bbbff8d3130a. I'll comment on the difference. @Hexcles can you take a look at how this happened? |
foolip
requested changes
Sep 11, 2019
|
Oh, wait, @LukeZielinski pushed a commit that accounts for the difference. Just need to make sure the fix works then. |
foolip
approved these changes
Sep 12, 2019
There was a problem hiding this comment.
Changes make sense, and https://wpt.fyi/results/css/CSS2/linebox/vertical-align-top-bottom-001.html?label=pr_head&max-count=1&pr=18956 results look good.
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 patch fixes
vertical-align: topandbottomwhenthey are applied to a box that has children with
text-topor
text-bottom.In LayoutNG,
vertical-alignis handled in 3 differenttimings.
topandbottomare added to the pending list of theline box, or to the nearest ancestor box that has
toporbottom.text-bopandtext-bottomare added to the pending listof the parent box.
When a box has both 1 and 2, this patch changes to compute 1
(
topandbottom) after 2. Before this patch, both 1 and 2are computed at the same time for each box. 3 test cases out
of 20 in this test fails without this patch.
Bug: 1001664
Change-Id: I0e7746447f5e401837c2c28e308171f0987eba35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1792396
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695194}