|
|
Created:
3 years, 7 months ago by Gleb Lanbin Modified:
3 years, 7 months ago 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. |
DescriptionMake 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 #
Messages
Total messages: 49 (32 generated)
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
glebl@chromium.org changed reviewers: + ikilpatrick@chromium.org, kojii@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ikilpatrick@chromium.org changed reviewers: + eae@chromium.org
lgtm if others are fine. +eae https://codereview.chromium.org/2847823002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/2847823002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:3006: // LayoutNG: oof is in inline context. did we want the end children in the inline context? (also this does that?)
Standalone case is fine with me. Could you mind to explain why we prefer to handle leading OOF in block context? That doesn't look intuitive to me, and it is slightly less efficient while it is very common case, no?
On 2017/04/28 at 02:18:17, kojii wrote: > Could you mind to explain why we prefer to handle leading OOF in block context? That doesn't look intuitive to me, and it is slightly less efficient while it is very common case, no? Also it creates a situation where inline/block children are mixed, which I'd like not to happen. https://docs.google.com/a/chromium.org/document/d/1kAJLEYHl5eE6ean3NSTnJOuGbJ... What is the problem you're trying to solve by making leading OOF as block?
On 2017/04/28 02:25:17, kojii wrote: > On 2017/04/28 at 02:18:17, kojii wrote: > > Could you mind to explain why we prefer to handle leading OOF in block > context? That doesn't look intuitive to me, and it is slightly less efficient > while it is very common case, no? > > Also it creates a situation where inline/block children are mixed, which I'd > like not to happen. > https://docs.google.com/a/chromium.org/document/d/1kAJLEYHl5eE6ean3NSTnJOuGbJ... > > What is the problem you're trying to solve by making leading OOF as block? I thought you agreed with this solution. May be I misunderstood you. from https://codereview.chromium.org/2829593002/#msg25 "...If this is the only difference, and Ian is good, I'm good,..." I discussed it with Ian and he agreed with this solution by LGTMing this patch. re: What is the problem you're trying to solve by making leading OOF as block? this patch is based on your patch http://crrev.com/2829593002 but fixes broken unittests/layout tests.
I'm good with 1 and 3. I meant by "I'm good" comment that I'm good with 1, but didn't mean 2... Did you check how the LayoutObject tree looks like for 2 with this change? I guess we create additional LayoutBlockFlow and additional NGBlockNode, no? Floats at the beginning of inline is probably the most common case I think, I wish us not to create anonymous blocks. If we intentionally accept the inefficiency, you might be seeing other benefits, and I'm wondering what the benefits are.
On 2017/04/30 17:03:51, kojii wrote: > I'm good with 1 and 3. I meant by "I'm good" comment that I'm good with 1, but > didn't mean 2... > > Did you check how the LayoutObject tree looks like for 2 with this change? I > guess we create additional LayoutBlockFlow and additional NGBlockNode, no? > Floats at the beginning of inline is probably the most common case I think, I > wish us not to create anonymous blocks. > > If we intentionally accept the inefficiency, you might be seeing other benefits, > and I'm wondering what the benefits are. We don't create any additional LayoutBlockFlow/NGBlockNode for the use case N2 with this change. Use case N2: <div> <span></span> <float></float> </div> NGInputNodeTree: NGBlockNode: 'LayoutNGBlockFlow HTML' NGBlockNode: 'LayoutNGBlockFlow BODY' NGBlockNode: 'LayoutNGBlockFlow DIV' NGInlineNode NGInlineItem. Type: 'OpenTag'. LayoutObject: 'LayoutInline SPAN' NGInlineItem. Type: 'CloseTag'. LayoutObject: 'LayoutInline SPAN' NGInlineItem. Type: 'Floating'. LayoutObject: 'LayoutNGBlockFlow (floating) DIV class='float-left'' As can be seen the float is an inline node in this case and this is expected. Another example: Use case N5 <div> <float></float> <span></span> <div></div> </div> NGInputNodeTree: NGBlockNode: 'LayoutNGBlockFlow HTML' NGBlockNode: 'LayoutNGBlockFlow BODY' NGBlockNode: 'LayoutNGBlockFlow DIV' NGBlockNode: 'LayoutNGBlockFlow (floating) DIV class='float-left'' NGBlockNode: 'LayoutNGBlockFlow (anonymous)' NGInlineNode NGInlineItem. Type: 'OpenTag'. LayoutObject: 'LayoutInline SPAN' NGInlineItem. Type: 'CloseTag'. LayoutObject: 'LayoutInline SPAN' NGBlockNode: 'LayoutNGBlockFlow DIV' same here, only one anonymous wrapper for <span> element. Please find the complete report by this link https://docs.google.com/document/d/1p9z05t7yDLe2ZvKhISLRnArlyeHisC8ejsngV62Al...
On 2017/05/02 at 01:43:23, glebl wrote: > We don't create any additional LayoutBlockFlow/NGBlockNode for the use case N2 with this change. The example 2 in the comment is the use case 1 in your report doc. There's "NGBlockNode: 'LayoutNGBlockFlow (anonymous)'", right?
On 2017/05/02 02:48:54, kojii wrote: > On 2017/05/02 at 01:43:23, glebl wrote: > > We don't create any additional LayoutBlockFlow/NGBlockNode for the use case N2 > with this change. > > The example 2 in the comment is the use case 1 in your report doc. There's > "NGBlockNode: 'LayoutNGBlockFlow (anonymous)'", right? yes, for the example below we need an anonymous block to wrap the inline node. <div> <float></float> <span></span> </div> the main benefit of this patch is to keep the code path predictable. In the legacy layout a first-child float can be a block or inline node(default). That leads to the situation that in the example below the float is inline. <div1 style="margin-bottom: 200px;"> <float></float> </div1> <div2 style="height: 10px;"> </div2> In the new layout system we resolve the block's position in space before any inline node. This patch tries to make a clear separation between block/inline nodes. The current version of the block layout algorithm handles the example above correctly and positions the float at div2's offset. I don't know why we introduced ShouldHandleByInlineContext instead of using ChildrenInline at the first place. The current code works correctly with the ShouldHandleByInlineContext's version of NGNode tree. There is another way to fix inline/block algorithms without this change by teaching the inline algorithm about MarginStrut, empty divs, unpositioned floats etc. It's just a double/extra work.
On 2017/05/02 16:46:09, Gleb Lanbin wrote: > On 2017/05/02 02:48:54, kojii wrote: > > On 2017/05/02 at 01:43:23, glebl wrote: > > > We don't create any additional LayoutBlockFlow/NGBlockNode for the use case > N2 > > with this change. > > > > The example 2 in the comment is the use case 1 in your report doc. There's > > "NGBlockNode: 'LayoutNGBlockFlow (anonymous)'", right? > > yes, for the example below we need an anonymous block to wrap the inline node. > <div> > <float></float> > <span></span> > </div> > > the main benefit of this patch is to keep the code path predictable. In the > legacy layout a first-child float can be a block or inline node(default). That > leads to the situation that in the example below the float is inline. > > <div1 style="margin-bottom: 200px;"> > <float></float> > </div1> > <div2 style="height: 10px;"> > </div2> > > In the new layout system we resolve the block's position in space before any > inline node. This patch tries to make a clear separation between block/inline > nodes. The current version of the block layout algorithm handles the example > above correctly and positions the float at div2's offset. > I don't know why we introduced ShouldHandleByInlineContext instead of using > ChildrenInline at the first place. The current code works correctly with the > ShouldHandleByInlineContext's version of NGNode tree. There is another way to > fix inline/block algorithms without this change by teaching the inline algorithm > about MarginStrut, empty divs, unpositioned floats etc. It's just a double/extra > work. Koji, we discussed this issue at the Layout meeting today. Per the last update from Ian/Email we want to try to introduce our own painter for inlines that will paint directly from the fragments tree. Therefore we're fine with a temporarily introduced anonymous block to wrap inlines in the example below. <div> <float></float> <span></span> </div> I think it makes the code path be more predictable as we now handle these 2 examples in the same way, i.e. floats before inline/block remain in the block context. Ex 1. <div> <float></float> <span></span> </div> The float is a block node here. Ex 2. <div> <float></float> <div></div> </div> The float is a block node here.
LGTM as per discussion in layout meeting today. I'm not thrilled about the extra anonymous block but it seems like the best option for the time being as it simplifies both the painting and the float code.
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm, Japan is in a holiday week and that I was slow to respond, sorry about this. I wasn't intended to slow this discussion this long, sorry about that.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2847823002/#ps20001 (title: "Do not reparent OOF if we have <float> following by <span>")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
> I don't know why we introduced ShouldHandleByInlineContext instead of using ChildrenInline at the first place. It was to avoid mixed children only in NG, but your fix avoids it in a different way, so that is fine now.
Description was changed from ========== 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 ========== to ========== 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() 5 new failing tests: virtual/layout_ng/external/wpt/css/CSS2/floats/floats-placement-vertical-001a.xht virtual/layout_ng/external/wpt/css/CSS2/floats/floats-placement-vertical-001b.xht virtual/layout_ng/external/wpt/css/CSS2/floats/floats-placement-vertical-001c.xht 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 ==========
Description was changed from ========== 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() 5 new failing tests: virtual/layout_ng/external/wpt/css/CSS2/floats/floats-placement-vertical-001a.xht virtual/layout_ng/external/wpt/css/CSS2/floats/floats-placement-vertical-001b.xht virtual/layout_ng/external/wpt/css/CSS2/floats/floats-placement-vertical-001c.xht 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 ========== to ========== 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 ==========
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by glebl@chromium.org
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, kojii@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2847823002/#ps60001 (title: "fix NGBlockNodeForTest")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494288656277830, "parent_rev": "947514553066c623a85712d05c3a01bd1bcbbffc", "commit_rev": "415a2e47b6e9f046a503388298ad2e94b24e3c7b"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/415a2e47b6e9f046a503388298ad... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/415a2e47b6e9f046a503388298ad... |