Closed Bug 1677917 Opened 5 months ago Closed 4 months ago

Change BlockReflowInput's skipsides setup to be sane.

Categories

(Core :: Layout, task)

task

Tracking

()

RESOLVED FIXED
85 Branch
Tracking Status
firefox85 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

This chunk of code tries to compute the right skip sides before reflow, but that seems wrong. It should probably use PreReflowBlockLevelLogicalSkipSides, and adjust the bottom bit after reflow. See the comments in https://phabricator.services.mozilla.com/D97356

Precomputing the skipBEnd bit is wrong. Using the PreReflow version
causes no regression, and a progression in a fieldset fragmentation test
which no longer creates an unnecessary page.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2da4b644dd9
Change BlockReflowInput's skipsides setup to be sane. r=mats,TYLin

So, that's http://wpt.live/css/css-break/break-at-end-container-edge-000.html, which starts not breaking after my patch, but breaks between the two inline-blocks with it.

WebKit and Blink break before the padding of the container, which makes no sense as there's no break after the second child in this case if my reading of the spec is right:

https://drafts.csswg.org/css-break/#possible-breaks:

  • Such a break wouldn't be either a Class A or Class B break.
  • To be a Class C break there's a condition of being a non-zero gap between the margin end of the child box and the parent's content edge, which is not the case.

With my patch we overflow the column, which is not super-duper amazing, but also it doesn't strike me as wrong.

I tried to allow us to break due to the overflowing padding/border, but that breaks a bunch of other reftests that rely on that behavior today (https://treeherder.mozilla.org/jobs?repo=try&revision=0dda73de7c6a43c0c7b1d2fb2eb2ce8ffee60c46).

Given that test fails everywhere else right now, and that it is only working by chance in Gecko, because right now we're not giving the right available space to the column, would you be fine with just annotating the failure Mats?

Flags: needinfo?(emilio) → needinfo?(mats)

WebKit and Blink break before the padding of the container

It's not possible to tell whether they actually intentionally break there or if they don't and then just fall back to slicing it. I'd guess it's the latter since if you manually change the 2nd child to have height:40px then they slice the padding.

What should happen in this testcase is that we should break between the two children (class B break), then the first fragment will be extended to fill the first column (making it all green) and in the 2nd column you have the 2nd child + the padding filling it, so the result is a filled green square.

With my patch we overflow the column, which is not super-duper amazing, but also it doesn't strike me as wrong.

Right, your patch is absolutely the correct approach to setting up the reflow. What happens is that the two children fits exactly so it's COMPLETE, then we need to account for the padding/border not fitting later. I looked into that code a bit and it's a bit complicated since it has to take many other factors into account as well: [min-/max-]height, whether we have a C break after the last child etc. But yeah, I think we can annotate this test as failing for now and then fix that in a follow-up bug.

Flags: needinfo?(mats)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf11f56d91cb
Change BlockReflowInput's skipsides setup to be sane. r=mats,TYLin
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26679 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.