The red inline-block element is too tall to fit the space to the right of the dark green box, and should be place next to the light green box.

I think this is probably a duplicate of bug 437920.

Review commit: https://reviewboard.mozilla.org/r/73344/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/73344/

After the line is completely reflowed, we query the float available space by using the line's BSize() in nsBlockFrame::PlaceLine(), which may cause the line to reflow again due to LINE_REFLOW_REDO_MORE_FLOATS reason. The issue lies in the second reflow. That is, we do not leverage the line's BSize() computed in the first reflow to query the float available space when updating the line layout band in BlockReflowInput::AddFloat(). So some tall inline elements could still overlap the floats as in the first reflow. To fix this we could use the max value of mMinLineHeight and the line's BSize() to get the precise float available size when updating nslineLayout. Review commit: https://reviewboard.mozilla.org/r/73346/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/73346/

To implement shape-outside, this bug need to be fixed.

Rebased to keep patches from bitrotten.

Comment on attachment 8783568 [details] Bug 1291110 Part 1 - Fix log and comment related to float. https://reviewboard.mozilla.org/r/73344/#review77436 ::: layout/generic/BlockReflowInput.cpp:359 (Diff revision 3) > } > > #ifdef DEBUG > if (nsBlockFrame::gNoisyReflow) { > nsFrame::IndentBy(stdout, nsBlockFrame::gNoiseIndent); > - printf("GetAvailableSpace: band=%d,%d,%d,%d hasfloats=%d\n", > + printf("%s: band=%d,%d,%d,%d hasfloats=%d\n", __FUNCTION__, This should use \_\_func\_\_, which is standard, rather than \_\_FUNCTION\_\_. ::: layout/generic/BlockReflowInput.cpp:392 (Diff revision 3) > } > > #ifdef DEBUG > if (nsBlockFrame::gNoisyReflow) { > nsFrame::IndentBy(stdout, nsBlockFrame::gNoiseIndent); > - printf("GetAvailableSpaceForHeight: space=%d,%d,%d,%d hasfloats=%d\n", > + printf("%s: space=%d,%d,%d,%d hasfloats=%d\n", __FUNCTION__, Same here.

Comment on attachment 8783569 [details] Bug 1291110 Part 4 - Use line BSize to query available space when updating nsLineLayout. https://reviewboard.mozilla.org/r/73346/#review77440 ::: layout/generic/BlockReflowInput.cpp:633 (Diff revision 3) > + if (line) { > + lineBSize = std::max(mMinLineHeight, (*line)->BSize()); > + } So if I'm understanding this correctly, this will use the line's BSize from the previous reflow of that line. That might be a previous pass of this reflow, but it might also be a previous reflow prior to some dynamic changes. This means that you're making layout depend on the *previous* state of the DOM, which violates the invariant that (ignoring things like animations/transitions) any sequence of dynamic changes that produces the same DOM and style should produce the same layout. Instead of this, I think maybe the right approach is probably something *roughly* like caching somewhere (nsLineLayout? BlockReflowInput?) the height that we used in nsBlockFrame::PlaceLine in the previous pass when we decide to reject the placement and return false. (In the first pass, we'd still use GetFloatAvailableSpace, not GetFloatAvailableSpaceForBSize.) You'd also need to be careful to clear that cached height if we redo reflowing the line at a different vertical position. Otherwise, I think this is the right approach; I never realized we were failing to call UpdateBand correctly there, and I think that is the right thing to do here. But please let me know if you think I'm misunderstanding something here. Floats are complicated, and even after reviewing this somewhat carefully I'm not entirely confident in what I'm saying. (And sorry for the delay in reviewing it.)

Comment on attachment 8787658 [details] Bug 1291110 Part 3 - Reflow the line again after splitting if it has floats and itss BSize() shrinks. https://reviewboard.mozilla.org/r/76358/#review77446 I don't think this is the right approach, since I think it can cause infinite loops. The way we avoid infinite loops is by only ever considering more height, and never reducing the height that we consider -- and at the same time, only ever reducing the width and never increasing it. That said, I don't remember all the interactions here. If you have a good argument that this won't cause infinite loops, I'd be willing to consider it. But it seems like this could cause infinite loops if there's something near the end of the line that both (a) contains a new float and (b) increases the height of the line.

(And if you want me to review a revised patch before I return from TPAC, please needinfo? me; I currently have review requests disabled. I can't promise I'll get to it since I'll be pretty busy, but I'll try.)

Comment on attachment 8783569 [details] Bug 1291110 Part 4 - Use line BSize to query available space when updating nsLineLayout. https://reviewboard.mozilla.org/r/73346/#review77440 > So if I'm understanding this correctly, this will use the line's BSize from the previous reflow of that line. That might be a previous pass of this reflow, but it might also be a previous reflow prior to some dynamic changes. > > This means that you're making layout depend on the *previous* state of the DOM, which violates the invariant that (ignoring things like animations/transitions) any sequence of dynamic changes that produces the same DOM and style should produce the same layout. > > Instead of this, I think maybe the right approach is probably something *roughly* like caching somewhere (nsLineLayout? BlockReflowInput?) the height that we used in nsBlockFrame::PlaceLine in the previous pass when we decide to reject the placement and return false. (In the first pass, we'd still use GetFloatAvailableSpace, not GetFloatAvailableSpaceForBSize.) You'd also need to be careful to clear that cached height if we redo reflowing the line at a different vertical position. > > Otherwise, I think this is the right approach; I never realized we were failing to call UpdateBand correctly there, and I think that is the right thing to do here. > > But please let me know if you think I'm misunderstanding something here. Floats are complicated, and even after reviewing this somewhat carefully I'm not entirely confident in what I'm saying. (And sorry for the delay in reviewing it.) Yes. You understand my intension clearly. Now I'm trying to still use GetFloatAvailableSpace() in the first pass to update band, and cache the line's BSize in BlockReflowInput when we reject the line placement, but I haven't got it working yet. For now I got this issue: In nsBlockFrame::PlaceLine(), it queries GetFloatAvailableSpaceForBSize() with the float manager state *before* placing this line [1]. So if the floats and the tall inline-block are on the same line like my test 1291110-1.html, the query result for GetFloatAvailableSpaceForBSize() is as if there's no float, which is wrong. Keep digging. [1] http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/layout/generic/nsBlockFrame.cpp#4457-4459

Comment on attachment 8787658 [details] Bug 1291110 Part 3 - Reflow the line again after splitting if it has floats and itss BSize() shrinks. https://reviewboard.mozilla.org/r/76358/#review77446 I haven't found a correct approach to fix 345369-2.html. I would like to put aside this for now, and post my revised patch (part 1 & part 2) for review since we need part 2 to implement shape-outside.

Latest try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28820c1ee924

Comment on attachment 8783569 [details] Bug 1291110 Part 4 - Use line BSize to query available space when updating nsLineLayout. https://reviewboard.mozilla.org/r/73346/#review83300 ::: layout/generic/BlockReflowInput.cpp:631 (Diff revision 4) > - nsFlowAreaRect floatAvailSpace = GetFloatAvailableSpace(mBCoord); > + // If we have mLineBSize, we are reflowing the line again due to > + // LineReflowStatus::RedoMoreFloats. We should use mLineBSize to query the > + // correct available space. > + nsFlowAreaRect floatAvailSpace = > + mLineBSize.isNothing() > + ? GetFloatAvailableSpace(mBCoord) > + : GetFloatAvailableSpaceForBSize(mBCoord, mLineBSize.value(), nullptr); Here it's worth adding a comment pointing out that, even though the BSize() of the result for the "ForBSize" variant has a different meaning (it's the BSize passed in, rather than the height to the next band of floats -- something we should probably have some additional comments pointing out), this is OK since nsLineLayout::UpdateBand doesn't use the BSize of its availSpace parameter. ::: layout/generic/nsBlockFrame.cpp:4457 (Diff revision 4) > - // We want to compare to the available space that we would have had in > - // the line's height *before* we placed any floats in the line itself. > - // Floats that are in the line are handled during line reflow (and may > - // result in floats being pushed to below the line or (I HOPE???) in a > - // reflow with a forced break position). > - LogicalRect oldFloatAvailableSpace(aFloatAvailableSpace); > + // We need to consider the floats in the current line. If there's no > + // mFloatAvailableSpaceForLineBSize, we are in the first pass trying to place > + // the line. Calling GetFloatAvailableSpace() like we do when updating line > + // reflow engine after placing a float in BlockReflowInput::AddFloat(). > + LogicalRect oldFloatAvailableSpace = > + aState.mFloatAvailableSpaceForLineBSize.isNothing() > + ? aState.GetFloatAvailableSpace(aLine->BStart()).mRect > + : aState.mFloatAvailableSpaceForLineBSize.value(); > + > // As we redo for floats, we can't reduce the amount of height we're > // checking. > aAvailableSpaceHeight = std::max(aAvailableSpaceHeight, aLine->BSize()); > + LogicalRect floatAvailableSpaceForLineBSize = > + aState.GetFloatAvailableSpaceForBSize(aLine->BStart(), > + aAvailableSpaceHeight, > + nullptr).mRect; > + > + // If the available space between the floats is smaller now that we > + // know the height, return false (and cause another pass with > + // LineReflowStatus::RedoMoreFloats). We ensure aAvailableSpaceHeight > + // never decreases, which means that we can't reduce the set of floats > + // we intersect, which means that the available space cannot grow. > + if (AvailableSpaceShrunk(wm, oldFloatAvailableSpace, > + floatAvailableSpaceForLineBSize, false)) { > + // Prepare data for redo the line. > + aState.mLineBSize = Some(aLine->BSize()); > + aState.mFloatAvailableSpaceForLineBSize = > + Some(floatAvailableSpaceForLineBSize); > + > + // Since we want to redo the line, we update aFloatAvailableSpace by using I don't understand why PlaceLine has so many changes. What was wrong with keeping it mostly as it was, and just saving the two variables in mState as needed? (Note that this approach does require passing aFloatStateBeforeLine through to GetFloatAvailableSpaceForBSize, as the existing and now (with patch) third call does.)

Also, from your commit message (which MozReview tries very hard to hide from me by hiding it in bugzilla and jumping past it in the review UI): > Another issue is in nsBlockFrame::PlaceLine(). When determined whether > the available space is shrunk, we use the float manager's state *before* > placing the line. So if current line has floats, they're not considered. It seems to me that in some (but not all) cases this should lead to the float being placed below the line, rather than making the line narrower. I believe we handle this for floats that intersect the top of the line, though handling it for floats that only intersect lower parts of the line is probably harder. But I'm not convinced that it should be handled this way. It's not clear to me if this sort of a situation should lead to the line being narrower, or to the float being placed below the line. But even then, it's not clear to me how our code would end up hitting such a case, since I'd think BlockReflowInput::AddFloat would put such a float in mBelowCurrentLineFloats even if it only needs to be pushed partway down the height of the current line rather than entirely below it.

Comment on attachment 8783569 [details] Bug 1291110 Part 4 - Use line BSize to query available space when updating nsLineLayout. https://reviewboard.mozilla.org/r/73346/#review83300 > Here it's worth adding a comment pointing out that, even though the BSize() of the result for the "ForBSize" variant has a different meaning (it's the BSize passed in, rather than the height to the next band of floats -- something we should probably have some additional comments pointing out), this is OK since nsLineLayout::UpdateBand doesn't use the BSize of its availSpace parameter. I'll add a comment here which says the following in next patchset. How about it? "When calling GetFloatAvailableSpaceForBSize(), the BSize() of the returned floatAvailSpace.mRect changes its meaning to the BSize passed in, rather than the height to the next band of floats when calling GetFloatAvailableSpace. However this is OK since nsLineLayout::UpdateBand() doesn't use the BSize of its availSpace parameter." I'll add comment to nsFloatManager::GetFlowArea() to point this out to when I modify the function while implmenting shape-outside. > I don't understand why PlaceLine has so many changes. > > What was wrong with keeping it mostly as it was, and just saving the two variables in mState as needed? > > (Note that this approach does require passing aFloatStateBeforeLine through to GetFloatAvailableSpaceForBSize, as the existing and now (with patch) third call does.) I changed this much because the floats in this line might affect the line's available space as you quoted in comment 25. If we just save the two variables in aState as needed (with aFloatStateBeforeLine pass through to GetFloatAvailableSpaceForBSize), the test 1291110-1.html added in this patch still fails.

Re comment 25: > It seems to me that in some (but not all) cases this should lead to the > float being placed below the line, rather than making the line narrower. I > believe we handle this for floats that intersect the top of the line, though > handling it for floats that only intersect lower parts of the line is > probably harder. But I'm not convinced that it should be handled this way. I'll sleep on it and see if I can think of a counterexample that my approach breaks. Perhaps this will lead to other ideas. > But even then, it's not clear to me how our code would end up hitting such a > case, since I'd think BlockReflowInput::AddFloat would put such a float in > mBelowCurrentLineFloats even if it only needs to be pushed partway down the > height of the current line rather than entirely below it. Our code will place the two floats in 1291110-1.html immediately on the same line.

Re comment 25: > But even then, it's not clear to me how our code would end up hitting such a > case, since I'd think BlockReflowInput::AddFloat would put such a float in > mBelowCurrentLineFloats even if it only needs to be pushed partway down the > height of the current line rather than entirely below it. I'll explain more for the concern above. BlockReflowInput::AddFloat() always places floats when the line is empty. One example is 1291110-1.html. The container has two floats following by an inline-block. When checking whether to place the second float immediately, aLineLayout->LineIsEmpty() [1] is still true because the first float frame is an placeholder which is an empty-frame. (A non-empty frame changes mLineIsEmpty to at [2].) As a result, the second float doesn't go into mBelowCurrentLineFloats even if it cannot fit the available space so it's only pushed partway down the height rather than entirely below it. In 1291110-1.html, if the inline-block and second float was swapped, i.e make it be float, inline-block, float, then the second float will be pushed entire below the line. We already do correctly in this case. David, would the above explanation make the patch more convincing? Or you might have other ideas that I could try? I believe fixing this bug will bring our floats rendering closer to what Blink and Webkit do. Both of them render these tests correctly. [1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/generic/BlockReflowInput.cpp#623 [2] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/generic/nsLineLayout.cpp#1111

Comment on attachment 8783569 [details] Bug 1291110 Part 4 - Use line BSize to query available space when updating nsLineLayout. https://reviewboard.mozilla.org/r/73346/#review90740 OK, after thinking about this further and talking about it in person, I'm now OK with the patch with a few changes, two of which are in the comments below. The third one should, I think, be a separate patch, probably *before* this one in the patch queue if that doesn't cause any merge conflicts. That patch should change PlaceLine so that it *enforces* that the available space never widens on either side, since I believe it's now possible for the available space to grow because fewer floats were placed on the line in this pass than the previous pass. This means that failing to enforce that it never grows could lead to infinite grow-shrink loops. I think that patch can also remove the aCanGrow parameter from AvailableSpaceShrunk and the related assertions inside of it, since the caller that passes aCanGrow == false will now be enforcing that the available space doesn't grow. r=dbaron with those changes ::: layout/generic/nsBlockFrame.cpp:4461 (Diff revision 4) > - // reflow with a forced break position). > - LogicalRect oldFloatAvailableSpace(aFloatAvailableSpace); > + LogicalRect oldFloatAvailableSpace = > + aState.mFloatAvailableSpaceForLineBSize.isNothing() > + ? aState.GetFloatAvailableSpace(aLine->BStart()).mRect > + : aState.mFloatAvailableSpaceForLineBSize.value(); I think it would be clearer to do two things here: (1) rename oldFloatAvailableSpace to floatAvailableSpaceWithOldHeight, and comment that it's the float available space with the old height, but including the floats that were added in this line (2) in the : part of your ?:, actually call GetFloatAvailableSpaceForBSize again, using aState.mLineBSize and aState.mLineBSize.IsSome(). (You can then remove aState.mFloatAvailableSpaceForLineBSize.) This will mean that you're consistent about doing the comparison (between the two different heights) always using the set of floats present in the line in the current reflow pass, rather than, as your patch currently does, sometimes using the set of floats present in the line on the previous pass. ::: layout/generic/nsBlockFrame.cpp:4489 (Diff revision 4) > + Some(floatAvailableSpaceForLineBSize); > + > + // 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); Here you only need to save aFloatAvailableSpace.BSize(wm) rather than saving the whole thing.

Comment on attachment 8783569 [details] Bug 1291110 Part 4 - Use line BSize to query available space when updating nsLineLayout. https://reviewboard.mozilla.org/r/73346/#review90740 > The third one should, I think, be a separate patch, probably before this one in the patch queue if that doesn't cause any merge conflicts. I made the patch in Part 3, and Part 2 is a simple patch to rename availableSpaceHeight to availableSpaceBSize to make the terminology consistent with writing-mode. > I think it would be clearer to do two things here: > > (1) rename oldFloatAvailableSpace to floatAvailableSpaceWithOldHeight, and comment that it's the float available space with the old height, but including the floats that were added in this line > > (2) in the : part of your ?:, actually call GetFloatAvailableSpaceForBSize again, using aState.mLineBSize and aState.mLineBSize.IsSome(). (You can then remove aState.mFloatAvailableSpaceForLineBSize.) This will mean that you're consistent about doing the comparison (between the two different heights) always using the set of floats present in the line in the current reflow pass, rather than, as your patch currently does, sometimes using the set of floats present in the line on the previous pass. I addressed the above comments, but I use floatAvailableSpaceWithOldBSize to match the writing-mode terminology used here.

Comment on attachment 8808550 [details] Bug 1291110 Part 2 - Rename availableSpaceHeight to availableSpaceBSize. https://reviewboard.mozilla.org/r/91356/#review91212

Comment on attachment 8808551 [details] Bug 1291110 Part 3 - Enforce float available space never grow on either side. https://reviewboard.mozilla.org/r/91358/#review91214 ::: layout/generic/nsBlockFrame.cpp:4495 (Diff revision 1) > + if (aFloatAvailableSpace.ISize(wm) > oldFloatAvailableSpace.ISize(wm)) { > + aFloatAvailableSpace.ISize(wm) = oldFloatAvailableSpace.ISize(wm); > + } This isn't quite right, since the check on ISize() isn't good enough if IStart has moved inwards. You should instead do the comparison using IEnd(), and then if the comparison fails, you need to include the difference between the two IStart() values in the setting. r=dbaron with that, although maybe I should double-check the revisions

Comment on attachment 8808551 [details] Bug 1291110 Part 3 - Enforce float available space never grow on either side. https://reviewboard.mozilla.org/r/91358/#review91214 > This isn't quite right, since the check on ISize() isn't good enough if IStart has moved inwards. > > You should instead do the comparison using IEnd(), and then if the comparison fails, you need to include the difference between the two IStart() values in the setting. > > r=dbaron with that, although maybe I should double-check the revisions You're right this isn't correct. I've updated Part 3 with a different approach using diffs. I would appreciate it if you can double-check the logic.

Comment on attachment 8808551 [details] Bug 1291110 Part 3 - Enforce float available space never grow on either side. https://reviewboard.mozilla.org/r/91358/#review91806 This looks good. One nit on the comment: ::: layout/generic/nsBlockFrame.cpp:4490 (Diff revision 2) > NS_ASSERTION(aFloatAvailableSpace.BStart(wm) == > oldFloatAvailableSpace.BStart(wm), "yikes"); > // Restore the BSize to the position of the next band. > aFloatAvailableSpace.BSize(wm) = oldFloatAvailableSpace.BSize(wm); > + > + // Enforce both IStart() and IEnd() never grow to prevent infinite I'd probably say "move outwards" or "expand" rather than "grow", since "grow" could be taken to mean the number getting bigger.

Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0bc5e1c941a2 Part 1 - Fix log and comment related to float. r=dbaron https://hg.mozilla.org/integration/autoland/rev/0ddd1de392d8 Part 2 - Rename availableSpaceHeight to availableSpaceBSize. r=dbaron https://hg.mozilla.org/integration/autoland/rev/5ebd8aebc99f Part 3 - Enforce float available space never grow on either side. r=dbaron https://hg.mozilla.org/integration/autoland/rev/ae7543286030 Part 4 - Use line BSize to query available space when updating nsLineLayout. r=dbaron

https://hg.mozilla.org/mozilla-central/rev/0bc5e1c941a2 https://hg.mozilla.org/mozilla-central/rev/0ddd1de392d8 https://hg.mozilla.org/mozilla-central/rev/5ebd8aebc99f https://hg.mozilla.org/mozilla-central/rev/ae7543286030