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

Side by Side 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:

« 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 »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 /*	1 /*

2 * Copyright (C) 2013 Google Inc. All rights reserved.	2 * Copyright (C) 2013 Google Inc. All rights reserved.

3 *	3 *

4 * Redistribution and use in source and binary forms, with or without	4 * Redistribution and use in source and binary forms, with or without

5 * modification, are permitted provided that the following conditions are	5 * modification, are permitted provided that the following conditions are

6 * met:	6 * met:

7 *	7 *

8 * * Redistributions of source code must retain the above copyright	8 * * Redistributions of source code must retain the above copyright

9 * notice, this list of conditions and the following disclaimer.	9 * notice, this list of conditions and the following disclaimer.

10 * * Redistributions in binary form must reproduce the above	10 * * Redistributions in binary form must reproduce the above

(...skipping 12 matching lines...) Expand all Loading...
23 * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT	23 * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT

24 * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,	24 * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,

25 * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY	25 * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY

26 * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT	26 * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT

27 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE	27 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE

28 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.	28 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

29 */	29 */

30	30

31 #include "core/layout/LayoutBlockFlow.h"	31 #include "core/layout/LayoutBlockFlow.h"

32	32

	33 #include <memory>

33 #include "core/editing/Editor.h"	34 #include "core/editing/Editor.h"

34 #include "core/frame/FrameView.h"	35 #include "core/frame/FrameView.h"

35 #include "core/frame/LocalFrame.h"	36 #include "core/frame/LocalFrame.h"

36 #include "core/html/HTMLDialogElement.h"	37 #include "core/html/HTMLDialogElement.h"

37 #include "core/layout/HitTestLocation.h"	38 #include "core/layout/HitTestLocation.h"

38 #include "core/layout/LayoutAnalyzer.h"	39 #include "core/layout/LayoutAnalyzer.h"

39 #include "core/layout/LayoutFlowThread.h"	40 #include "core/layout/LayoutFlowThread.h"

40 #include "core/layout/LayoutInline.h"	41 #include "core/layout/LayoutInline.h"

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

41 #include "core/layout/LayoutMultiColumnFlowThread.h"	43 #include "core/layout/LayoutMultiColumnFlowThread.h"

42 #include "core/layout/LayoutMultiColumnSpannerPlaceholder.h"	44 #include "core/layout/LayoutMultiColumnSpannerPlaceholder.h"

43 #include "core/layout/LayoutPagedFlowThread.h"	45 #include "core/layout/LayoutPagedFlowThread.h"

44 #include "core/layout/LayoutView.h"	46 #include "core/layout/LayoutView.h"

45 #include "core/layout/TextAutosizer.h"	47 #include "core/layout/TextAutosizer.h"

46 #include "core/layout/line/GlyphOverflow.h"	48 #include "core/layout/line/GlyphOverflow.h"

47 #include "core/layout/line/InlineIterator.h"	49 #include "core/layout/line/InlineIterator.h"

48 #include "core/layout/line/InlineTextBox.h"	50 #include "core/layout/line/InlineTextBox.h"

49 #include "core/layout/line/LineWidth.h"	51 #include "core/layout/line/LineWidth.h"

50 #include "core/layout/shapes/ShapeOutsideInfo.h"	52 #include "core/layout/shapes/ShapeOutsideInfo.h"

51 #include "core/paint/BlockFlowPaintInvalidator.h"	53 #include "core/paint/BlockFlowPaintInvalidator.h"

52 #include "core/paint/PaintLayer.h"	54 #include "core/paint/PaintLayer.h"

53 #include "platform/RuntimeEnabledFeatures.h"	55 #include "platform/RuntimeEnabledFeatures.h"

54 #include "wtf/PtrUtil.h"	56 #include "wtf/PtrUtil.h"

55 #include <memory>

56	57

57 namespace blink {	58 namespace blink {

58	59

59 bool LayoutBlockFlow::s_canPropagateFloatIntoSibling = false;	60 bool LayoutBlockFlow::s_canPropagateFloatIntoSibling = false;

60	61

61 struct SameSizeAsLayoutBlockFlow : public LayoutBlock {	62 struct SameSizeAsLayoutBlockFlow : public LayoutBlock {

62 LineBoxList lineBoxes;	63 LineBoxList lineBoxes;

63 void* pointers[2];	64 void* pointers[2];

64 };	65 };

65	66

(...skipping 812 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
878 if (marginInfo.atBeforeSideOfBlock() && !childIsSelfCollapsing)	879 if (marginInfo.atBeforeSideOfBlock() && !childIsSelfCollapsing)

879 marginInfo.setAtBeforeSideOfBlock(false);	880 marginInfo.setAtBeforeSideOfBlock(false);

880	881

881 // Now place the child in the correct left position	882 // Now place the child in the correct left position

882 determineLogicalLeftPositionForChild(child);	883 determineLogicalLeftPositionForChild(child);

883	884

884 LayoutSize childOffset = child.location() - oldRect.location();	885 LayoutSize childOffset = child.location() - oldRect.location();

885	886

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

887 // position.	888 // position.

888 setLogicalHeight(logicalHeight() + logicalHeightForChild(child));	889 if (isListItem())

	890 toLayoutListItem(this)->addLogicalHeightFromChild(child);
	mstensho (USE GERRIT) 2017/03/20 10:19:01 Without this special-code, the list item marker wo 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. :) cathiechentx 2017/03/20 12:56:44 Yeah, that make sense. But I'm concerned about the Show quoted text 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. mstensho (USE GERRIT) 2017/03/20 13:51:20 Looks like Firefox doesn't really put the marker n Show quoted text 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. cathiechentx 2017/03/21 14:07:08 Thanks very much for the patient explanation! This Show quoted text 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 Show quoted text 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. Show quoted text 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.
	891 else

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

889 if (mustSeparateMarginAfterForChild(child)) {	893 if (mustSeparateMarginAfterForChild(child)) {

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

891 marginInfo.clearMargin();	895 marginInfo.clearMargin();

892 }	896 }

893 // If the child has overhanging floats that intrude into following siblings	897 // If the child has overhanging floats that intrude into following siblings

894 // (or possibly out of this block), then the parent gets notified of the	898 // (or possibly out of this block), then the parent gets notified of the

895 // floats now.	899 // floats now.

896 if (childLayoutBlockFlow)	900 if (childLayoutBlockFlow)

897 addOverhangingFloats(childLayoutBlockFlow, !childNeededLayout);	901 addOverhangingFloats(childLayoutBlockFlow, !childNeededLayout);

898	902

(...skipping 3706 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
4605 return LayoutBlock::invalidatePaintIfNeeded(paintInvalidationState);	4609 return LayoutBlock::invalidatePaintIfNeeded(paintInvalidationState);

4606 }	4610 }

4607	4611

4608 void LayoutBlockFlow::invalidateDisplayItemClients(	4612 void LayoutBlockFlow::invalidateDisplayItemClients(

4609 PaintInvalidationReason invalidationReason) const {	4613 PaintInvalidationReason invalidationReason) const {

4610 BlockFlowPaintInvalidator(*this).invalidateDisplayItemClients(	4614 BlockFlowPaintInvalidator(*this).invalidateDisplayItemClients(

4611 invalidationReason);	4615 invalidationReason);

4612 }	4616 }

4613	4617

4614 } // namespace blink	4618 } // namespace blink

OLD	NEW