Closed Bug 1466231 Opened 6 years ago Closed 6 years ago

Inline frames without enough inline space should clear floats, regardless of shape-outside

Categories

(Core :: Layout: Floats, enhancement)

Product:
Core
Component:
Layout: Floats
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 6 obsolete files)

float-should-push.html
1.80 KB, text/html
Details
Bug 1466231 Part 5: Update a wpt reftest to remove its reliance on fitting inline-blocks into a too-narrow space.
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1466231 Part 1: Change nsBlockFrame::PlaceLine to accept an nsFlowAreaRect and update flags if it shrinks the area due to a float.
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1466231 Part 2: Add a submitted WPT reftest.
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1466231 Part 4: Correct the size of an element in a submitted WPT test, that was only passing due to this bug.
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1466231 Part 3: Change some reftests to expected pass, one with fuzziness.
59 bytes, text/x-review-board-request
dbaron
: review+
Details
Attached file float-should-push.html (obsolete) — 
We have odd behavior with processing of inline blocks against floats. In the example testcase, float-should-push.html, there are 100 x 100 pixel floats defined a variety of ways, and a wide inline block is next to them. In all cases, there is not enough space for the inline block, and so the block should be pushed below the float. That happens correctly for "bare" floats and floats with shape-outside: inset(0). It does not happen correctly for inset(0.01px) or for polygon() with a shape that fills the 100 x 100 area.
David, I need help understanding this one. In the correct cases, nsBlockFrame::ReflowInlineFrames encounters a placeholder frame and correctly adds the float before processing the inline block frame. In the failure cases, the placeholder frame is processed AFTER the inline block frame, and therefore the available space has no floats marked, and so LineReflowStatus::RedoNextBand is never applied.

I don't understand how the placeholder frames are created, or in what order relative to the other inline frames. It seems like the order must somehow be dependent on the BStart of the FloatInfo, which would account for the difference in behavior between inset(0px) and inset(0.01px). All insights appreciated.
Flags: needinfo?(dbaron)
Attached file float-should-push.html (obsolete) — 
Updated testcase shows that shape-outside: image is another failure case.
Attachment #8982652 - Attachment is obsolete: true
So the thing that should happen to make the inline go below the float is that nsBlockFrame::DoReflowInlineFrames should set lineReflowStaus to LineReflowStatus::RedoNextBand.  I'd note that if I change the inset() one so that the top side is 0 instead of 0.01px, then things work.  This makes me realize that if the very top point in the line has sufficient width (which I think it does in the inset() case, at least -- and maybe also in the polygon case due to some rounding bug), then in order to get to that point, we'd first need to go through a more complicated sequence of events:
 * first, reflow the line with the available space between floats coming only from its top edge, discover that the height of the line causes the line to impinge on more floats, and return LineReflowStatus::RedoMoreFloats
 * then, reflow again with the float data from a height rather than an edge, and discover that nothing on the line fits, and *then* return LineReflowStatus::RedoNextBand.

That said, the testcase is pretty confusing because what should be the "</div>" for the class="container" divs are missing the "/", so everything is nested inside each other.  The testcase is a bit clearer with that fixed (and the "height" on container changed to a min-height), but it doesn't change the fundamentals.

This makes me suspect that the problem is that the shapes aren't causing creation of multiple "bands" in the float manager.  The code that causes RedoMoreFloats and the code that processes RedoNextBand is currently assuming that within a float manager "band", the available width doesn't vary.  If that assumption is now false (is it?), a bunch of the block code would no be broken -- and we'd end up with this problem.
Attached file float-should-push.html — 
Corrected closing div issue in the testcase.
Attachment #8982667 - Attachment is obsolete: true
(In reply to David Baron :dbaron: ⌚UTC-7 from comment #3)
> So the thing that should happen to make the inline go below the float is
> that nsBlockFrame::DoReflowInlineFrames should set lineReflowStaus to
> LineReflowStatus::RedoNextBand.  I'd note that if I change the inset() one
> so that the top side is 0 instead of 0.01px, then things work.  This makes
> me realize that if the very top point in the line has sufficient width
> (which I think it does in the inset() case, at least -- and maybe also in
> the polygon case due to some rounding bug), then in order to get to that
> point, we'd first need to go through a more complicated sequence of events:
>  * first, reflow the line with the available space between floats coming
> only from its top edge, discover that the height of the line causes the line
> to impinge on more floats, and return LineReflowStatus::RedoMoreFloats
>  * then, reflow again with the float data from a height rather than an edge,
> and discover that nothing on the line fits, and *then* return
> LineReflowStatus::RedoNextBand.

I'm struggling to implement this, and I could use more help. I'm having trouble finding the right way to alter the current calling pattern for layout of the too-large nsBlockFrame on the too-short band. This is my understanding of what is happening:

In nsLineLayout::ReflowFrame, nsLineLayout::CanPlaceFrame returns true (because it ignores block space), and so nsLineLayout::PlaceFrame is called. Eventually nsBlockFrame::PlaceLine sets the line size based on the mRootSpan's mMaxBCoord and mMinBCoord, notes that AvailableSpaceShrunk is true, and therefore returns false. This triggers RedoMoreFloats, but the too-large frame has already been placed.

So, I've tried to make nsLineLayout::CanPlaceFrame return false in this case. This makes the line split into a new line. The original line has no block size, so the call to nsBlockFrame::PlaceLine succeeds. RedoMoreFloats is never triggered. The next line placement happens the same as the first, with the same narrow band that doesn't consider the nearby float.

Basically I can't figure out how to get the aLine to have a definite BSize without actually placing the frames that don't fit. Can you offer any further advice?
Flags: needinfo?(dbaron)
So, for a start, I'd assume that you'd build on top of the work you've done in bug 1463745.  The underlying problem is similar -- variation in the available space within a band -- though in bug 1463745 the problem is related to the space getting wider (the float narrowing) within the band, whereas here the problem is related to the opposite.

(In reply to Brad Werth [:bradwerth] from comment #5)
> Eventually nsBlockFrame::PlaceLine sets the line size based on the
> mRootSpan's mMaxBCoord and mMinBCoord, notes that AvailableSpaceShrunk is
> true, and therefore returns false. This triggers RedoMoreFloats, but the
> too-large frame has already been placed.

This actually all sounds fine.  RedoMoreFloats means we should *redo* reflowing the line with the new, narrower, width.  So the fact that the too-large frame was placed the first time around is fine -- but we should redo reflowing the line with the narrower width, and it shouldn't get placed the second time around.

> So, I've tried to make nsLineLayout::CanPlaceFrame return false in this
> case. This makes the line split into a new line. The original line has no
> block size, so the call to nsBlockFrame::PlaceLine succeeds. RedoMoreFloats
> is never triggered. The next line placement happens the same as the first,
> with the same narrow band that doesn't consider the nearby float.
> 
> Basically I can't figure out how to get the aLine to have a definite BSize
> without actually placing the frames that don't fit. Can you offer any
> further advice?

I don't think that's a problem -- it's why we redo reflowing the line.
Flags: needinfo?(dbaron)
Blocks: shape-outside
Attachment #8985099 - Attachment is obsolete: true
Attachment #8984326 - Attachment is obsolete: true
Attachment #8984327 - Attachment is obsolete: true
Attachment #8985100 - Attachment is obsolete: true
Assignee: nobody → bwerth
Attachment #8983929 - Flags: review?(dbaron)
Attachment #8983930 - Flags: review?(dbaron)
Attachment #8985102 - Flags: review?(dbaron)
Attachment #8985101 - Flags: review?(dbaron)
Attachment #8983928 - Flags: review?(dbaron)
Comment on attachment 8983929 [details]
Bug 1466231 Part 1: Change nsBlockFrame::PlaceLine to accept an nsFlowAreaRect and update flags if it shrinks the area due to a float.

https://reviewboard.mozilla.org/r/249780/#review257230

I think this looks good, except for this one comment around the bulk of the changes.  I'd like to look at the revisions.

::: layout/generic/nsBlockFrame.cpp:4544
(Diff revision 5)
> -    // Since we want to redo the line, we update aFloatAvailableSpace by
> -    // using the aFloatStateBeforeLine, which is the float manager's state
> -    // before the line is placed.
> -    LogicalRect oldFloatAvailableSpace(aFloatAvailableSpace);
> -    aFloatAvailableSpace =
> +    // Since we want to redo the line, we update aFlowArea by using the
> +    // aFloatStateBeforeLine, which is the float manager's state before the
> +    // line is placed.
> +    LogicalRect oldFloatAvailableSpace(aFlowArea.mRect);
> +    nsFlowAreaRect oldFlowArea =
>        aState.GetFloatAvailableSpaceForBSize(aLine->BStart(),
>                                              aAvailableSpaceBSize,
> -                                            aFloatStateBeforeLine).mRect;
> -    NS_ASSERTION(aFloatAvailableSpace.BStart(wm) ==
> +                                            aFloatStateBeforeLine);
> +    aFlowArea.mRect = oldFlowArea.mRect;
> +    NS_ASSERTION(aFlowArea.mRect.BStart(wm) ==
>                   oldFloatAvailableSpace.BStart(wm), "yikes");
>      // Restore the BSize to the position of the next band.
> -    aFloatAvailableSpace.BSize(wm) = oldFloatAvailableSpace.BSize(wm);
> +    aFlowArea.mRect.BSize(wm) = oldFloatAvailableSpace.BSize(wm);

This refactoring is pretty confusing -- in particular, the naming of oldFlowArea, which is really newFlowArea.

But why not leave the code structure the same way it was, and just assign to aFlowArea directly here, and then fix up the bits just like they were fixed up before?
Attachment #8983929 - Flags: review?(dbaron) → review-
Comment on attachment 8983930 [details]
Bug 1466231 Part 2: Add a submitted WPT reftest.

https://reviewboard.mozilla.org/r/249782/#review257234

Any reason these aren't in layout/reftests/w3c-css/submitted/shapes1 (with the correct metadata)?
Attachment #8983930 - Flags: review?(dbaron)
Comment on attachment 8985102 [details]
Bug 1466231 Part 3: Change some reftests to expected pass, one with fuzziness.

https://reviewboard.mozilla.org/r/250814/#review257238

r=dbaron with the following fix

::: layout/reftests/w3c-css/submitted/shapes1/reftest.list:110
(Diff revision 4)
> -fails == float-retry-push-image.html float-retry-push-ref.html
> +# The next test offsets a 250px wide element by a pixel due to small offsets in gradient generation.
> +fuzzy(255,500) == float-retry-push-image.html float-retry-push-ref.html

Please annotate this as fuzzy(255-255,500-500) so that if the failure is fixed we'll get a TEST-UNEXPECTED_PASS.
Attachment #8985102 - Flags: review?(dbaron) → review+
Comment on attachment 8985101 [details]
Bug 1466231 Part 4: Correct the size of an element in a submitted WPT test, that was only passing due to this bug.

https://reviewboard.mozilla.org/r/250812/#review257242
Attachment #8985101 - Flags: review?(dbaron) → review+
Comment on attachment 8983928 [details]
Bug 1466231 Part 5: Update a wpt reftest to remove its reliance on fitting inline-blocks into a too-narrow space.

https://reviewboard.mozilla.org/r/249778/#review257246

Is this change actually needed?  It sounds like you're making the test test less than it used to.  And I'm also a little less inclined to change tests contributed by others unless they're actually broken.
Comment on attachment 8983928 [details]
Bug 1466231 Part 5: Update a wpt reftest to remove its reliance on fitting inline-blocks into a too-narrow space.

https://reviewboard.mozilla.org/r/249778/#review257246

The change is needed to make the test pass in Firefox. Yes, this makes the test narrower than it was originally. The part that is being removed is whether the first child frame of a line should fit no matter what. I'm not familiar enough with the spec to judge it, but it's clearly not an issue that's specific to shape-outside. I am assuming that correct behavior on this issue is already covered by tests and the fact that this patch didn't break any of those tests is an indication that we still comply with the spec.
Comment on attachment 8983930 [details]
Bug 1466231 Part 2: Add a submitted WPT reftest.

https://reviewboard.mozilla.org/r/249782/#review257234

It was a conscious choice on my part. I'm willing to go the other way if you feel it would be correct. Here is my thinking: though this issue is revealed by shape-outside floats, it is not ABOUT shape-outside floats. The bug is in the float layout code, and will trigger on any float that doesn't "hit" on a zero-height line aimed at its BStart. In that case, nsBlockFrame::PlaceLine will fail to update the nsFlowAreaRect with the HAS_FLOATS flag, no optional break position will be set, and that will allow the frame to be placed in nsLineLayout::CanPlaceFrame because aNotSafeToBreak is true. All of this feels very specific to Firefox float layout code, thus I propose the test goes in layout/reftests/floats. Additionally, there is an open bug on a closely related issue in Bug 1309830 that has nothing to do with shape-outside, and that test (currently failing) is in layout/reftests/floats.
Comment on attachment 8983929 [details]
Bug 1466231 Part 1: Change nsBlockFrame::PlaceLine to accept an nsFlowAreaRect and update flags if it shrinks the area due to a float.

https://reviewboard.mozilla.org/r/249780/#review257280

::: layout/generic/nsBlockFrame.cpp:4544
(Diff revision 5)
> -    // Since we want to redo the line, we update aFloatAvailableSpace by
> -    // using the aFloatStateBeforeLine, which is the float manager's state
> -    // before the line is placed.
> -    LogicalRect oldFloatAvailableSpace(aFloatAvailableSpace);
> -    aFloatAvailableSpace =
> +    // Since we want to redo the line, we update aFlowArea by using the
> +    // aFloatStateBeforeLine, which is the float manager's state before the
> +    // line is placed.
> +    LogicalRect oldFloatAvailableSpace(aFlowArea.mRect);
> +    nsFlowAreaRect oldFlowArea =
>        aState.GetFloatAvailableSpaceForBSize(aLine->BStart(),
>                                              aAvailableSpaceBSize,
> -                                            aFloatStateBeforeLine).mRect;
> -    NS_ASSERTION(aFloatAvailableSpace.BStart(wm) ==
> +                                            aFloatStateBeforeLine);
> +    aFlowArea.mRect = oldFlowArea.mRect;
> +    NS_ASSERTION(aFlowArea.mRect.BStart(wm) ==
>                   oldFloatAvailableSpace.BStart(wm), "yikes");
>      // Restore the BSize to the position of the next band.
> -    aFloatAvailableSpace.BSize(wm) = oldFloatAvailableSpace.BSize(wm);
> +    aFlowArea.mRect.BSize(wm) = oldFloatAvailableSpace.BSize(wm);

I've changed the code to directly set aFlowArea and then fix it up. The final update to mFlags goes away, as it's now handled in the assignment to aFlowArea.
Comment on attachment 8983929 [details]
Bug 1466231 Part 1: Change nsBlockFrame::PlaceLine to accept an nsFlowAreaRect and update flags if it shrinks the area due to a float.

https://reviewboard.mozilla.org/r/249780/#review257390
Attachment #8983929 - Flags: review?(dbaron) → review+
Comment on attachment 8983928 [details]
Bug 1466231 Part 5: Update a wpt reftest to remove its reliance on fitting inline-blocks into a too-narrow space.

https://reviewboard.mozilla.org/r/249778/#review257392

r=dbaron with the following

::: commit-message-cec4a:3
(Diff revision 6)
> +This reftest currently uses a pixel that is 1 pixel narrower than needed
> +to fit two blocks side-by-side. That's useful for making the first two blocks
> +land on separate lines, but it makes the test rely on the browser forcing the
> +first inline block on a line to fit even if there is no space for it. That may
> +be correct behavior, but this test does not need to rely on that behavior to
> +test the correct float shape of the repeating gradient.

OK, I understand the change you're making now, but the commit message clearer.

"pixel that is" -> "container that is"

"no space for it" -> "no space for it (in this case, the block that should be pushed to the right)"

And drop the sentence on "That may be correct behavior", since I don't believe it is.
Attachment #8983928 - Flags: review?(dbaron) → review+
Comment on attachment 8983930 [details]
Bug 1466231 Part 2: Add a submitted WPT reftest.

https://reviewboard.mozilla.org/r/249782/#review257394

I don't think being a Gecko-specific bug is justification for not contributing the test.  (Justification would be that we don't believe the behavior is required by the standard.)

In general, we should probably be putting new tests into web-platform-tests, not here.

That said, I'm OK with this for now, plus a followup patch to move the test later.
Attachment #8983930 - Flags: review?(dbaron) → review+
Comment on attachment 8983930 [details]
Bug 1466231 Part 2: Add a submitted WPT reftest.

https://reviewboard.mozilla.org/r/249782/#review257394

I'll make it a submitted WPT test, in shapes1.
Please do get this landed before the merge (you can fix the test for WPT later).  (I notice you landed bug 1457297 first.)
Flags: needinfo?(bwerth)
Yep, just doing one more try run now that I've moved the tests to WPT.
Flags: needinfo?(bwerth)
(In reply to Brad Werth [:bradwerth] from comment #73)
> Yep, just doing one more try run now that I've moved the tests to WPT.

And if there's any failure, I'll roll back to the pre-WPT version of the test and merge that, etc.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f926e605d5a9
Part 1: Change nsBlockFrame::PlaceLine to accept an nsFlowAreaRect and update flags if it shrinks the area due to a float. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/332453c76ec7
Part 2: Add a submitted WPT reftest. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/012e16dcc677
Part 3: Change some reftests to expected pass, one with fuzziness. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/2250da9a0dd0
Part 4: Correct the size of an element in a submitted WPT test, that was only passing due to this bug. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/8141b2848c2c
Part 5: Update a wpt reftest to remove its reliance on fitting inline-blocks into a too-narrow space. r=dbaron
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11536 for changes under testing/web-platform/tests
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a8d4d870932
Part 1: Change nsBlockFrame::PlaceLine to accept an nsFlowAreaRect and update flags if it shrinks the area due to a float. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/44b9d3b11c4d
Part 2: Add a submitted WPT reftest. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/23d8e8736a00
Part 3: Change some reftests to expected pass, one with fuzziness. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/4d4ce1a9b81a
Part 4: Correct the size of an element in a submitted WPT test, that was only passing due to this bug. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/074014588d2f
Part 5: Update a wpt reftest to remove its reliance on fitting inline-blocks into a too-narrow space. r=dbaron
(In reply to Cosmin Sabou [:CosminS] from comment #79)
> Backed out for reftest failures on float-retry-push-image.html.

I made the fuzziness too strict on this test and there are small differences per platform that triggered the unexpected-pass. I've updated and re-pushed the patches.

> Please also take a look at these failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=8141b2848c2c01542cdca4f068ebec284cac2889&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=runnable&selectedJob=183396726

That WPT reftest requires some fuzziness to be correct (as many WPT reftests do for shape-outside). Since WPT reftests have no allowance for fuzziness, it trips up the test-verify job. The tests are often inherited from other vendors, and when we duplicate an existing in-tree test to add some nuance -- generating the same result -- the new test fails the test-verify job. I have no solution for this.
Upstream PR was closed without merging
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Flags: needinfo?(bwerth)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: