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

Issue 2847823002: Make leading OOF objects to be handled by block layout in LayoutNG (Closed)

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

Description

Make leading OOF objects to be handled by block layout in LayoutNG This patch changes LayoutNG's LayoutObject tree for DOM examples with OOF object We want to use the block layout for out of flow positioned objects when they go in front of inline blocks or if they are just standalone objects. Example 1: <div id="zero"><div id="oof"></div></div> Legacy Layout: #oof is in inline context. LayoutNG: #oof is in block context. Example 2: <div id=container><oof></oof>Hello!</div> Legacy Layout: oof is in inline context. LayoutNG: oof is in block context. Example 3: <div id=container>Hello!<oof></oof></div> Legacy Layout: oof is in inline context. LayoutNG: oof is in inline context. This also deprecates ShouldHandleByInlineContext in favor of LayoutObject::ChildrenInline() 2 new failing tests: floats-clear/floats-114.xht floats-clear/floats-040.xht Those are reftest, this patch fixes border/padding issue for OOF so they started failing because we need to fix reftest as well. BUG=712665, 635619 Review-Url: https://codereview.chromium.org/2847823002 Cr-Commit-Position: refs/heads/master@{#470179} Committed: https://chromium.googlesource.com/chromium/src/+/415a2e47b6e9f046a503388298ad2e94b24e3c7b

Patch Set 1 #

Total comments: 1

Patch Set 2 : Do not reparent OOF if we have <float> following by <span> #

Patch Set 3 : git rebase-update, fixed/investigated failed LayouTests etc. #

Patch Set 4 : fix NGBlockNodeForTest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -47 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 7 chunks +17 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp View 1 2 2 chunks +26 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc View 1 2 3 chunks +16 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_node.cc View 1 2 1 chunk +1 line, -22 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_node_test.cc View 1 2 3 2 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 49 (32 generated)
Gleb Lanbin
3 years, 7 months ago (2017-04-27 18:01:54 UTC) #4
ikilpatrick
lgtm if others are fine. +eae https://codereview.chromium.org/2847823002/diff/1/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/2847823002/diff/1/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode3006 third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:3006: // LayoutNG: oof ...
3 years, 7 months ago (2017-04-27 20:34:23 UTC) #12
kojii
Standalone case is fine with me. Could you mind to explain why we prefer to ...
3 years, 7 months ago (2017-04-28 02:18:17 UTC) #13
kojii
On 2017/04/28 at 02:18:17, kojii wrote: > Could you mind to explain why we prefer ...
3 years, 7 months ago (2017-04-28 02:25:17 UTC) #14
Gleb Lanbin
On 2017/04/28 02:25:17, kojii wrote: > On 2017/04/28 at 02:18:17, kojii wrote: > > Could ...
3 years, 7 months ago (2017-04-28 03:41:19 UTC) #15
kojii
I'm good with 1 and 3. I meant by "I'm good" comment that I'm good ...
3 years, 7 months ago (2017-04-30 17:03:51 UTC) #16
Gleb Lanbin
On 2017/04/30 17:03:51, kojii wrote: > I'm good with 1 and 3. I meant by ...
3 years, 7 months ago (2017-05-02 01:43:23 UTC) #17
kojii
On 2017/05/02 at 01:43:23, glebl wrote: > We don't create any additional LayoutBlockFlow/NGBlockNode for the ...
3 years, 7 months ago (2017-05-02 02:48:54 UTC) #18
Gleb Lanbin
On 2017/05/02 02:48:54, kojii wrote: > On 2017/05/02 at 01:43:23, glebl wrote: > > We ...
3 years, 7 months ago (2017-05-02 16:46:09 UTC) #19
Gleb Lanbin
On 2017/05/02 16:46:09, Gleb Lanbin wrote: > On 2017/05/02 02:48:54, kojii wrote: > > On ...
3 years, 7 months ago (2017-05-03 22:15:05 UTC) #20
eae
LGTM as per discussion in layout meeting today. I'm not thrilled about the extra anonymous ...
3 years, 7 months ago (2017-05-03 22:28:12 UTC) #21
kojii
lgtm, Japan is in a holiday week and that I was slow to respond, sorry ...
3 years, 7 months ago (2017-05-05 09:55:49 UTC) #26
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/2847823002/20001
3 years, 7 months ago (2017-05-05 10:50:20 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/428274)
3 years, 7 months ago (2017-05-05 10:53:12 UTC) #31
kojii
> I don't know why we introduced ShouldHandleByInlineContext instead of using ChildrenInline at the first ...
3 years, 7 months ago (2017-05-05 11:15:29 UTC) #32
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/2847823002/60001
3 years, 7 months ago (2017-05-09 00:18:54 UTC) #46
commit-bot: I haz the power
3 years, 7 months ago (2017-05-09 04:02:17 UTC) #49
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/415a2e47b6e9f046a503388298ad...

Powered by Google App Engine
This is Rietveld 408576698