Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(15)

Issue 2347773002: [LayoutNG] Fix broken layout while using CSS's writing modes. (Closed)

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

Description

[LayoutNG] Fix broken layout while using CSS with writing modes. 1) This will set a writing mode to the fragment builder so it can be used at the time when we calculate a physical fragment. 2) Change computeMargin to accept 2 more parameters: Direction and writing mode. TEST=NGBlockLayoutAlgorithmTest.LayoutBlockChildrenWithWritingMode BUG=635619 Committed: https://crrev.com/085cf0072e0fee548055cb21e34ac66a12d73321 Cr-Commit-Position: refs/heads/master@{#419309}

Patch Set 1 #

Total comments: 4

Patch Set 2 : fixed comments #

Total comments: 24

Patch Set 3 : add ToAbstractDimensions, put direction after writing_mode etc. #

Total comments: 4

Patch Set 4 : ToAbstractDimensions -> ToLogicalDimensions #

Patch Set 5 : fix a compile issue #

Patch Set 6 : synced to the head #

Unified diffs Side-by-side diffs Delta from patch set Stats (+189 lines, -49 lines) Patch
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc View 1 2 3 4 5 2 chunks +5 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc View 1 2 3 4 5 8 chunks +62 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_box.cc View 1 2 3 4 5 2 chunks +7 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h View 1 2 3 chunks +11 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc View 1 2 3 chunks +15 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_constraint_space_test.cc View 1 2 3 chunks +8 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_layout_opportunity_iterator.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_length_utils.h View 1 2 chunks +5 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc View 1 2 3 4 4 chunks +62 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_length_utils_test.cc View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_units.h View 1 2 3 4 5 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 38 (22 generated)
Gleb Lanbin
4 years, 3 months ago (2016-09-16 02:58:48 UTC) #2
cbiesinger
https://codereview.chromium.org/2347773002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2347773002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc#newcode120 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:120: EXPECT_EQ(child->Height(), kHeight); Doesn't the constant go first? https://codereview.chromium.org/2347773002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc File ...
4 years, 3 months ago (2016-09-16 03:10:02 UTC) #5
cbiesinger
Or maybe computeMargins needs to take a writing mode argument, then the implementation first computes ...
4 years, 3 months ago (2016-09-16 03:11:59 UTC) #6
Gleb Lanbin
On 2016/09/16 03:11:59, cbiesinger wrote: > Or maybe computeMargins needs to take a writing mode ...
4 years, 3 months ago (2016-09-16 06:09:19 UTC) #11
ikilpatrick
https://codereview.chromium.org/2347773002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2347773002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc#newcode88 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:88: // from the parent's one. s/lied/laid https://codereview.chromium.org/2347773002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_box.h File third_party/WebKit/Source/core/layout/ng/ng_box.h ...
4 years, 3 months ago (2016-09-16 17:02:06 UTC) #12
cbiesinger
lgtm https://codereview.chromium.org/2347773002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2347773002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#newcode56 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:56: builder_->SetWritingMode(constraint_space->WritingMode()); Do you also need to call SetDirection ...
4 years, 3 months ago (2016-09-16 17:09:09 UTC) #13
cbiesinger
lgtm https://codereview.chromium.org/2347773002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc (right): https://codereview.chromium.org/2347773002/diff/40001/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc#newcode56 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:56: builder_->SetWritingMode(constraint_space->WritingMode()); Do you also need to call SetDirection ...
4 years, 3 months ago (2016-09-16 17:09:09 UTC) #14
Gleb Lanbin
thanks, PTAL. https://codereview.chromium.org/2347773002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2347773002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc#newcode120 third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:120: EXPECT_EQ(child->Height(), kHeight); On 2016/09/16 03:10:02, cbiesinger wrote: ...
4 years, 3 months ago (2016-09-16 18:25:41 UTC) #16
cbiesinger
https://codereview.chromium.org/2347773002/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc (right): https://codereview.chromium.org/2347773002/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc#newcode21 third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:21: NGBoxStrut ToAbstractDimensions(const NGPhysicalDimensions& physical_dim, Perhaps ToLogicalDimensions? I don't think ...
4 years, 3 months ago (2016-09-16 18:36:50 UTC) #17
Gleb Lanbin
https://codereview.chromium.org/2347773002/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc (right): https://codereview.chromium.org/2347773002/diff/80001/third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc#newcode21 third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:21: NGBoxStrut ToAbstractDimensions(const NGPhysicalDimensions& physical_dim, On 2016/09/16 18:36:50, cbiesinger wrote: ...
4 years, 3 months ago (2016-09-16 18:47:42 UTC) #18
cbiesinger
lgtm
4 years, 3 months ago (2016-09-16 19:10:00 UTC) #19
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/2347773002/100001
4 years, 3 months ago (2016-09-16 20:10:46 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/201368) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
4 years, 3 months ago (2016-09-16 20:21:35 UTC) #23
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/2347773002/140001
4 years, 3 months ago (2016-09-16 22:30:57 UTC) #34
commit-bot: I haz the power
Committed patchset #6 (id:140001)
4 years, 3 months ago (2016-09-16 22:37:36 UTC) #36
commit-bot: I haz the power
4 years, 3 months ago (2016-09-16 22:39:37 UTC) #38
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/085cf0072e0fee548055cb21e34ac66a12d73321
Cr-Commit-Position: refs/heads/master@{#419309}

Powered by Google App Engine
This is Rietveld 408576698