Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(53)

Issue 2803433002: [LayoutNG] Initial support for the 'vertical-align' property (Closed)

Created:
2 years, 5 months ago by kojii
Modified:
2 years, 5 months ago
Reviewers:
ikilpatrick, eae
CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[LayoutNG] Initial support for the 'vertical-align' property This patch supports the 'vertical-align' property. Positioning based on 'vertial-align' requires, depends on values: a. Per-element positioning to support nested cases. b. The layout size of the element. c. The layout size of the parent element. d. The layout size of the line box. This patch adds NGInlineBoxState to keep track of the current inline box. This is also used to cache information common to a box, similar to what VerticalPositionCache does in LayoutObject. Another struct, NGPendingPositions, keeps track of descendants that need to be positioned when ancestor layout is done. NGInlineBoxState is expected to be used for other prorperties, while NGPendingPositions may be only for 'vertical-align'. This patch passes several tests in CSS2/linebox, but their expected files use yet to be supported properties such as inline margins or relative positions that no tests can be marked as pass in TestExpectations yet. BUG=636993 Review-Url: https://codereview.chromium.org/2803433002 Cr-Commit-Position: refs/heads/master@{#465409} Committed: https://chromium.googlesource.com/chromium/src/+/2333a26198a16f94991afa411f1066c9d4687423

Patch Set 1 #

Patch Set 2 : WIP #

Patch Set 3 : Fix tests #

Patch Set 4 : WIP #

Patch Set 5 : WIP #

Patch Set 6 : WIP #

Patch Set 7 : #

Patch Set 8 : WIP #

Patch Set 9 : WIP #

Patch Set 10 : Fix tests #

Patch Set 11 : #

Patch Set 12 : Move NGInlineBoxState to its own file #

Total comments: 1

Patch Set 13 : Move all On*() to NGInlineLayoutStateStack #

Unified diffs Side-by-side diffs Delta from patch set Stats (+343 lines, -39 lines) Patch
M third_party/WebKit/Source/core/layout/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/layout/ng/inline/ng_inline_box_state.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +87 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/layout/ng/inline/ng_inline_box_state.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +173 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +50 lines, -28 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_line_box_fragment_builder.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_line_box_fragment_builder.cc View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_line_height_metrics.h View 1 2 3 4 5 6 7 2 chunks +7 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_line_height_metrics.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_text_fragment_builder.h View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_text_fragment_builder.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -7 lines 0 comments Download

Messages

Total messages: 43 (33 generated)
kojii
PTAL. CSSWG tests are back online and this is ready for review. I ended up ...
2 years, 5 months ago (2017-04-17 03:15:24 UTC) #14
kojii
Split NGInlineBoxState to a separate file. Now I'm not sure which class should ApplyBaselineShift be, ...
2 years, 5 months ago (2017-04-17 14:03:56 UTC) #19
ikilpatrick
https://codereview.chromium.org/2803433002/diff/240001/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.h File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.h (right): https://codereview.chromium.org/2803433002/diff/240001/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.h#newcode141 third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.h:141: NGInlineBoxState* OnBeginPlaceItems(); Oh, sorry, does it make sense to ...
2 years, 5 months ago (2017-04-18 01:04:37 UTC) #20
eae
This is awesome, LGTM pending Ians comments.
2 years, 5 months ago (2017-04-18 02:08:59 UTC) #21
kojii
Thank you for having look, both. On 2017/04/18 at 01:04:37, ikilpatrick wrote: > Oh, sorry, ...
2 years, 5 months ago (2017-04-18 02:38:56 UTC) #22
ikilpatrick
On 2017/04/18 02:38:56, kojii wrote: > Thank you for having look, both. > > On ...
2 years, 5 months ago (2017-04-18 02:54:21 UTC) #23
kojii
Done, PTAL. # no sorry please ;)
2 years, 5 months ago (2017-04-18 14:29:58 UTC) #36
ikilpatrick
lgtm
2 years, 5 months ago (2017-04-18 20:55:02 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2803433002/300001
2 years, 5 months ago (2017-04-18 22:48:44 UTC) #40
commit-bot: I haz the power
2 years, 5 months ago (2017-04-18 22:55:39 UTC) #43
Message was sent while issue was closed.
Committed patchset #13 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/2333a26198a16f94991afa411f10...

Powered by Google App Engine
This is Rietveld 408576698