2014323 - 0-height frame is generated on testing/web-platform/tests/css/css-scroll-snap/multicol-001.html

Status: RESOLVED FIXED

(Core :: Layout: Columns, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

#target element in multicol-001.html is split into 3 frames;

  Block(div id=target)(3)@7f48ba7d81a8 parent=7f48ba7d8720 next-in-flow=7f48ba7d87e8 (x=0, y=100, w=  100, h=0) [content=7f48b  a404670][cs=7f48ba7e77a8] <  
  Block(div id=target)(3)@7f48ba7d87e8 parent=7f48ba7d8900 prev-in-flow=7f48ba7d81a8 next-in-flow=7f  48ba7d89c8 (x=0, y=0, w=  100, h=100) [content=7f48ba404670][cs=7f48ba7e77a8] <  
  Block(div id=target)(3)@7f48ba7d89c8 parent=7f48ba7d8ae0 next=7f48ba7d8270 prev-in-flow=7f48ba7d87  e8 (x=0, y=0, w=100, h=2  0) [content=7f48ba404670][cs=7f48ba7e77a8] <

I was told by dholbert the first frame x=0, y=100, w=100, h=0), i.e. generating zero height frame is a multicol/fragmentation bug.

This is the root cause of multicol-001.html failure (bug 2014322).

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Looks like multicol-002.html has the same issue;

 Block(div id=target)(3)@77e85a1e15d8 parent=77e85a1e1c40 next-in-flow=77e85a1e1d08 (  x=0, y=100, w=100, h=0) [content=77e85  9004820][cs=77e85a1e8be8] <
 Block(div id=target)(3)@77e85a1e1d08 parent=77e85a1e1e20 prev-in-flow=77e85a1e15d8 n  ext-in-flow=77e85a1e1ee8 (x=0, y=0, w=  100, h=100) [content=77e859004820][cs=77e85a1e8be8] <

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a testcase, reduced from multicol-001.html, which has a red outline to let you visualize the 0-height frame.

EXPECTED RESULTS:
Only the right column should have a red outline.

ACTUAL RESULTS:
There's also a red outline around a 0-height area at the bottom of the left column.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

The zero height #target doesn't look right. nsBlockFrame::ReflowDirtyLines() is the function I'd investigate first, to see how we behave when the block frame doesn't have enough available block-size for lines.

Hiro, is this the only bug blocking css/css-scroll-snap/multicol-[001|002].html? If so, would you like me to investigate this soon?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Those ones that I've found so far, but they will be a part of interop-2026. So I'd like you to investigate in 10 months or so if it's the case:)

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Thanks for letting me know. NI myself as a reminder.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thank you! I've told Frank that if you are busy on other tasks, I am happy to jump in.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I had poked this bug for two days, what I've learned so far is before reflowing we add line boxes in the first column for each children which might be fragmented. So for example in the case of multicol-001.html 3 line boxes exist in the block frame which is corresponding to the first column, and then while reflowing the second kid #target element, the second line is laid out in the first column as 0-height frame, and generates a continuation frame, pushes the frame (and it's line box) to the next column.

So what I am guessing now is, we can't avoid the initial line box generation in the first column, rather we can push the line itself to the second column, we have similar code here and here.

FWIW, note that I've locally tweaked the second similar code part, it works for this specific bug, but it breaks a bunch of multi-col wpts.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Ting-Yu told me that tweaking this part is a wrong approach and he gave kindly me a patch what he thought. And he also told that with his patch class-c-breakpoint-after-float-001.html fails.

So I've poked the class-c-breakpoint-after-float-001.html, and I realized there are other test failures such as out-of-flow-in-multicolumn-029.html. Then I realized a common pattern in those failures is a placeholder whose height is 0. Now I believe placeholders for clearance or out-of-flow items need 0-height frame there.

One more important note; With Ting-Yu's patch some grid reftests such as grid-fragmentation-006.html also fail.

I've poked the grid reftest too, what I've found so far are;

1. grid-fragmentation-006.html is not affected by the patch
2. rather grid-fragmentation-006-ref.html is affected by the patch
3. the result of grid-fragmentation-006-ref.html with the patch looks closer to chrome's result

You can see the these test failures on this try run.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Also note that fixing this bug makes offset-top-block-in-inline.html, since this hotpink element is incorrectly laid out as 0-height frame in the first column so that the Element.offSetTop is calculated based on the wrong Y offset, just like y=100 in the dump list in comment 0.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2014323 - Avoid zero-height fragments by rejecting incomplete zero-height blocks in PlaceBlock. r?#layout-reviewers

When a block is reflowed with zero available block-size in a fragmentainer,
it produces a zero-height fragment with Incomplete status. PlaceBlock now
rejects such fragments so the block is pushed to the next fragmentainer
instead of leaving a useless zero-height fragment.

Genuinely zero-height blocks are unaffected: they have Complete status, so
the new check does not trigger.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

I uploaded the Ting-Yu's patch. Oddly phabricator didn't respect the author name in the patch. :/

Anyways, I think remaining tasks are

1. Added a new check whether it's a placeholder or not and if it is, do not reject 0-height frame
2. Clarify whether the reason of the grid test failures is an issue in our grid implementation or it's actually an issue in the patch

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case demonstrating the difference on grid-fragmentation-006-ref.html introduced by D283622

Attaching file is a simplified version of grid-fragmentation-006-ref.html to see the difference introduced by the path.

If you open the file on the current Firefox you will see grayed space at the the top of the second column, and if you click "add/remove border-top-style: 0px" button, you will not see the grayed space in the second column.

The patch shows the grayed space regardless of the border style, that's actually the difference happens on grid-fragmentation-006-ref.html.

I believe the patched result is correct, as you can see there's another button "add/remove columns:3", if you click the button you will always see the space regardless of the border style.

Also note that It matches Chrome's result.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: A simplified version of grid-fragmentation-006.html

So now there's another question; is the result of grid-fragmentation-006.html correct?

Attaching file is a simplified version of grid-fragmentation-006.html. If you open the file on the current nightly or even on builds with D283622, you will see 3 columns, and the first column is entirely blank.

If you open the file on Chrome you will only 2 columns.

So now I am pretty sure that this test case is another variant of this bug, 0-height frame generation issue; In fact there's 0-height GridContainer in the first column;

 GridContainer(div)(1)@73e70a1cc328 parent=73e70a1cc260 next-in-flow=73e70a1cc610 (x=0, y=0, w=32  5.333, h=0) [content=73e70a0045e0][cs=73e70a188018] <

The reason why 0-height GridContainer remains there is that D283622 does just reject 0-height block frames, it doesn't help 0-height GridContainer. So if we fixed the 0-height GridContainer case altogether, then I expect those grid tests will not fail.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)

The reason why 0-height GridContainer remains there is that D283622 does just reject 0-height block frames, it doesn't help 0-height GridContainer. So if we fixed the 0-height GridContainer case altogether, then I expect those grid tests will not fail.

Well, we also need to revise each reference cases since in the reference case we forcibly create 3 columns by adding a some amount of height element.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I debugged the case in comment 13. The GridContainer actually runs through the code where D283622 tweaked. But in the GridContainer frame case, this aForceFit flag is true so that it doesn't run into the if branch.

And in my naiive thinking about the aForceFit is that it looks correct to me that the flag is true for the GridContainer for the first column.

A remaining question is why the GridContainer size ends up being 0-height.

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Hiro, thank you for looking into this bug. I'm clearing my needinfo.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2014323 - Refine the zero-height fragment rejection to avoid overkill cases. r?TYLin

The previous check rejected any zero-height incomplete fragment
unconditionally, which was too aggressive and broke two cases:

1. A block that is a containing block for absolutely-positioned children must
   keep its zero-height fragment as the coordinate origin for those OOF boxes.
   This is now guarded by HasAbsolutelyPositionedChildren().
   (e.g. out-of-flow-in-multicolumn-029.html)

2. class-c-breakpoint-after-float-001.html: A block with a clear:both child
   inside a multicol column can end up with the child pushed by clearance to
   exactly the column boundary, leaving it 0px of available space. The child
   produces a zero-height incomplete fragment, but its parent still has a
   non-zero-height fragment covering the clearance gap. Rejecting the child's
   fragment caused the parent's fragment to collapse, losing the class C break
   opportunity and the green background over the float area.

   More generally, any gap before the child in the parent's fragment (from
   clearance, a non-collapsing margin, float avoidance, or preceding siblings)
   means the parent fragment is non-zero regardless of this child. The child's
   zero-height fragment is only truly useless when it is the very first content
   in the fragmentainer with nothing before it.

The refined condition therefore only rejects when fragmentBStart equals
mSpace.BStart(), i.e. the child lands at the very beginning of the available
space. The variable fragmentBStart is introduced to name this quantity
clearly and to avoid recomputing mBCoord - backupContainingBlockAdvance.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

I think D292681 has fixed all test failures.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/15967bda110a
https://hg.mozilla.org/integration/autoland/rev/1363df5dce02
Avoid zero-height fragments by rejecting incomplete zero-height blocks in PlaceBlock. r=layout-reviewers,dholbert
https://github.com/mozilla-firefox/firefox/commit/be83cf7346ad
https://hg.mozilla.org/integration/autoland/rev/d9cf25919b9b
Refine the zero-height fragment rejection to avoid overkill cases. r=TYLin,layout-reviewers
https://github.com/mozilla-firefox/firefox/commit/e53f6426b2f8
https://hg.mozilla.org/integration/autoland/rev/b1d0a38c7d02
apply code formatting via Lando

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/dd9e9acab7d4
https://hg.mozilla.org/integration/autoland/rev/7c01d5760ef6
Revert "Bug 2014323: apply code formatting via Lando" for causing wr failures @multi-line-row-flex-fragmentation-093.html.

Comment 21

[agoloman]

Backed out for causing wr failures @multi-line-row-flex-fragmentation-093.html.

• Backout link
• Push with failures
• Failure Log

Comment 22

[Hiroyuki Ikezoe (:hiro)]

a link to reftests-analyzer on a failure.

For some reasons orange/yellow/purple boxes' color are slightly different, something like:

Image 1: 	#c080c0 	rgb(192,128,192)
Image 2: 	#bf7fbf 	rgb(191,127,191)

I've confirmed that D283622 changed the color. I am going to add fuzzy annotation here in this bug will file a follow-up bug.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/db6e7668fcb8
https://hg.mozilla.org/integration/autoland/rev/b55428c5d7f6
Avoid zero-height fragments by rejecting incomplete zero-height blocks in PlaceBlock. r=layout-reviewers,dholbert
https://github.com/mozilla-firefox/firefox/commit/61d464bdacf1
https://hg.mozilla.org/integration/autoland/rev/f2f4591e81aa
Refine the zero-height fragment rejection to avoid overkill cases. r=TYLin,layout-reviewers

Comment 24

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/b55428c5d7f6
https://hg.mozilla.org/mozilla-central/rev/f2f4591e81aa

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/59485 for changes under testing/web-platform/tests

Comment 26

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/876ef6b2ce5f
https://hg.mozilla.org/integration/autoland/rev/5d4479056614
Revert "Bug 2014323 - Refine the zero-height fragment rejection to avoid overkill cases. r=TYLin,layout-reviewers"
https://github.com/mozilla-firefox/firefox/commit/c03d6a3f226b
https://hg.mozilla.org/integration/autoland/rev/3185a8f35b52
Revert "Bug 2014323 - Avoid zero-height fragments by rejecting incomplete zero-height blocks in PlaceBlock. r=layout-reviewers,dholbert"

Comment 28

[Silaghi Andreea]

Backed out as requested by Hiro on element.
https://hg.mozilla.org/integration/autoland/rev/3185a8f35b52

Comment 29

[Cosmin Sabou [:CosminS]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/5d4479056614

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2014323 - Split out `break-before: always` test cases from grid-fragmentation-020.html. r?TYLin,dholbert

Test cases involving break-before: always in
grid-fragmentation-020-ref.html are not properly tested because of bug 2020239.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2014323 - Split out `break-inside: avoid` test cases from grid-fragmentation-006.html. r?TYLin,dholbert

Test cases involving Class C breaks triggered by break-inside:avoid in
grid-fragmentation-006.html are split into a separate -avoid-break file
so they can be addressed separately in bug 2038550.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2014323 - Split out some `align-content: center` and `align-content: space-evenly` reference cases from grid-fragmentation-[020|023|028].html. r?TYLin,dholbert

Some of the reference files used by these grid reftests are affected
by D283622, causing the reftests to fail once that patch is applied.
The new rendering of the references appears correct, which means the
reftests had been relying on incorrect rendering results and need to
be revised. This will be addressed in bug 2038553.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/208b483e5bd5
https://hg.mozilla.org/integration/autoland/rev/d3eb3bf61078
Avoid zero-height fragments by rejecting incomplete zero-height blocks in PlaceBlock. r=layout-reviewers,dholbert
https://github.com/mozilla-firefox/firefox/commit/ba2236cfd795
https://hg.mozilla.org/integration/autoland/rev/ae92c4ca808c
Refine the zero-height fragment rejection to avoid overkill cases. r=TYLin,layout-reviewers
https://github.com/mozilla-firefox/firefox/commit/88702be9f270
https://hg.mozilla.org/integration/autoland/rev/472e9479771f
Split out `break-before: always` test cases from grid-fragmentation-020.html. r=layout-reviewers,dshin
https://github.com/mozilla-firefox/firefox/commit/3e977b71573f
https://hg.mozilla.org/integration/autoland/rev/08367f7651ad
Split out `break-inside: avoid` test cases from grid-fragmentation-006.html. r=layout-reviewers,dshin
https://github.com/mozilla-firefox/firefox/commit/6ce4234b30f2
https://hg.mozilla.org/integration/autoland/rev/1d89d8b5f69f
Split out some `align-content: center` and `align-content: space-evenly` reference cases from grid-fragmentation-[020|023|028].html. r=layout-reviewers,dshin

Comment 34

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/d3eb3bf61078
https://hg.mozilla.org/mozilla-central/rev/ae92c4ca808c
https://hg.mozilla.org/mozilla-central/rev/472e9479771f
https://hg.mozilla.org/mozilla-central/rev/08367f7651ad
https://hg.mozilla.org/mozilla-central/rev/1d89d8b5f69f