w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.52k stars 673 forks source link

[css-flexbox] Intrinsic main size algorithm for row flexboxes not web compatible #8884

Open davidsgrogan opened 1 year ago

davidsgrogan commented 1 year ago

Blink launched an implementation of the specified intrinsic sizing algorithm on Canary for a few days. It was quite clear that the algorithm for computing the intrinsic main size for single-line row flexboxes is not web compatible. min-content sizes are too big. Not yet sure about max-content sizes.

The existing web needs the container's min-content size to be 100px in the case below. But the specified algorithm gives it a width of 300px[1].

<style>
  .flex {
    display: flex;
    width: min-content; /* [2] */
    border: 2px solid;
  }
  span {
    float:left;
    height: 25px;
    width: 100px;
    background: orange;
  }
</style>

<!-- Container must be 100px wide to be web compatible. New algorithm makes it 300px -->
<div class="flex">
  <!-- flex-basis of this item is 300px. -->
  <!-- min-content contribution is 100px. -->
  <!-- desired flex fraction is negative -->
  <div>
    <span></span>
    <span></span>
    <span></span>
  </div>
  <!-- desired flex fraction is 0 -->
  <div id="some-other-item-where-flex-fraction-is-0"></div>
</div>

[1] chosen flex fraction == 0. So first item's final contribution is 300px + 0*0. Second item's final contribution is 0. Their sum is the min-content width of 300px. [2] No one actually specifies width: min-content. But the container's min-content size percolates up to ancestor flexboxes (used in automatic minimum widths) or table cells (for fit-content sizing). The new larger min-content sizes cause compat problems for such ancestors.

We implemented a variant of the algorithm that is closer to what engines currently ship. In this variant, for containers where chosen flex fraction <= 0, items' final contribution to the container's min-content size is (1) its flex-basis if the item has a 0 flex factor in whichever direction it wants to go (shrink if flex-basis > min-content contribution, grow otherwise); otherwise: (2) its min-content contribution.

let ComputeIntrinsicSizes = (flex_container) => {
  // https://drafts.csswg.org/css-flexbox/#intrinsic-main-sizes
  let [container_min_size, container_max_size, chosen_flex_fraction] = ComputeIntrinsicMainSizesPerSection991();

  // chosen_flex_fraction <=0 means no item is growing from their flex basis
  // to meet their min contribution.
  if (flex_container.IsSingleLineRow() && chosen_flex_fraction <= 0) {
    container_min_size = 0;

    for (item in flex_container.items) {
      // https://drafts.csswg.org/css-flexbox/#intrinsic-item-contributions
      const min_contribution = item.ComputeMinContributionPerSection993();
      const base_size = item.flex_base_size;
      const cant_move = (item.shrink_factor == 0 && base_size > min_contribution) || 
                        (item.grow_factor == 0 && base_size < min_contribution);
      if (cant_move) {
        container_min_size += base_size;
      } else {
        container_min_size += min_contribution;
      }
    }
  }
  return { container_min_size, container_max_size };
}

In local testing, this variant has proven to be significantly more web compatible than what is currently specified, but possibly still insufficiently compatible. We haven't tested it in the wild.

We've also discussed, but haven't experimented much with, another variant even closer to what engines currently ship. In this variant, we don't examine flex fractions at all for min-content computations. Instead, each item's final contribution is (1) flex-basis if the flex-basis is not derived from the item's contents AND the item has a 0 flex factor in whichever direction it wants to go; otherwise: (2) its min-content contribution.

let ComputeIntrinsicSizes = (flex_container) => {
  // https://drafts.csswg.org/css-flexbox/#intrinsic-main-sizes
  let [container_min_size, container_max_size, chosen_flex_fraction] = ComputeIntrinsicMainSizesPerSection991();

  if (flex_container.IsSingleLineRow()) {
    container_min_size = 0;

    for (item in flex_container.items) {
      // https://drafts.csswg.org/css-flexbox/#intrinsic-item-contributions
      const min_contribution = item.ComputeMinContributionPerSection993();
      const base_size = item.flex_base_size;
      const cant_move = (item.shrink_factor == 0 && base_size > min_contribution) ||
                        (item.grow_factor == 0 && base_size < min_contribution);
      if (cant_move && item.basis != "content") {
        container_min_size += base_size;
      } else {
        container_min_size += min_contribution;
      }
    }
  }
  return { container_min_size, container_max_size };
}

Both of these variants break the promise, in some cases, that an item will end up at least as large as its min-content contribution after the flex algorithm runs. But that seems to be necessary given where the web is today. (Also, I omitted clamping by used min/max sizes in the descriptions above, but of course they need to be accounted for.)

@tabatkins @fantasai, you've thought about this algorithm a lot. Do you have ideas for variants that are more principled than our attempts, but that preserve current behavior in the reduced case presented above?

We haven't yet gotten as strong a signal on multiline row or multiline column flexboxes. Investigations ongoing.

bfgeek commented 1 year ago

cc/ @dholbert as FYI

dholbert commented 1 year ago

cc/ @dholbert as FYI

(Thanks! I've been meaning to implement in Gecko as well but haven't found time to do so yet. I'll keep an eye on this & related issues.)

davidsgrogan commented 1 year ago

Status update:

There's an experimental conservative algorithm for calculating intrinsic sizes of row containers in Chrome Canary. It's been enabled at 100% installs for three weeks with virtually no issues.[^1] The goal is to establish a web-compat floor.

We don't examine flex fractions at all for intrinsic size computations. Instead, an item's final contribution is (1) its hypothetical main size IF not derived from the item's contents[^2] AND the item has a 0 flex factor in whichever direction it wants to go; otherwise: (2) same as old algorithm, which is intrinsic contribution from css-sizing-3 (notably NOT the contribution specified in css-flexbox)

This conservative algorithm is an attempt to cause the least possible compat problems. It is the smallest, most conservative change we think we can make. The downside is that it fixes only the most egregious of the old algorithm's shortcomings. If this conservative algorithm has nontrivial compat issues, we are permanently stuck with the existing algorithm. The positive flip side is that if this conservative algorithm IS web compatible, we could then try to derive a less conservative algorithm that addresses more of the old algorithm's shortcomings.

In that vein, we've collected 6 author-submitted examples of the old algorithm's shortcomings at http://wpt.live/css/css-flexbox/intrinsic-size/row-use-cases-001.html. Regressions from our previous attempts to change this algorithm are at http://wpt.live/css/css-flexbox/intrinsic-size/row-compat-001.html. The specified algorithm fixes all the use cases but fails all the compat tests. This conservative algorithm passes all the compat tests but fixes only half of the use cases.

let ComputeIntrinsicSizesOfSingleLineRowContainer = (flex_container) => {
  let container_min_size, container_max_size = container_border_and_padding;

  for (item in flex_container.items) {
    let {min_contribution, max_contribution} = ComputeIntrinsicContributionsPerCSSSizing3(item);

    const cant_move = (item.shrink_factor == 0 && item.flex_base_size > min_contribution) ||
                      (item.grow_factor == 0 && item.flex_base_size < min_contribution);
    if (cant_move && item.basis != "content")
      min_contribution = item.hypothetical_main_size;

    // Analog for max, which is identical to above except s/min/max/g.
    const cant_move = (item.shrink_factor == 0 && item.flex_base_size > max_contribution) ||
                      (item.grow_factor == 0 && item.flex_base_size < max_contribution);
    if (cant_move && item.basis != "content")
      max_contribution = item.hypothetical_main_size;

    container_min_size += min_contribution;
    container_max_size += max_contribution;
  }

  return { container_min_size, container_max_size };
}

[^1]: Caveat: we get many more compat reports when our changes graduate release channels, and this change has to survive 3 more graduations.

[^2]: In other words: if (the specified value of flex-basis is definite) OR (the specified value of flex-basis is auto AND the specified main size is definite).

css-meeting-bot commented 1 year ago

The CSS Working Group just discussed [css-flexbox] Intrinsic main size algorithm for row flexboxes not web compatible, and agreed to the following:

The full IRC log of that discussion <bramus> iank_: currently all the implemrentations have same interop algo for single line row flexboxes
<bramus> … this however does not take a bunch of stuff like flex basis
<bramus> … effort by tab and elika to specify an algo that would be compatible that takes all that suff into account
<bramus> … chrome has been experimenting with this algo
<bramus> … and tried to ship on canary. tldr is that currently spec algo is not web compatible
<bramus> … we added test cases for sites that we know that we broke
<bramus> … impact was very very wide
<bramus> … we broke a lot of stuff, like gmail
<bramus> … we are likely a lot or compat constrained
<bramus> … have a proposed algo, that we have been running in canary and we have not broken anything (yet)
<bramus> … I think this is a floor for us to start
<bramus> TabAtkins: we}ve been avoiding looking at this issue for a while
<bramus> … i need to refresh my brain on why this case gets 300px wide in the spec
<bramus> iank_: keep in mind that is only 1 of the issues that we ran into, there is at least 5 different issues that we run into that are subtly different
<astearns> q?
<bramus> … we tried a variation where we tried to fix a few things when the flex fraction is ??? its not gonna work. we are very compat constrained
<bramus> … we think that we can go for a super relative small incremental fix, that fill fix the ??? case that will fix some flex basis being a definite thing.
<bramus> … roughly it is if the flex base size is greater than the main contribution and it cant shriink then use that. same for growing and same for max contributions.
<bramus> … for other things we are way more compat constrained
<bramus> … we broke everything
<astearns> ack fantasai
<bramus> fantasai: there are two havles to intricinsic size computation. min and max content sizing
<bramus> … these are different in a bunch of cases. did we break both of them?
<bramus> iank_: yeah, both
<bramus> … got reports for both
<bramus> fantasai: the min content stuff we wer etyring to make it there was no overflow within the items. so the ??? was larger than the naive version for party that reason
<bramus> … the max content one we get a size that is bigger but also dont get the max content size if you do the current behavior, which does not seem like it satisifes the use case?
<bramus> iank_: need to check, but we also broke the max content size. people have relied on current behavior because we have good interop. wet hink we can get away with respecting flex-basis as described bc that is very broken
<bramus> … is running on canary, so we might identify issues
<bramus> fantasai: seems unfortunate, bc we cant size a flexbox to fit its content
<bramus> … like you get a random size
<bramus> astearns: proposed change is to revert the algo?
<bramus> fantasai: this was original algo that was never implemented
<bramus> iank_: we tried to implement to see if its compatible
<bramus> astearns: how did we manage to get to interop?
<bramus> fantasai: were doing a much more naive implementation. if you take a flex-grow that wraps, then it would just pretend there is 1 row and next content would be widest item which is not the case.
<bramus> … that kind of thing were a simple approach was taken ???
<bramus> … people are therefore relying on werid behavior
<bramus> iank_: i think we can fix flex-wrap column case
<bramus> fantasai: the hardest one
<bramus> iank_: like col wrapping flexbox boxes are mininal while the nhnmber of row flexboxes is hight
<bramus> fantasai: I have not dug into this issue, but wonder if we at some point might need a switch sizes properly
<bramus> iank_: I think we can get a long way there. ???. it would be nice, but i am not hearing from devs about the brokenness, as opposed to the column wrap case
<bramus> … the row case is “sure, whatever”. dont know how much value there is in a switch for devs.
<bramus> … we would have had a lot more reports about it
<bramus> astearns: so, waht do we want to do?
<bramus> fantasai: I think tab and I are gonna need to do a follow up and figure ou twhat spec changes are needed and also what comes back from web compat
<bramus> iank_: having a quick look at definitvie flex basis case is pretty simple. we are 90% sure that we can do this change.
<bramus> … it seems like a col wrap case and not a lot people set a flex basis
<bramus> … if we get a resolution about path forward with flex basis algo then we can roll forward. wont push to beta channel without out
<bramus> fantasai: seems like step in right direction. we should take resolution to support going in this direction
<bramus> TabAtkins: yeah
<bramus> iank_: yeah, we think this is a flaw and that we can ship this
<bramus> … incremental changes
<bramus> … i think that is path forward
<bramus> fantasai: from spec perspective we ???
<bramus> astearns: take the algo that is in the spec that turned out to be problematic from a compat concern, leave it in the spec as informative note with explanation about it being a note and then add this 1 change so that blink can try shipping
<bramus> fantasai: it is creating an algo for … we have to draft the algo + blink change
<bramus> … it is adding a new algo
<bramus> … i agree on keeping it as informative note
<bramus> … with mention that we ar egoing to tweak it more
<bramus> iank_: at the very least, also writing down what browsers are currently doing
<bramus> … and the incremental thing
<bramus> astearns: so first resolution is to take existing algo, mark it as informative, and explain why that is.
<bramus> astearns: objections?
<bramus> RESOLVED: take existing algo, mark it as informative, and explain why that is.
<bramus> fantasai: second bit is to tentatively spec what chrome has implemented, and mark it as tentative
<bramus> iank_: also tweak it if there is feedback
<bramus> astearns: objections?
<bramus> astearns: with intent to improved it as much as we can
<bramus> astearns: proposed is to tentatively spec what is interoperable for this area of flexbox sizing, and mark it as tentative
<bramus> fantasai: I’d rather spec what chrome is doing
<bramus> astearns: other browsers any concerns about this?
<bramus> dholbert: seems fine to me
<TabAtkins> sammy_gill ?
<bramus> sammy_gill: double check that is the conservative and web compatible version
<bramus> iank_: yes, web compatible up to canary
<bramus> … we might find out that we broke stuff when pushing to beta
<bramus> astearns: and you said you found 6 issues?
<bramus> iank_: yes, we’ve had cases that broke large sites
<bramus> RESOLVED: spec what chrome has implemented in canary currently
<bramus> TabAtkins: it is in the thread
<bramus> astearns: (missed)