Closed Bug 1745310 Opened 3 years ago Closed 3 years ago

Behavior-difference with Chrome/WebKit, with larger float next to display:flex/grid/flow-root with negative margin

Categories

(Core :: Layout: Floats, defect)

defect

Tracking

()

RESOLVED FIXED
102 Branch
Webcompat Priority P1
Tracking Status
firefox-esr91 --- wontfix
firefox100 --- wontfix
firefox101 --- wontfix
firefox102 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

893 bytes, text/html
Details
918 bytes, text/html
Details
1004 bytes, text/html
Details
1.25 KB, text/html
Details
1.65 KB, text/html
Details
1.69 KB, text/html
Details
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
Attached file testcase 1

I'm filing this bug mostly to have a central place to track a category of known interop pain, where we are pretty sure that we're correct, but sites seem to be depending on the Chromium/WebKit behavior.

Hopefully Chromium/WebKit fix their bug. Worst-case: if they don't fix the bug and we continue hitting interop issues due to this, then we'd eventually need to get their behavior specced so we could see whether it can be coherently described and implemented on our end.

STR:

  1. Load attached testcase.

FIREFOX RESULTS:
Case 1 and Case 2 inside of the testcase should look the same.

CHROMIUM/WEBKIT RESULTS:
In "Case 2", the "A B" text is pushed down, for no clear reason.

The only difference between Case 1 and Case 2 is: in Case 2, the "A B" element has a negative margin. So if anything, it should fit even better; there should be even less reason to push it down. This is why I'm pretty sure this is a bug in the other engines, rather than a bug in Firefox.

Note also that "legacy" EdgeHTML (e.g. Edge 18) matches the Firefox behavior here (just as an independent confirmation that this behavior is sensible). I don't think they recognize display:flow-root, so their rendering of testcase 2 isn't necessarily meaningful; but they definitely support display:flex and everything else in testcase 1.

We've got https://github.com/twbs/bootstrap/issues/35277 filed to change Bootstrap to have them stop relying on this technique to do line-wrapping (or something like that), BTW.

Also, note that https://github.com/web-platform-tests/interop-2022/issues/33 is filed on potentially including this in the interop2022 effort.

Strictly speaking, it looks like this area is undefined behavior in CSS2.

The spec says (emphasis added by me):

The border box of a table, a block-level replaced element, or an element in the normal flow that establishes a new block formatting context (such as an element with overflow other than visible) must not overlap the margin box of any floats in the same block formatting context as the element itself. If necessary, implementations should clear the said element by placing it below any preceding floats, but may place it adjacent to such floats if there is sufficient space. They may even make the border box of said element narrower than defined by section 10.3.3. CSS 2 does not define when a UA may put said element next to the float or by how much said element may become narrower.

In this case, we are dealing with "an element in the normal flow which establishes a new block formatting context". Chrome/Safari are placing the element below the preceding float (specifically in this case with a negative margin), whereas we are not; the spec allows UAs to do either. Nonetheless, it seems reasonable that the behavior should be predictable & coherent, and I'm not sure whether it is. I'm putting together some more testcases locally to better-understand what's going on in Firefox as well as in Chrome/Safari.

Making some sense of things here... I'll try to explain my original intuition about why the Chrome/Safari behavior felt obviously-wrong, followed by some more subtlety and why it's actually less-obviously-wrong (i.e. potentially-reasonable).

Firstly, the intuition: negative margins reduce the size of the margin-box, which typically make things fit better and less-prone to linewrapping. This typically leads to overlap, and increases the amount of (overlapping) content that can fit on a line. Consider e.g. this example where "YYY" gets pulled back and overlaps "aaa" a bit:
data:text/html,aaa<div style="display:inline-block;margin-left:-10px">YYY
This is where my intuition was coming from -- essentially, it feels like the negative margins generally make things "fit better" and shift earlier in the page, rather than causing linewrapping or causing content to be pushed further in the page.

BUT, on the other hand: in many situations (cases where the margin-box size is effectively held constant via how width:auto works), negative margins actually just increase the size of the border-box. E.g. in this example:
data:text/html,<div style="border:2px solid black">A</div><div style="border:2px solid red; margin-right: -30px">A</div>
Here, the default width:auto makes each div's margin box fill the container (the body in this case). Since the second div has a negative margin, its border box is forced to be wider than the body, in order for there to be space that its negative margin can steal back away such that the margin-box will match the body's width.

Also, importantly: for the purposes of placing a formatting-context-forming element next to a float (the case in question for this bug's testcases), it's the element's border-box that counts; not the element's margin-box. That's because, per the spec-text quoted in comment 8, it's the element's border-box that is prohibited from overlapping the float. And in order to achieve this, we're allowed to violate the regular block-flow sizing rules, so that e.g. width:auto doesn't necessarily have to stretch our margin-box to fill the container. This rule-breaking behavior is left unspecified, but UAs seem to agree that it involves compressing the element's border-box to fit inside of the container, as shown on the first line here (where the first black box is fully inside the red container, despite having a negative margin-right that makes it seem like it should overflow rightwards like the second one does).
data:text/html,<div style="width:100px; border: 2px solid red"><div style="float:left">f</div><div style="display:flow-root;margin-right:-20px; border:2px solid black">A</div><div style="display:flow-root;margin-right:-20px; border:2px solid black">A</div>


So. That's a lot, but recapping the key points that we've established so far:
(1) For an auto-width block-level element, a negative margin effectively just contributes a positive size to the border-box.
(2) An element's border-box may also get squashed in unspecified but mostly-interoperable ways, if it establishes an independent formatting context (e.g. display:flow-root/flex) and is placed next to a float. (And this essentially involves just compressing its border-box to fit the available space in the container.)

Now: having established those two points, let's consider a scenario where we have a negative margin that "contributes some size" to the border-box [per (1) above] AND the border-box is forced to be compressed [per (2) above], and it would need to be compressed to be smaller than the size that was contributed by the margin. Firefox simply allows this; Chromium/WebKit apparently do not. In this circumstance, Chromium/WebKit seem to treat the negative-margin's contribution to the border-box as being an "uncompressble" piece, and so they simply decline to place the element next to the float. They push it down below the float, where it doesn't have to deal with floats at all.

Note that all browsers including Firefox have this same "uncompressible" behavior for e.g. large values of border and padding -- e.g. if you have the same setup but with lots of padding "propping up" the border-box size (instead of a negative margin), then browsers interoperably push the element down. It's just the negative margin's positive contribution that is handled differently.

One possible encapsulation/explanation of the Chromium/WebKit behavior here: when determining whether a formatting-context-forming element's border-box will fit next to a float vs. whether it needs pushing down (per the spec text quoted in comment 8), we must treat the (positive!) contribution from the border, padding, and any negative-margins as establishing a minimum size of the border-box.

(I'll have this philosophy in mind when writing tests for this, and potentially when writing a patch for this, assuming it does indeed end up remaining as coherent as it seems to be now.)

:dholbert - That's more or less how currently we interpret it - there's a lot more subtleties around this area when Morten & I were writing this logic in LayoutNG. Unfortunately all this is basically unspecified (like most complex cases in flow layout).

When I say "size" below I just mean "border-box size".

The three invariants we have are:

  1. The available_space must be >= zero
  2. The final size of a fragment/element must be >= zero. ("zero" here being border+padding, but lets just pretend that doesn't exist).
  3. size = available_space - margins

In the absence of floats, everything behaves as expected. Applying -ve margins will grow the size of the box, applying +ve margins will shrink.

For positive margins we have great interoperability, this roughly works as follows.

  1. Take the original available-size. (available_size = original_available_size).
  2. Subtract the margins. (available_size -= margin_sum)
  3. Are there any floats intersecting? Yes? Shrink the available-size further to avoid floats. (available_size -= intersecting_floats). The intersecting floats is based on accounting for margins.
    This is where we likely differ internally to you, but it ends up having the same result.
  4. We add back the margins such that when we run our sizing algorithm, we don't end up with a negative size. (available_size += margins).
  5. Size the new formatting context. (size = available_size - margins).

Steps (4) & (5) are likely were we differ slightly, but it doesn't matter for the positive margin case, I suspect that Gecko in the presence of floats will drop margins for steps (4) & (5).

For negative margins everything goes off the rails. We actually ended up matching quite a lot of Gecko's behaviour here (while writing LayoutNG), and differ from WebKit quite substantially.

For us (Blink) we more or less follow the behaviour above. The bit which "explains" our behaviour is that in step (4) above we don't allow the available_size to go below zero. (From invariant (1) above). This means that for the testcases above we pretend that the new formatting-context is sitting within a zero-width containing block, and will be sized accordingly.
Step (5) produces a size > zero, and we decide then it can't fit within that zero width layout area, and move onto the next area.
This is "uncompressible" margin behaviour that you've described above.

Gecko does something a little bit different as far as I can tell. As soon as there are any floats, and any negative margins you stop considering the negative margins completely. For negative margins this "explains" the behaviour. The test for this is:

<!DOCTYPE html>
<div style="border: solid; width: 200px; display: flow-root;">
  <div style="float: left; width: 150px; height: 20px; background: lime;"></div>
  <div style="margin-left: 50px; outline: solid red;">
    <div style="display: flow-root; margin-left: -100px; height: 50px; background: orange; outline: solid orange;"></div>
  </div>
</div>

We consider the size of the new-FC as 100px, Gecko 50px.

WebKit does something different again (partly why we match a lot of your behaviour previously). E.g.

<!DOCTYPE html>
<div style="border: solid; width: 200px; display: flow-root;">
  <div style="float: left; width: 100px; height: 20px; background: lime;"></div>
  <div style="margin-left: 50px; outline: solid red;">
    <div style="display: flow-root; margin-left: -50px; height: 50px; background: orange; outline: solid orange;"></div>
  </div>
</div>

For the above case Blink & Gecko match, but WebKit is different. WebKit behaves (as far as I can tell) in a way that effectively means that if there are any floats, and any negative margins, they'll always move down to avoid floats.
When we were writing our new version (LayoutNG) we liked Gecko's behaviour here a little more in that it is possible to find an "auto" inline-size which satisfies all the constraints, and fixed a few bugs reported on us due to this behaviour.

(Everything here gets even more complicated when you start considering +ve margins on one side, and -ve on the other, e.g.)

<!DOCTYPE html>
<div style="border: solid; width: 200px; display: flow-root;">
  <div style="float: left; width: 150px; height: 20px; background: lime;"></div>
  <div style="margin-left: 50px; outline: solid red;">
    <div style="display: flow-root; margin-left: -100px; margin-right: 50px; height: 50px; background: orange; outline: solid orange;"></div>
  </div>
</div>

Or when you allow expansion outside the available area (which is another topic entirely).

Thanks, Ian! This is super-helpful.

(In reply to Ian Kilpatrick [:iank] from comment #11)

For the above case Blink & Gecko match, but WebKit is different. WebKit behaves (as far as I can tell) in a way that effectively means that if there are any floats, and any negative margins, they'll always move down to avoid floats.

Yeah, this is what I'm seeing locally. (This WebKit always-move-down behavior seems odd & we probably won't match it; so far, the Chromium/LayoutNG behavior seems reasonable-to-match though.)

I'm going to attach a few testcases here for cross-browser testing, which will probably turn into WPT .tentative tests in the patch here.

Browsers agree on this testcase (where the formatting context's border-box size is "propped up" via some padding).

As above, here's a testcase that's currently interoperable, but now with border being the thing to prop up the border-box size.

Attachment #9255972 - Attachment description: testcase 5 (not interoperable): various amounts of negative padding → testcase 5 (not interoperable): various amounts of negative margin

In testcases 3 and 4, all browsers push the flow-root down when the inline-axis border/padding is larger than the available space.

In testcases 5 and 6:

  • Firefox doesn't push any of the flow-roots down.
  • Safari always pushes the flow-root down when there's any negative margin (i.e. it pushes the flow-root down in all of the subtests except for the first, in testcase 5 and 6)
  • Chrome only pushes the flow-root down when the margin-left and margin-right are collectively negative enough to "contribute" more than the available space. (i.e. the bottom three subtests in Testcase 5, and the bottom four subtests in Testcase 6).

:dholbert - are you expecting to land these testcases in wpt, or do we have some other test we can use for the interop-2022 metrics here (c.f. https://github.com/web-platform-tests/interop-2022/issues/9#issuecomment-1008959096 )

Flags: needinfo?(dholbert)

I'm expecting to land these in WPT, yes. I'm not aware of other tests (in WPT or elsewhere) that cover this area of interop pain.

Flags: needinfo?(dholbert)
See Also: → 1750047
See Also: 1750047
Webcompat Priority: --- → ?

This is breaking a lot of sites in bad ways, WebCompat P1 for now.

Webcompat Priority: ? → P1

Why is this marked as a "task"? And, should we consider increasing the severity given the number of affected websites?

Flags: needinfo?(dholbert)
Flags: needinfo?(aethanyc)

(In reply to Marco Castelluccio [:marco] from comment #24)

Why is this marked as a "task"?

That was probably just a mistake. Looks like this was because, as of comment 0, we initially thought this interop issue was a bug in other browsers rather than a bug in Firefox.

And, should we consider increasing the severity given the number of affected websites?

Sure, we could probably consider this an S2. I'm also working on this, so I'll assign it to myself.

Assignee: nobody → dholbert
Severity: S3 → S2
Type: task → defect
Flags: needinfo?(dholbert)
Flags: needinfo?(aethanyc)
Depends on: 1767069

I think the attached patch fixes this bug. It's marked WIP for now because I need to add automated tests.

Some of the tests will be interop2022-worthy -- basically, just the ones that we weren't previously interoperable on but will now be, as of this patch. I think those will cover all of the scenarios that are relevant for the webcompat dupe-bugs.

I'll also include a pile of other tests where we currently lack test coverage (and that's bad), where browsers continue to behave differently, but not in ways that are relevant for the webcompat dupe-bugs. Those tests aren't relevant to interop2022 (or to the webcompat issues duped here).

Also: since we're currently in a soft freeze for behavior-impacting changes, I'll be sure not to land the fix here until after the merge (which is this coming Monday, I think).

Attachment #9274526 - Attachment description: WIP: Bug 1745310 WIP: Account for the impact of negative margins, when determining how much space a float-dodging box will need in order to fit alongside a float. → WIP: Bug 1745310 WIP: Account for the impact of negative margins when determining how much space a float-avoiding box will need in order to fit alongside a float.
Attachment #9274526 - Attachment description: WIP: Bug 1745310 WIP: Account for the impact of negative margins when determining how much space a float-avoiding box will need in order to fit alongside a float. → Bug 1745310 part 1: Account for the impact of negative margins when determining how much space a float-avoiding box will need in order to fit alongside a float. r?emilio

(note-to-self when checking the webcompat issues here: it looks like webcompat.com is having some server issues, but swapping in https://github.com/webcompat/web-bugs/ into the URL gets to the github-hosted version of the bug report.

e.g. https://webcompat.com/issues/102848 is currently error 500, but https://github.com/webcompat/web-bugs/issues/102848 is fine.

Blocks: 1767217
Attachment #9274608 - Attachment description: Bug 1745310 part 2: Add a copy of the WPT test with "fill-available" (and prefixed aliases). r?emilio → Bug 1745310 part 2: Add a copy of the WPT test with "width:stretch" (and prefixed aliases). r?emilio

(In reply to Ian Kilpatrick [:iank] from comment #11)

WebKit behaves (as far as I can tell) in a way that effectively means that if there are any floats, and any negative margins, they'll always move down to avoid floats.

FWIW, I filed https://bugs.webkit.org/show_bug.cgi?id=239976 on this WebKit-specific behavior.

FWIW, I filed https://bugs.webkit.org/show_bug.cgi?id=239976 on this WebKit-specific behavior.

👍

Re: width:stretch behaviour.... yeah we looked at this when we were doing these changes and came to the conclusion that you wrote in the test. E.g. "auto" maps (effectively) to either "fit-content" or "stretch" depending on the context. In this context "auto" maps to "stretch" so there should be any behaviour difference.

so there should be any behaviour difference.

so there shouldn't be any behaviour difference.

Thanks, Ian!

FWIW: I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1322774 on a Chrome-specific bug that I encountered when testing larger positive margin values, which cause the BFC to visibly overlap the float (which seems to pretty directly contradict CSS2's requirements).

The margins in this test aren't more-negative than the available space, so they
don't "pump up" the BFC's border-box enough to force it to wrap to a new line.

Firefox and Chromium agree on this testcase, but WebKit does not, per
https://bugs.webkit.org/show_bug.cgi?id=239976

Given that this is a vaguely-specced area, I'm naming the test with
".tentative" at this point.

Depends on D145197

Specifically, the new copy of the test uses margin values that are larger than
the space that's available after allocating some space for the float's
margin-box and the BFC's border. These trigger wrapping in some cases, but not
always.

Given that this is a vaguely-defined area of the spec and implementations
disagree, I'm writing the reference case to just match Firefox's behavior for
now and I'm naming the test with ".tentative". (Though it's worth noting that
Chrome does have at least one bug that's triggered by this test, which I filed
as https://bugs.chromium.org/p/chromium/issues/detail?id=1322774 ; so at least
in that case, I have some confidence in Firefox's behavior being more-correct.)

Depends on D145530

See Also: → 1768048
Attachment #9275119 - Attachment description: WIP: Bug 1745310 part 4 WIP: Add a copy of the WPT test with large positive margin values. → Bug 1745310 part 4: Add a copy of this bug's WPT test with large positive margin values. r?emilio
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/843d54cba1f6 part 1: Account for the impact of negative margins when determining how much space a float-avoiding box will need in order to fit alongside a float. r=emilio https://hg.mozilla.org/integration/autoland/rev/09c31015a54c part 2: Add a copy of the WPT test with "width:stretch" (and prefixed aliases). r=emilio https://hg.mozilla.org/integration/autoland/rev/d8f56c1bd7e9 part 3: Add a copy of the WPT test with negative margin values that shouldn't trigger wrapping. r=emilio https://hg.mozilla.org/integration/autoland/rev/857ea33dfdb0 part 4: Add a copy of this bug's WPT test with large positive margin values. r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/33959 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot

(going through the associated webcompat reports, and https://webcompat.com/issues/92024 turns out to be unrelated. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1768555 for that one.)

I've confirmed that all of the other "see-also" webcompat.com reports (aside from the one I removed in comment 41) are either fixed by this bug or no-longer-reproducible-due-to-a-site-redesign. I left a note on each one to indicate this.

I filed https://github.com/w3c/csswg-drafts/issues/8142 to get the spec updated to fill in undefined areas to match the test expectations here, FWIW.

See Also: → 1849469
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: