1307853 - {inc}On horizontal resize, we don't correctly resize block with box-sizing:border-box, fixed width, and percentage padding (when unaffected by floats)

Status: VERIFIED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Chris Mills [:cmills]]

Recently we've been writing some documentation on CSS Grid frameworks/native grids for MDN:

https://developer.mozilla.org/en-US/docs/Learn/CSS/CSS_layout/Grids

One thing we noticed was that when you resize a window containing a CSS grid framework-generated grid in Firefox, the grid starts to distort - the columns start to go out of line. You can see this effect in the following demo page:

http://mdn.github.io/learning-area/css/css-layout/grids/fluid-grid.html

We've seen this problem in Nightly and Firefox release, on both Windows and Mac. Note that it mainly seems to be a problem with fluid grids (i.e. sized by percentages), so it is probably some kind of rounding error in Gecko.

The problem doesn't appear in Chrome/Opera/Safari.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Mats, could you have a look at this?

Comment 2

[Mats Palmgren (inactive)]

The given example isn't using Grid layout at all.
It's using percentage-sized floats to simulate a grid or something.

Comment 3

[Chris Mills [:cmills]]

(In reply to Mats Palmgren (:mats) from comment #2)
> The given example isn't using Grid layout at all.
> It's using percentage-sized floats to simulate a grid or something.

Yes, apologies - I perhaps didn't phrase this very well. I am not talking about the CSS Grid spec (that works wonderfully and I can't wait for it to be available everywhere!) - I am talking about an HTML/CSS grid framework; a simulated grid like we do currently in the absence of the CSS Grid spec.

The linked example is a homegrown HTML/CSS framework we wrote ourselves, to demonstrate the way these kinds of CSS framework work.

Comment 4

[Sebastian Zartner [:sebo]]

FWIW, after a reload of the page the layout looks correct again, so this really just happens on resizing the viewport.

I've tested this on Windows 7 and 10 using Firefox 49.0.1 to 52.0a1.

Sebastian

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reporter's testcase

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: somewhat simpler testcase

The problem is that we're failing to resize lines that we don't detect as being adjacent to floats.  This means we're failing to properly resize the content on the first line.  But if you uncomment the height declaration in this testcase, then we fail to resize all the lines.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the underlying problem here is that on horizontal resize, we don't correctly resize block with box-sizing:padding box, fixed width, and percentage padding (when it's unaffected by floats).  In the original more complicated testcase, I think there's something that makes us do the resize some of the time (leading to the width being only slightly off some of the time), but in my simpler testcase we more reliably fail to resize the first line.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think the basic problem here is the following:

In ReflowInput::InitResizeFlags we initialize the IsIResize ("is this frame resizing in the inline direction") flag based on whether the border-box is resizing.

However, when:
 * the width is fixed
 * box-sizing is border-box, and
 * there's a percentage side padding
then it's possible for the content box to resize even when neither (a) the border box is resizing or (b) there was a style change.  (I don't see how it's possible without all three of those conditions.)

This means that nsBlockFrame::Reflow won't call PrepareResizeReflow, and we'll end up with the bug.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the best solution would be to set the flag when *either* the content-box or border-box width is changing.  It might even be safe to do it only when the content-box width is changing, but that would be riskier.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I wrote patches that I thought would work:

https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ed5763a9b160/set-iresize-content-box
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ed5763a9b160/iframe-resize-test

but the patch doesn't work, because my call to mFrame->ContentISize(wm) in ReflowInput::InitResizeFlags is using the new usedPaddingProperty.  (Just reordering ReflowInput initialization order wouldn't be enough to fix this, since we might construct 2 different ReflowInput objects and want the resize flags to be right on the second one.)


I'm really not sure how to fix this in an efficient way.  Maybe we should just explicitly check for the presence of percentage padding combined with box-sizing != content-box when setting resize flags?  I think that would catch the cases I currently know to be broken.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7a5e4e2c435fd6a51e720df439ddc0c8c7af002&group_state=expanded

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Set inline-resize flag when the content-box size is changing (in addition to border-box)

MozReview-Commit-ID: FjUwuQB8g5j

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Add web platform test (in mochitest suite)

This test failed with the initial (non-working) version of the patch,
and passes with the patch.

MozReview-Commit-ID: IPmd7Yh604Z

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8800028 [details] [diff] [review]
Set inline-resize flag when the content-box size is changing (in addition to border-box)

r=mats, with a few nits fixed as you see fit.

>+  // (It's possible for
>+  // the content-box size to change without a border-box size change or
>+  // a style change given a fixed width, box-sizing:border-box, and
>+  // percentage padding.

I think you mean "given a fixed max-width", right?  I don't see how
the content-box size can change with a fixed 'width' value.
Also, the end-paren is missing.

>+  // That case is the
>+  // combination of percentage padding with box-sizing other than the
>+  // default content-box.

I don't understand what this sentence is trying to say.
It seems superfluous.

>   bool isIResize =
>     mFrame->ISize(wm) !=
>-      ComputedISize() + ComputedLogicalBorderPadding().IStartEnd(wm);
>+      ComputedISize() + ComputedLogicalBorderPadding().IStartEnd(wm) ||
>+    (mStylePosition->mBoxSizing != StyleBoxSizing::Content &&
>+     mStylePadding->IsWidthDependent());

After the long comment above, I still think it's hard for the reader to
understand which part of the isIResize expression it concerns.
Perhaps this would be clearer if you separate out that part?
E.g.

// long comment here
bool contentISizeMayChangeDueToPercentPadding =
  mStylePosition->mBoxSizing != StyleBoxSizing::Content &&
  mStylePadding->IsWidthDependent();
bool isIResize = ... || contentISizeMayChangeDueToPercentPadding;

Comment 15

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #14)
> I think you mean "given a fixed max-width", right?  I don't see how
> the content-box size can change with a fixed 'width' value.

Ah, border-box sizing, gotcha.  Need more coffee here... :-)

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Review comments addressed with these changes:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/0e9cf8aab094

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d61b47e3ac8a46f515978ac9cc2628485650b7f7
Bug 1307853 - Set inline-resize flag when the content-box size is changing (in addition to border-box).  r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/8002cb8086cd19892c926d7d511db954bde8653b
Bug 1307853 - Add web platform test (in mochitest suite).  r=mats

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d61b47e3ac8a
https://hg.mozilla.org/mozilla-central/rev/8002cb8086cd

Comment 19

[Sebastian Zartner [:sebo]]

Works fine for me in Nightly 52.0a1 2016-10-16. Thank you for the quick fix, David!

Sebastian