Change BlockReflowInput's skipsides setup to be sane.
Categories
(Core :: Layout, task)
Tracking
()
| 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
|
Assignee | |
Comment 1•4 years ago
|
||
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.
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 2•4 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b2da4b644dd9 Change BlockReflowInput's skipsides setup to be sane. r=mats,TYLin
|
||
Comment 4•4 years ago
|
||
Backed out for perma failures.
Log: https://treeherder.mozilla.org/logviewer?job_id=322538401&repo=autoland&lineNumber=1554
Backout: https://hg.mozilla.org/integration/autoland/rev/8b21247508414f1cadb6db9aac1a53055259cbff
|
|
Assignee | |
Comment 5•3 years ago
|
||
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?
|
||
Comment 6•3 years ago
•
|
||
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.
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
|
||
Comment 9•3 years ago
|
||
| bugherder | ||
Upstream PR merged by moz-wptsync-bot
Description
•