third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp - Issue 2759883002: Get markers back while list items have overflow:hidden children

Unified Diff: third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

Issue 2759883002: Get markers back while list items have overflow:hidden children (Closed)

Patch Set: add layout test case Created 3 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« third_party/WebKit/LayoutTests/fast/lists/list-marker-before-overflow-hidden.htm ('K') | « third_party/WebKit/LayoutTests/fast/lists/list-marker-before-overflow-hidden.htm ('k') | third_party/WebKit/Source/core/layout/LayoutListItem.h » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

diff --git a/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp b/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

index 5bc764405211b05a2e686e5006311afe10ca48c3..e95f037153fe4d5ab961f7e2ea9809ce9bbf8c1b 100644

--- a/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

+++ b/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp

@@ -30,6 +30,7 @@

#include "core/layout/LayoutBlockFlow.h"

+#include <memory>

#include "core/editing/Editor.h"

#include "core/frame/FrameView.h"

#include "core/frame/LocalFrame.h"

@@ -38,6 +39,7 @@

#include "core/layout/LayoutAnalyzer.h"

#include "core/layout/LayoutFlowThread.h"

#include "core/layout/LayoutInline.h"

+#include "core/layout/LayoutListItem.h"

#include "core/layout/LayoutMultiColumnFlowThread.h"

#include "core/layout/LayoutMultiColumnSpannerPlaceholder.h"

#include "core/layout/LayoutPagedFlowThread.h"

@@ -52,7 +54,6 @@

#include "core/paint/PaintLayer.h"

#include "platform/RuntimeEnabledFeatures.h"

#include "wtf/PtrUtil.h"

-#include <memory>

namespace blink {

@@ -885,7 +886,10 @@ void LayoutBlockFlow::layoutBlockChild(LayoutBox& child,

// Update our height now that the child has been placed in the correct

// position.

- setLogicalHeight(logicalHeight() + logicalHeightForChild(child));

+ if (isListItem())

+ toLayoutListItem(this)->addLogicalHeightFromChild(child);

mstensho (USE GERRIT) 2017/03/20 10:19:01 Without this special-code, the list item marker wo

cathiechentx 2017/03/20 12:56:44 Yeah, that make sense. But I'm concerned about the

mstensho (USE GERRIT) 2017/03/20 13:51:20 Looks like Firefox doesn't really put the marker n

Looks like Firefox doesn't really put the marker next to the line box - it just fakes it somehow: <!DOCTYPE html> <ul> <li> <div style="position:relative; top:100px;"> ←Marker to the left here? </div> </li> </ul> In Firefox, the list item marker stays put, while the DIV and the line of text are moved down by 100px. Edge and Blink render it identically, with the marker next to the DIV. The new code inside LayoutListItem (invoked from here) tries to make it appear as if the marker is part of the line inside the DIV by kind of taking the marker out of flow. It falls apart as soon as one does something like this, though: <!DOCTYPE html> <ul> <li> <div style="overflow:hidden; margin-top:100px;"> ←Is the marker still there? </div> </li> </ul> Or this: <!DOCTYPE html> <ul> <li> <div style="overflow:hidden;"> ←Marker baseline alignment <span style="font-size:5em;">FAIL</span> </div> </li> </ul> Here the marker aligns itself with an imaginary line inside the hotpink block: <!DOCTYPE html> <ul> <li> <div style="overflow:hidden; height:100px; background:hotpink;"></div> <div> ←Here's the first line. Should the marker be here? </div> </li> </ul> Therefore I feel that we don't want that code, since omitting it will make the behavior more deterministic than without it, with less code.

cathiechentx 2017/03/21 14:07:08 Thanks very much for the patient explanation! This

On 2017/03/20 13:51:20, mstensho wrote: > On 2017/03/20 12:56:44, cathiechentx wrote: > > On 2017/03/20 10:19:01, mstensho wrote: > > > Without this special-code, the list item marker would occupy a line on its > > own, > > > and here's how you attempt to avoid it. But I'm not sure if you really have > to > > > do that. Edge (but not Firefox) renders your test just like that, i.e. with > a > > > marker on a separate line, followed by the overflow:hidden block. > > > > > > So I suggest you do that too (i.e just back out this change), unless there's > a > > > spec that requires us to behave differently. If we can keep it simple, we > > should > > > do that. :) > > > > Yeah, that make sense. But I'm concerned about the difference between > > overflow:hidden and overflow:visible might confuse people. With > overflow:hidden > > child we change it from no list marker to occupied one-line list marker. It > > doesn't seem to have improved. > > Looks like Firefox doesn't really put the marker next to the line box - it just > fakes it somehow: > <!DOCTYPE html> > <ul> > <li> > <div style="position:relative; top:100px;"> > ←Marker to the left here? > </div> > </li> > </ul> > > In Firefox, the list item marker stays put, while the DIV and the line of text > are moved down by 100px. Edge and Blink render it identically, with the marker > next to the DIV. > > The new code inside LayoutListItem (invoked from here) tries to make it appear > as if the marker is part of the line inside the DIV by kind of taking the marker > out of flow. It falls apart as soon as one does something like this, though: > <!DOCTYPE html> > <ul> > <li> > <div style="overflow:hidden; margin-top:100px;"> > ←Is the marker still there? > </div> > </li> > </ul> > > Or this: > <!DOCTYPE html> > <ul> > <li> > <div style="overflow:hidden;"> > ←Marker baseline alignment > <span style="font-size:5em;">FAIL</span> > </div> > </li> > </ul> > > Here the marker aligns itself with an imaginary line inside the hotpink block: > <!DOCTYPE html> > <ul> > <li> > <div style="overflow:hidden; height:100px; background:hotpink;"></div> > <div> > ←Here's the first line. Should the marker be here? > </div> > </li> > </ul> > > Therefore I feel that we don't want that code, since omitting it will make the > behavior more deterministic than without it, with less code.

Thanks very much for the patient explanation! This is much more complicate than I imagine. I'm trying to figure it out in this way: 1. Directly add marker into list item 2. Skip marker while list item layout children 3. In positionListMarker() find the first leaf child and calculate its offset from list item. 4. Move marker by this offset This approach is not simple at all and may not cover other issues. List item is very steady now, maybe I shouldn't make this change.

mstensho (USE GERRIT) 2017/03/22 22:28:55 Yes, I think you should omit the addLogicalHeightF

On 2017/03/21 14:07:08, cathiechentx wrote: > On 2017/03/20 13:51:20, mstensho wrote: > > On 2017/03/20 12:56:44, cathiechentx wrote: > > > On 2017/03/20 10:19:01, mstensho wrote: > > > > Without this special-code, the list item marker would occupy a line on its > > > own, > > > > and here's how you attempt to avoid it. But I'm not sure if you really > have > > to > > > > do that. Edge (but not Firefox) renders your test just like that, i.e. > with > > a > > > > marker on a separate line, followed by the overflow:hidden block. > > > > > > > > So I suggest you do that too (i.e just back out this change), unless > there's > > a > > > > spec that requires us to behave differently. If we can keep it simple, we > > > should > > > > do that. :) > > > > > > Yeah, that make sense. But I'm concerned about the difference between > > > overflow:hidden and overflow:visible might confuse people. With > > overflow:hidden > > > child we change it from no list marker to occupied one-line list marker. It > > > doesn't seem to have improved. > > > > Looks like Firefox doesn't really put the marker next to the line box - it > just > > fakes it somehow: > > <!DOCTYPE html> > > <ul> > > <li> > > <div style="position:relative; top:100px;"> > > ←Marker to the left here? > > </div> > > </li> > > </ul> > > > > In Firefox, the list item marker stays put, while the DIV and the line of text > > are moved down by 100px. Edge and Blink render it identically, with the marker > > next to the DIV. > > > > The new code inside LayoutListItem (invoked from here) tries to make it appear > > as if the marker is part of the line inside the DIV by kind of taking the > marker > > out of flow. It falls apart as soon as one does something like this, though: > > <!DOCTYPE html> > > <ul> > > <li> > > <div style="overflow:hidden; margin-top:100px;"> > > ←Is the marker still there? > > </div> > > </li> > > </ul> > > > > Or this: > > <!DOCTYPE html> > > <ul> > > <li> > > <div style="overflow:hidden;"> > > ←Marker baseline alignment > > <span style="font-size:5em;">FAIL</span> > > </div> > > </li> > > </ul> > > > > Here the marker aligns itself with an imaginary line inside the hotpink block: > > <!DOCTYPE html> > > <ul> > > <li> > > <div style="overflow:hidden; height:100px; background:hotpink;"></div> > > <div> > > ←Here's the first line. Should the marker be here? > > </div> > > </li> > > </ul> > > > > Therefore I feel that we don't want that code, since omitting it will make the > > behavior more deterministic than without it, with less code. > > Thanks very much for the patient explanation! This is much more complicate than > I imagine. I'm trying to figure it out in this way: > > 1. Directly add marker into list item > 2. Skip marker while list item layout children > 3. In positionListMarker() find the first leaf child and calculate its offset > from list item. > 4. Move marker by this offset > > This approach is not simple at all and may not cover other issues. List item is > very steady now, maybe I shouldn't make this change.

Yes, I think you should omit the addLogicalHeightFromChild() part of your change.

cathiechentx 2017/03/23 10:13:27 OK, that will be done.

On 2017/03/22 22:28:55, mstensho wrote: > On 2017/03/21 14:07:08, cathiechentx wrote: > > On 2017/03/20 13:51:20, mstensho wrote: > > > On 2017/03/20 12:56:44, cathiechentx wrote: > > > > On 2017/03/20 10:19:01, mstensho wrote: > > > > > Without this special-code, the list item marker would occupy a line on > its > > > > own, > > > > > and here's how you attempt to avoid it. But I'm not sure if you really > > have > > > to > > > > > do that. Edge (but not Firefox) renders your test just like that, i.e. > > with > > > a > > > > > marker on a separate line, followed by the overflow:hidden block. > > > > > > > > > > So I suggest you do that too (i.e just back out this change), unless > > there's > > > a > > > > > spec that requires us to behave differently. If we can keep it simple, > we > > > > should > > > > > do that. :) > > > > > > > > Yeah, that make sense. But I'm concerned about the difference between > > > > overflow:hidden and overflow:visible might confuse people. With > > > overflow:hidden > > > > child we change it from no list marker to occupied one-line list marker. > It > > > > doesn't seem to have improved. > > > > > > Looks like Firefox doesn't really put the marker next to the line box - it > > just > > > fakes it somehow: > > > <!DOCTYPE html> > > > <ul> > > > <li> > > > <div style="position:relative; top:100px;"> > > > ←Marker to the left here? > > > </div> > > > </li> > > > </ul> > > > > > > In Firefox, the list item marker stays put, while the DIV and the line of > text > > > are moved down by 100px. Edge and Blink render it identically, with the > marker > > > next to the DIV. > > > > > > The new code inside LayoutListItem (invoked from here) tries to make it > appear > > > as if the marker is part of the line inside the DIV by kind of taking the > > marker > > > out of flow. It falls apart as soon as one does something like this, though: > > > <!DOCTYPE html> > > > <ul> > > > <li> > > > <div style="overflow:hidden; margin-top:100px;"> > > > ←Is the marker still there? > > > </div> > > > </li> > > > </ul> > > > > > > Or this: > > > <!DOCTYPE html> > > > <ul> > > > <li> > > > <div style="overflow:hidden;"> > > > ←Marker baseline alignment > > > <span style="font-size:5em;">FAIL</span> > > > </div> > > > </li> > > > </ul> > > > > > > Here the marker aligns itself with an imaginary line inside the hotpink > block: > > > <!DOCTYPE html> > > > <ul> > > > <li> > > > <div style="overflow:hidden; height:100px; > background:hotpink;"></div> > > > <div> > > > ←Here's the first line. Should the marker be here? > > > </div> > > > </li> > > > </ul> > > > > > > Therefore I feel that we don't want that code, since omitting it will make > the > > > behavior more deterministic than without it, with less code. > > > > Thanks very much for the patient explanation! This is much more complicate > than > > I imagine. I'm trying to figure it out in this way: > > > > 1. Directly add marker into list item > > 2. Skip marker while list item layout children > > 3. In positionListMarker() find the first leaf child and calculate its offset > > from list item. > > 4. Move marker by this offset > > > > This approach is not simple at all and may not cover other issues. List item > is > > very steady now, maybe I shouldn't make this change. > > Yes, I think you should omit the addLogicalHeightFromChild() part of your > change.

OK, that will be done.

+ else

+ setLogicalHeight(logicalHeight() + logicalHeightForChild(child));

if (mustSeparateMarginAfterForChild(child)) {

setLogicalHeight(logicalHeight() + marginAfterForChild(child));

marginInfo.clearMargin();