[css-flexbox] overflow clip on SVG elements and flex layout

[khushalsagar]

This issue is related to the change in #7144 which included a change to add overflow: clip to svg elements in UA stylesheet. This causes a compat issue with the following test case:

<style>
  div {
    display: flex;
    width: 100px;
    height: 100px;
  }
  svg {
    flex-grow: 1;
  }
</style>
<div>
  <svg width="150" height="150">
    <rect width="150" height="150" fill="green"></rect>
  </svg>
</div>

SVG has a min-width/min-height value of auto via default value for these properties. Before the resolution in #7144, SVG also had overflow: hidden applied to it via UA stylesheet.

During flex layout, the minimum width computed for this SVG element with these inputs was 0. Theoretically this aligns with the spec based on the text here: "the used value of a main axis automatic minimum size on a flex item that is not a scroll container is its content-based minimum size". overflow: hidden causes the element to be a scroll container. But it's specifically not the case for SVG and neither of Chrome/Firefox/Safari make the element scrollable. The relevant spec text is here.

With the change in #7144, the SVG element now gets a value of 'clip' for 'overflow'. This value lines up with how 'hidden' is used on these elements in each browser above. But a side-effect of this is that min-width/min-height:auto now uses the content-based-minimum-size on this element which is incompatible with existing behaviour. The options to fix this are:

1. Add min-width/min-height: 0 to SVG to retain the old behaviour. But this would be breaking if a developer stylesheet changed the value of 'overflow' to 'visible' which earlier would've used min-width/min-height auto.
2. Carve a special-case for svg where 'clip' behaves same as other non-visible values when computing min-width/min-height to retain the current behaviour.

Leaning towards 2) since 'overflow: visible' is likely to be a common use by developers for SVG. SVG has respected overflow:visible to not clip content before #7144.

@bfgeek FYI.

[bfgeek]

A third option also is to not change the UA-stylesheet for SVGs (they remain overflow:hidden in the UA-stylesheet which is the case today).

[khushalsagar]

Based on the resolution in #7144, hidden on replaced elements (including SVG) should have a used value of clip. See comment here.

Given the above, no change to the UA-stylesheet for SVG should behave the same as adding overflow: clip to the stylesheet since they result in the same used value. I think the safest option is to special-case overflow:clip on SVGs in flex layout to be backwards compatible with the current behaviour.

[css-meeting-bot]

The CSS Working Group just discussed overflow clip on SVG elements and flex layout, and agreed to the following:

• RESOLVED: in the UA style sheet, we keep `svg { overflow: hidden; }`
• RESOLVED: in the flex spec, for auto min sizing, instead of saying scroll container, we read the computed overflow property value
• RESOLVED: overflow:hidden on replaced elements gets coerced to overflow:clip at paint time

The full IRC log of that discussion

<heycam> Topic: overflow clip on SVG elements and flex layout
<heycam> github: https://github.com//issues/7714
<heycam> khush: this concerns how the min width / min-height auto is computed for a flex child
<heycam> ... spec says that if an element is a scroll container, the min-width/height will be 0
<heycam> ... but if not a scroll container, then it's content based min size
<heycam> ... the two cases where it's not a scroll container, is if overflow is clip/hidden
<heycam> ... previous change made replaced elements [...]
<heycam> ... SVG has for a long time had `overflow: hidden` in the UA style sheet
<heycam> ... now we're setting overflow:clip, it causes this layout behavior
<heycam> ... before, min-width/height would be 0
<heycam> ... but now it's computing to the content based min size
<heycam> Ian: we got many bugs
<heycam> khush: fun part is that was accidentally rolled out to stable channel
<heycam> ... have to keep this behavior for compat
<heycam> ... options are to first add min-width/height:0 to the UA style sheet, which when combined with overlfow:clip gives you the preivous behavior
<heycam> ... but now we have different compat problems
<heycam> ... second option is to special case it
<heycam> ... either in the layout computation spot. clip behaves this way except for SVG
<heycam> ... other option is that we don't set the overflow:clip in the UA style sheet only for SVG, and at layout time the previous behavior continues to apply, but at paint time we use it as if it's clip
<heycam> ... it'd have to be a used value time thing
<emilio> q+
<heycam> ... used value would be different in paint time
<heycam> fantasai: maybe should've gone over #7144 first
<heycam> dholbert: I'm fond of the third option, keeping overflow:hidden SVG. is there a reason we need to change SVG?
<heycam> Ian: mainly for consistency
<heycam> ... we didn't perceive the compat risk
<fantasai> +1 to keeping overflow:hidden on svg
<heycam> ... only weird quirk is that at layout time, when we're looking at overflow, pretend overflow means clip at paint time
<heycam> ... at paint time
<heycam> s/layout time/paint time/
<heycam> dholbert: where is that used value magic? already exists?
<heycam> ... seems orthogonal to this
<heycam> fantasai: maybe we should switch to 7144
<Rossen_> ack emilio
<heycam> emilio: I have a question. overflow:clip by default on SVG, it changes the sizing behavior
<heycam> Ian: so min-content sizes
<heycam> ... auto min size will be 0
<heycam> emilio: if I load the test case in the first comment, and set overflow:clip/hidden, I don't see any layout change
<heycam> khush: flex layout was implemented first, so where it's supposed to check is this a scroll container or not, it looks at overflow value
<heycam> ... assumes it's not a scroll container only if overlfow:visible, but we are also now checking for clip
<heycam> ... this is a change in the process of landing
<heycam> emilio: right now SVG is not a scroll container
<heycam> Ian: but it does have overflow:hidden
<heycam> emilio: right now that decision is based on scrollable overflow
<heycam> Ian: computed overflow property value
<heycam> emilio: and you are changing that?
<heycam> ... but you want SVG to keep behaving the same way?
<heycam> Ian: the flexbox auto min size thing likely needs to say look at the computed value of overflow, rather than "scroll container"
<heycam> ... could make that narrower
<heycam> ... it becomes a used value, after layout thing
<heycam> emilio: I'm still confused
<heycam> ... I see a different if I set overflow:visible on the SVG, but not if I set overflow:clip
<heycam> Ian: we currently don't support overflow:clip
<heycam> dholbert: in Firefox I see a difference
<heycam> Ian: but that's not the compat concern
<dholbert> (If I add `overflow:clip` to https://github.com//issues/7714#issue-1366802020 , then the green thing gets wider)
<heycam> ... overflow visible/clip should behave the same
<heycam> ... but we changed the UA style sheet from hidden to clip, and that broke things
<heycam> emilio: maybe not change the UA style sheet?
<heycam> ... since it doesn't create a scroll container either
<heycam> Ian: at layout time you use the computed value of overflow, like we do right now
<heycam> ... to determine if we apply the auto min size
<heycam> ... and at paint time, we convert this to a used value of overflow:cip
<heycam> emilio: but that's basically what happens now?
<heycam> Ian: differences are overflow clip margin will start to apply
<heycam> khush: in terms of the changes in the UA sheet, we can skip setting overflow:clip, but we still need the overflow clip margin box for it to work with these elements
<heycam> khush: overflow-clip: margin-box
<heycam> ... so we'll skip setting overflow:clip, leave it as hidden
<heycam> emilio: proposed solution only changes the UA sheet?
<heycam> emilio: do we need to add magic to make overflow:hidden to behave like overflow:clip for SVG, rather than letting authors do it?
<heycam> Ian: we had a resolution that overflow:hidden would behave as overflow:clip
<heycam> ... it basically behaves like that anyway
<heycam> ... this is harmonizing
<heycam> emilio: bit unfortunatey
<heycam> s/unfortunatey/unfortunate/
<heycam> Ian: the coercion would apply to all the replaced elements, not just SVG
<heycam> ... if they're overflow:hidden
<heycam> emilio: as long as you're a replaced element
<heycam> ... you don't get weird cases where there's only clip on one axis
<heycam> ... as long as this is well tested
<heycam> khush: I promise!
<heycam> dholbert: the thing where you check the computed value for flex layout purposes, I think that already matches what we already do internally, and it's what the spec used to say
<heycam> ... it results to 0 if overflow is hidden/scroll/auto
<heycam> ... but the spec now says scroll container
<heycam> ... so we might want to consider reverting that, and explicitly defer to computed overflow value
<heycam> Ian: I think I'd prefer that
<heycam> ... in summary, part 1: in the UA style sheet, we keep `svg { overflow: hidden; }`. part 2: in the flex spec, for auto min sizing, instead of saying scroll container, we read the computed overflow property value
<heycam> ... part 3: which may already be specced, is overflow:hidden on replaced elements gets coerced to overflow:clip at paint time
<heycam> dholbert: you can't do scrollTop on an <img>
<heycam> khush: part 3 was resolved in #7144
<fantasai> +1
<heycam> khush: one other thing, for SVG, we're still keeping overflow-clip-margin in the UA sheet
<heycam> RESOLVED: in the UA style sheet, we keep `svg { overflow: hidden; }`
<heycam> RESOLVED: in the flex spec, for auto min sizing, instead of saying scroll container, we read the computed overflow property value
<heycam> RESOLVED: overflow:hidden on replaced elements gets coerced to overflow:clip at paint time
<heycam> <br dur="13m">

See official minutes

[dholbert]

Quoting one resolution:

In the flex spec, for auto min sizing, instead of saying scroll container, we read the computed overflow property value

It wasn't explicitly stated, but I think it's implied that we'll need this change for the automatic minimum size for grid items as well.

[emilio]

@fantasai @tabatkins grid also uses scroll container, do we want the same "look at the computed overflow value" instead?

[emilio]

Ah I guess Daniel's comment is exactly my comment too :)

[bfgeek]

@emilio @dholbert - We noticed some new tests came in. It looks like you were asserting that we don't read the computed value of the element in grid like we do in flexbox - is that understanding correct?

(also the tests are kinda asserting unspecified things we believe - like if scrollbars are allowed on buttons etc, but we can fix that in the tests separately).

[dholbert]

It looks like you were asserting that we don't read the computed value of the element in grid like we do in flexbox - is that understanding correct?

Not quite. I think Gecko does treat grid/flex consistently[i] but we do seem to treat them differently from Chromium/WebKit.

Looking at e.g. http://wpt.live/css/css-grid/stretch-grid-item-text-input-overflow.html (one of the recent test changes), I think the idea of the changes was:
(1) overflow:clip and overflow:visible should behave the same, RE impact on automatic minimum size (this part likely isn't controversial)
(2) inherently-scrollable things like <input> shouldn't have their min-size be conditioned on their explicit overflow value (this is the part where engines differ I think)

During code-review, I only noticed change (1) and didn't think too much about (2), but it does seem like (2) is in contradiction to the resolution here and is a divergence from other browsers, so we may need to rethink/revert that part.

[i] RE treating grid/flex consistently, here's an example:

data:text/html,
<style>f { display: flex; width: 1px;}g { display: grid; width: 1px}</style>
<f><input></f>
<f><input style="overflow:auto"></f>
<g><input></g>
<g><input style="overflow:auto"></g>

Firefox Nightly renders all 4 lines the same (no shrinking).
Chrome renders the 2nd and 4th one as being small (shrinking).
So: we're both being consistent between flex (top 2) vs. grid (bottom 2), but the implementations disagree about whether explicit overflow makes an automatic-minimum-size difference here. (And the test's recent changes currently make it expect the Firefox Nightly behavior.)

(also the tests are kinda asserting unspecified things we believe - like if scrollbars are allowed on buttons etc, but we can fix that in the tests separately).

Mmm right, that seems to be https://wpt.fyi/results/css/css-grid/stretch-grid-item-button-overflow.html (which I think is just a button-flavored version of the text-input testcase). I think that one is similarly expecting that overflow has no effect on buttons (in terms of creating scrollbars and also in terms of min-sizing impact), but that's perhaps unjustified on both counts.

[dholbert]

(I posted https://bugzilla.mozilla.org/show_bug.cgi?id=1871412#c7 to discuss next-steps on our end.)

[emilio]

<input> has non-scrollable overflow per spec, see https://html.spec.whatwg.org/#form-controls

[emilio]

The button case might be different / unspecified tho

[emilio]

<input> has non-scrollable overflow per spec, see https://html.spec.whatwg.org/#form-controls

Note that this is a somewhat recent change tho, and in particular note whatwg/html#10025 which is a proposed tweak to that.

[dholbert]

<input> has non-scrollable overflow per spec, see https://html.spec.whatwg.org/#form-controls

Aha, indeed, I misdiagnosed what was going on with the input elements there. (I thought we were checking whether a scroll container was/wasn't being generated by overflow; but we're in fact just checking the computed value like we're supposed to, per the resolution here.)

So I think that means Firefox's rendering of http://wpt.live/css/css-grid/stretch-grid-item-text-input-overflow.html (and my data URI in my previous comment) is correct, because the computed value of overflow is always clip regardless of author-specified styles (via that !important UA stylesheet rule from the HTML spec that emilio linked to).

And it looks like we're roughly in agreement on http://wpt.live/css/css-grid/stretch-grid-item-button-overflow.html, if we disregard the column of overflow:scroll buttons. I'll file a followup bug on doing that or similar.

[dholbert]

I filed bug 1873301 to track the behavioral difference for button scrollability, and I filed bug 1873300 on fixing the WPT to not depend on either of the possible behaviors.