Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LayoutNG] Fix `vertical-align: top` and `bottom` #18956

Merged
merged 3 commits into from Sep 12, 2019
Merged

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Sep 10, 2019

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}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1792396 branch from aa031d1 to 57dd7f3 Sep 10, 2019
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}
@foolip
Copy link
Member

@foolip foolip commented Sep 11, 2019

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
Copy link
Member

@foolip foolip commented Sep 11, 2019

Oh, wait, @LukeZielinski pushed a commit that accounts for the difference. Just need to make sure the fix works then.

@LukeZielinski LukeZielinski requested a review from foolip Sep 11, 2019
@foolip
foolip approved these changes Sep 12, 2019
Copy link
Member

@foolip foolip left a comment

@foolip foolip merged commit 2ed1d14 into master Sep 12, 2019
11 checks passed
11 checks passed
update-pr-preview
Details
Azure Pipelines Build #20190911.85 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@foolip foolip deleted the chromium-export-cl-1792396 branch Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.