Closed Bug 1304636 Opened 8 years ago Closed 8 years ago

[css-grid][css-flexbox] A {flex,grid} item's min-{width,height} "auto" value should be reported as "auto" in getComputedStyle()

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bmo, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webcompat])

Attachments

(5 files)

testcase
461 bytes, text/html
Details
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304636 Part 4: Rename test_flexbox_min_size_auto.html to a more inclusive name.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304636 Part 2: Make GetComputedStyle report 'auto' for min-height:auto flex/grid items.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1304636 Part 3: Update the existing test_flexbox_min_size_auto.html test to match new behavior.
58 bytes, text/x-review-board-request
dholbert
: review+
jcristau
: approval-mozilla-aurora+
Details 
A follow up from bug 1292447 comment 48. Based on flex-box spec[1], resolved value for 'auto' on min-{width|height} should be 'auto' instead of '0px'.

[1] https://drafts.csswg.org/css-flexbox-1/#valdef-min-width-auto
Assignee: nobody → aschen
Status: NEW → ASSIGNED
Depends on: 1292447
Priority: -- → P3
Is the same is true for Grid?
If so, please make sure the fix includes grid items too.  Thanks!
(In reply to Mats Palmgren (:mats) from comment #1)
> Is the same is true for Grid?
> If so, please make sure the fix includes grid items too.  Thanks!

Based on spec[1], yes, true for grid item.
Thanks for pointing it out, I'll include the fix to grid too.

[1] https://www.w3.org/TR/css-grid-1/#min-size-auto
Summary: min-{width,height} prop on flex items with auto value should be resolved to auto instead of 0px → A {flex,grid} item whose min-{width,height} auto value should be resolved to auto instead of abs length
Actually it seems to me we need to ensure min-{width,height}: auto is computed to 0 if it is not a {flex,grid} item, and then just serialize whatever presents in the style struct for getComputedStyle.
Hmm, I wonder if this change would be web compatible for flexbox.
I checked Chrome - they also return 0px for both flex/grid.
Have they committed to changing it to 'auto' ?
Is any other UA returning 'auto' ?
Attached file testcase — 
It seems this is web-compatible. Blink does return "auto" for flex item. We currently return -moz-min-content for that, which looks wrong anyway, but fixing that may not be trivial.
My bad, I checked the grid/flex container!  Oops.
OK, good to know Chrome has already made the change.
Summary: A {flex,grid} item whose min-{width,height} auto value should be resolved to auto instead of abs length → A {flex,grid} item's min-{width,height} "auto" value should be reported as "auto" in getComputedStyle()
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5)
> It seems this is web-compatible. Blink does return "auto" for flex item. We
> currently return -moz-min-content for that, which looks wrong anyway, but
> fixing that may not be trivial.

I think it should be reasonably trivial (and should only require changes in nsComputedDOMStyle.cpp).  We represent these values with eStyleUnit_Auto, which we then report as either "0" or "-moz-min-min-content" in nsComputedDOMStyle::DoGetMinHeight() and nsComputedDOMStyle::DoGetMinWidth()  (because an older version of the spec actually required "auto" to *compute* to min-content).
https://hg.mozilla.org/mozilla-central/annotate/29beaebdfaccbdaeb4c1ee5a43a9795ab015ef49/layout/style/nsComputedDOMStyle.cpp#l4957

To fix this, we just want to report "auto" in that case -- so we should be conditionally setting minWidth to 0, and otherwise leaving it at eStyleUnit_Auto.

One note: for flex items, "auto" only has special significance in the flex container's *main axis* (so e.g. "min-width:auto" should only resolve to "auto" if we're in a horizontal flexbox. If we're in a vertical flexbox, min-width:auto should resolve to 0).  The code already does that -- I'm just mentioning it here to explain those IsHorizontal() checks.
Blocks: css-grid
Summary: A {flex,grid} item's min-{width,height} "auto" value should be reported as "auto" in getComputedStyle() → [css-grid][css-flexbox] A {flex,grid} item's min-{width,height} "auto" value should be reported as "auto" in getComputedStyle()
Whiteboard: [webcompat]
Astley, do you have an ETA for fixing this?
We should probably try to fix this soon-ish if there are webcompat issues
starting to appear...
Flags: needinfo?(aschen)
I will try to fix it on FF52. Thanks for your notice.
Flags: needinfo?(aschen)
Priority: P3 → P2
I'll take a run at this.
Assignee: aschen → bwerth
Attachment #8811408 - Flags: review?(xidorn+moz)
Attachment #8811409 - Flags: review?(xidorn+moz)
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review93688

::: layout/style/nsComputedDOMStyle.cpp:4909
(Diff revision 1)
> -    // "min-width: auto" means "0", unless we're a flex item in a horizontal
> -    // flex container, in which case it means "min-content"
> +    // "min-width: auto" remains "auto", unless we're a flex item in a vertical
> +    // flex container, in which case it becomes "0"

This looks wrong to me.

It seems to me the spec says the value should be computed to "0" unless it is a flex item whose 'overflow' in the main axis is 'visible'. [1] And this seems to apply on grid item as well. [2]

[1] https://drafts.csswg.org/css-flexbox-1/#valdef-min-width-auto
[2] https://drafts.csswg.org/css-grid/#min-size-auto

::: layout/style/nsComputedDOMStyle.cpp:4911
(Diff revision 1)
>  
>    if (eStyleUnit_Auto == minWidth.GetUnit()) {
> -    // "min-width: auto" means "0", unless we're a flex item in a horizontal
> -    // flex container, in which case it means "min-content"
> +    // "min-width: auto" remains "auto", unless we're a flex item in a vertical
> +    // flex container, in which case it becomes "0"
> -    minWidth.SetCoordValue(0);
>      if (mOuterFrame && mOuterFrame->IsFlexItem()) {

From the description in the spec, it seems to me this is a purely style-based computation which frame probably shouldn't involve.

I think you can compute this based on the style context and its ancestors.

Also, I think dholbert would be a more suitable reviewer for this patch, since he's more familiar with flexbox.
Attachment #8811408 - Flags: review?(xidorn+moz) → review-
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review93692

Also, since we no longer need to compute auto to min-content, I think we can fix min-height at the same time.
Comment on attachment 8811409 [details]
Bug 1304636 Part 4: Rename test_flexbox_min_size_auto.html to a more inclusive name.

https://reviewboard.mozilla.org/r/93514/#review93694

Since I think your part 1 is wrong, I suppose this test is also wrong.

Other than that, I suggest you add this test as a web-platform js test (in /testing/web-platform), rather than a mochitest in the style system, so that other browsers can see this test as well.
Attachment #8811409 - Flags: review?(xidorn+moz) → review-
For the test, you probably just want to modify the existing test test_flexbox_min_size_auto.html (which I suspect would otherwise fail after this bug is fixed), rather than adding a new test.

(Ideally that test should be reshaped into a web platform test as well, but that's not absolutely necessary -- simply updating the existing test would be sufficient.)
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review93688

> This looks wrong to me.
> 
> It seems to me the spec says the value should be computed to "0" unless it is a flex item whose 'overflow' in the main axis is 'visible'. [1] And this seems to apply on grid item as well. [2]
> 
> [1] https://drafts.csswg.org/css-flexbox-1/#valdef-min-width-auto
> [2] https://drafts.csswg.org/css-grid/#min-size-auto

Agreed. The new patch version implements behavior that matches observed behavior in Chrome 54.

I'll let dholbert comment on if this is the right place to do this mapping.
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review93970

::: layout/style/nsComputedDOMStyle.cpp:4913
(Diff revision 2)
> +    // FIXME: This behavior ignores nuance in the specs
> +    // https://drafts.csswg.org/css-flexbox-1/#valdef-min-width-auto and
> +    // https://drafts.csswg.org/css-grid/#min-size-auto, which indicate that
> +    // flex items and grid items without visible overflow should also
> +    // become "0px".
> +
> +    // We implement this simplified form for web compatibility reasons.

This note about "web compatibility reasons" sounds like it's saying we tried the more complex (spec-compliant) solution, and it broke websites.

But I don't think that's the case? Based on comment 21, I think you just mean "compatibility with what Chrome happens to do".

If that's the case, let's just fix this correctly (and address your FIXME).  It's possible (though hopefully unlikely) that there may be actual web-compat reasons that we need to do the simpler thing here -- but we shouldn't presume that up-front, and we should probably file a Chrome bug to be sure they're aware that they're not matching the spec on this.

It should be pretty easy to address this -- I think you can just check whether "StyleDisplay()->mOverflowX !=  NS_STYLE_OVERFLOW_VISIBLE".

(And "StyleDisplay()->mOverflowY for the min-width version. Technically you could check mOverflowX in both cases, because if either "overflow" subproperty is non-"visible" then it forces the other one to be non-"visible" too.  But since we have clear axis mappings here, we might as well use the axis-specific "overflow" property, for symmetry.)

::: layout/style/nsComputedDOMStyle.cpp:4928
(Diff revision 2)
> +      if (containerFrame && (
> +        containerFrame->GetType() == nsGkAtoms::flexContainerFrame ||
> +        containerFrame->GetType() == nsGkAtoms::gridContainerFrame
> +      )) {

Three things:

(1) Use containerFrame->IsFlexOrGridContainer() here (Side note: GetType() is a virtual function call, so it's semi-expensive -- so if you're checking its result against multiple things, you want to store its result in a local variable rather than calling it repeatedly.  Fortunately, IsFlexOrGridContainer() will do that for you.)

(2) Please group parens on the same line as the condition that they're wrapping, to match Mozilla coding-style conventions.

So this should be:
if (containerFrame &&
    IsFlexOrGridContainer->IsFlexOrGridContainer()) {

(3) And then (per above) we should check "StyleDisplay()->mOverflowX !=  NS_STYLE_OVERFLOW_VISIBLE" inside of that, before we call SetAutoValue().
Attachment #8811408 - Flags: review?(dholbert) → review-
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review93970

> This note about "web compatibility reasons" sounds like it's saying we tried the more complex (spec-compliant) solution, and it broke websites.
> 
> But I don't think that's the case? Based on comment 21, I think you just mean "compatibility with what Chrome happens to do".
> 
> If that's the case, let's just fix this correctly (and address your FIXME).  It's possible (though hopefully unlikely) that there may be actual web-compat reasons that we need to do the simpler thing here -- but we shouldn't presume that up-front, and we should probably file a Chrome bug to be sure they're aware that they're not matching the spec on this.
> 
> It should be pretty easy to address this -- I think you can just check whether "StyleDisplay()->mOverflowX !=  NS_STYLE_OVERFLOW_VISIBLE".
> 
> (And "StyleDisplay()->mOverflowY for the min-width version. Technically you could check mOverflowX in both cases, because if either "overflow" subproperty is non-"visible" then it forces the other one to be non-"visible" too.  But since we have clear axis mappings here, we might as well use the axis-specific "overflow" property, for symmetry.)

Sorry, I used "that's the case" to mean opposite things back-to-back here. :) Let me clarify/translate:

> [...] and it broke websites.
> But I don't think that's the case

"But I don't think we actually tried the more spec-compliant thing & found that it broke websites

> I think you just mean "compatibility with what Chrome happens to do".
> If that's the case,

"If you were indeed just talking about Chrome compatibility,"
Comment on attachment 8811908 [details]
Bug 1304636 Part 2: Make GetComputedStyle report 'auto' for min-height:auto flex/grid items.

https://reviewboard.mozilla.org/r/93806/#review93984

r- on part 2 for the same reasons as part 1 (since they're basically the same)
Attachment #8811908 - Flags: review?(dholbert) → review-
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review93970

> Three things:
> 
> (1) Use containerFrame->IsFlexOrGridContainer() here (Side note: GetType() is a virtual function call, so it's semi-expensive -- so if you're checking its result against multiple things, you want to store its result in a local variable rather than calling it repeatedly.  Fortunately, IsFlexOrGridContainer() will do that for you.)
> 
> (2) Please group parens on the same line as the condition that they're wrapping, to match Mozilla coding-style conventions.
> 
> So this should be:
> if (containerFrame &&
>     IsFlexOrGridContainer->IsFlexOrGridContainer()) {
> 
> (3) And then (per above) we should check "StyleDisplay()->mOverflowX !=  NS_STYLE_OVERFLOW_VISIBLE" inside of that, before we call SetAutoValue().

I think it should use style data, not frame data. We don't want to flush layout for getting computed value of min-{width,height}, and without flushing layout, relying on frame data could lead to incorrect result.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> I think it should use style data, not frame data. We don't want to flush
> layout for getting computed value of min-{width,height}, and without
> flushing layout, relying on frame data could lead to incorrect result.

Per IRC, all we really require here is that we've flushed frame construction -- and we can assume that's already happened here. So this should still be fine.
Comment on attachment 8811409 [details]
Bug 1304636 Part 4: Rename test_flexbox_min_size_auto.html to a more inclusive name.

https://reviewboard.mozilla.org/r/93514/#review94024

::: layout/style/test/test_computed_style_min_width.html:4
(Diff revision 2)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for min-width:auto computed style issues</title>

As I noted in comment 16, I think you want to extend/improve the existing test, rather than adding this new test.

Otherwise, we'll have two tests basically testing the same thing, with some overlapping testing (e.g. behavior of "min-width:auto" on horizontal flex items) and with some non-overlapping testing (e.g. your test covers grid while the existing one doesn't; and the existing one covers "min-height:auto"  while this one doesn't).

So let's just rename the existing test to drop "flexbox" from its name, and update its comments/checks to be correct (as you've already partly done) and add some grid code to it.  Then you'll be more robustly testing your "part 2" patch (on min-height:auto), which this test otherwise doesn't exercise at all.

::: layout/style/test/test_computed_style_min_width.html:36
(Diff revision 2)
> +  let minWidthAuto = ["flexitem", "flexitem-OH", "flexitem-vertical", "flexitem-vertical-OH", "griditem", "griditem-OH"];
> +  for (let id of minWidthAuto) {
> +    testIsMinWidth(id, "auto");

Two issues with this array:
(1) I think the "OH" elements here (overflow:hidden) really belong in "minWidthZero" array, NOT in the "minWidthAuto" array, right? (from a spec correctness perspective)  It's a bit confusing to just have them up here with no explanation.

You probably should've included a comment explaining (or at least hinting at) the situation around these values.

Fortunately, if you address my notes on part 1, we'll get these right and resolve them to 0 after all, so you should be able to check that these OH elements resolve to 0 instead of auto after you've fixed that.

(2) "flexitem-vertical" should have "min-height:auto", NOT "min-width:auto".  (It should have min-width:0.)  That's indicative of a bug in part 1 & part 2.  We should only be resolving the min-size as "auto" in the flex container's main axis, and the other axis's min-size should resolve "auto" to 0.
Attachment #8811409 - Flags: review?(dholbert) → review-
Comment on attachment 8811909 [details]
Bug 1304636 Part 3: Update the existing test_flexbox_min_size_auto.html test to match new behavior.

https://reviewboard.mozilla.org/r/93808/#review93992

r- since I think your new test code needs to be merged into here, per above, so that we don't have two separate "min-width:auto" mochitests. A few other notes on updates that we need here:

::: layout/style/test/test_flexbox_min_size_auto.html:35
(Diff revision 1)
>   * Quoting that chunk of spec:
>   *     # auto
>   *     #   When used as the value of a flex item's min main size property,
>   *     #   this keyword indicates a minimum of the min-content size, to
>   *     #   help ensure that the item is large enough to fit its contents.
>   *     #
>   *     #   | It is intended that this will compute to the 'min-content'

This whole comment wants to be mostly-removed, as part of your updates to this test.

Right now it still has a bunch of stuff about "min-content" which is becoming stale, as of this bug's changes.  (The spec quotes are clearly already stale, but it's otherwise up-to-date RE our code & the checks in this test, until the changes that you're making in this bug.)

::: layout/style/test/test_flexbox_min_size_auto.html:79
(Diff revision 1)
>  // (There's an optional final argument, to specify a "todo" expected value for
>  // the min-height, for cases when we *should* have a particular value, but we
>  // don't support it yet. In that case, aExpectedMinHeight is the value we
>  // currently expect to have, and aExpectedMinHeightTodo is the value we really
>  // *should* have.)
>  function checkElemMinSizes(aElemId,
>                             aExpectedMinWidth,
>                             aExpectedMinHeight,
>                             aExpectedMinHeightTodo /* optional */)

This aExpectedMinHeightTodo arg is no longer used, as of this patch -- you're removing the last usage (which had to do with checking for "min-height: -moz-min-content").


So: please remove that arg and its associated "special bonus check" from this function.
Attachment #8811909 - Flags: review?(dholbert) → review-
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review94046

::: layout/style/nsComputedDOMStyle.cpp:4928
(Diff revision 2)
> +      if (containerFrame && (
> +        containerFrame->GetType() == nsGkAtoms::flexContainerFrame ||
> +        containerFrame->GetType() == nsGkAtoms::gridContainerFrame
> +      )) {

UPDATE: Don't actually use IsFlexOrGridContainer() here after all, because in the flex-container case, we need to *also* check whether the flex container is horizontal.

SO: I think you should really factor this logic out into a static helper function, called something like
static bool
ShouldHonorMinSizeAutoInAxis(nsIFrame* aFrame,
                             PhysicalAxis aAxis)
{
 ....
}

And then, the logic here in DoGetMinWidth could just be reduced to:

  if (eStyleUnit_Auto == minWidth.GetUnit() &&
      !ShouldHonorMinSizeAutoInAxis(mOuterFrame, eAxisHorizontal)) {
    minWidth.SetCoordValue(0)
  }

The internals of ShouldHonorMinSizeAutoInAxis(nsIFrame* aFrame) would need to:
 - immediately return false if aFrame or its parent is null (i.e. we only preserve "auto" if we can definitively determine that aFrame is a flex/grid item)
 - then, store aFrame's parent's GetType() in a local variable
 - If its parent's GetType() is nsGkAtoms::gridContainerFrame, return true
 - If its parent's GetType() is nsGkAtoms::nsFlexContainerFrame **and* static_cast<nsFlexContainerFrame*>(parent)->IsHorizontal()'s truthiness corresponds to the passed-in axis's horizontalness, return true
 - Otherwise, return false.
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review94270

This is nearly there!

First, a commit message nit:
>  Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto items.

Please replace "items" with "flex/grid items" here, to make the scope of this a bit clearer. (Of course the logic is a bit more subtle than that, but we don't need to get into that subtlety in the commit message.)

::: layout/style/nsComputedDOMStyle.cpp:4886
(Diff revision 3)
>  }
>  
> +bool
> +nsComputedDOMStyle::ShouldHonorMinSizeAutoInAxis(PhysicalAxis aAxis)
> +{
> +  // Bug 1304636 - A {flex,grid} item's min-{width,height} "auto" value

Let's drop the bug number reference here, because:

 (1) Your comment explains this well enough already - no need to direct readers to a bug (and anyone who really *wants* to learn more could easily discover this bug number via hg blame for this line).

 (2) In our source code, "Bug NNN" code-comments are mostly used as references for *open issues*, i.e. things that are known-broken/incomplete, which will be fixed in Bug NNN.  (And then we can use tools to periodically discover code/code-comments that need updating, by searching for bug references which refer to bugs which are actually closed.  And if you add an unnecessary Bug NNN reference for a closed bug here, then that'll add some noise to that process.)

::: layout/style/nsComputedDOMStyle.cpp:4890
(Diff revision 3)
> +{
> +  // Bug 1304636 - A {flex,grid} item's min-{width,height} "auto" value
> +  // should be reported as "auto" in getComputedStyle().
> +  // https://drafts.csswg.org/css-flexbox-1/#valdef-min-width-auto
> +  // https://drafts.csswg.org/css-grid/#min-size-auto
> +  // In most cases, "min-width: auto" is mapped to "0px", unless we're a

s/min-width:auto/min-{width|height}:auto/

::: layout/style/nsComputedDOMStyle.cpp:4891
(Diff revision 3)
> +  // Bug 1304636 - A {flex,grid} item's min-{width,height} "auto" value
> +  // should be reported as "auto" in getComputedStyle().
> +  // https://drafts.csswg.org/css-flexbox-1/#valdef-min-width-auto
> +  // https://drafts.csswg.org/css-grid/#min-size-auto
> +  // In most cases, "min-width: auto" is mapped to "0px", unless we're a
> +  // flex item or a grid item with visible overflow in the horizontal axis,

This needs a bit of rewording:

Please delete...
"unless we're a flex item or a grid item with visible overflow in the horizontal axis"

...and replace with this clarified version:
"unless we're a flex item (and the min-size is in the flex container's main axis), or we're a grid item, AND we also have overflow:visible".

(The phrase "with visible overflow" sounded like it means "with content that is visibly overflowing".  And you don't need to bother mentioning the overflow axis, since it's irrelevant.  And "horizontal" is too specific here anway, since this function is used for both axes.)

::: layout/style/nsComputedDOMStyle.cpp:4898
(Diff revision 3)
> +      if (containerType == nsGkAtoms::flexContainerFrame &&
> +          (static_cast<nsFlexContainerFrame*>(containerFrame)->IsHorizontal() ==
> +           (aAxis == eAxisHorizontal)) &&
> +          (aAxis == eAxisHorizontal ?
> +            StyleDisplay()->mOverflowX == NS_STYLE_OVERFLOW_VISIBLE :
> +            StyleDisplay()->mOverflowY == NS_STYLE_OVERFLOW_VISIBLE)) {
> +        return true;
> +      } else if (containerType == nsGkAtoms::gridContainerFrame &&
> +                 (aAxis == eAxisHorizontal ?
> +                   StyleDisplay()->mOverflowX == NS_STYLE_OVERFLOW_VISIBLE :
> +                   StyleDisplay()->mOverflowY == NS_STYLE_OVERFLOW_VISIBLE)) {
> +        return true;
> +      }

Three nits on this section:
 (1) Let's just unconditionally check mOverflowX here. No need to have logic to differentiate between mOverflowX vs mOverflowY, because if one is non-"visible", the other is necessarily non-"visible" as well.  (My note in comment 22 about using mOverflowX vs mOverflowY was back when I was thinking we'd have this logic duplicated in DoGetMinWidth / DoGetMinHeight. In that case, we might as well use the axis-specific one -- but it doesn't really matter.)

 (2) Let's restructure the logic so that we're only checking "overflow" in one place.  Maybe just fold it all into one larger "if" check (which won't be too large, now that we don't need to select between mOverflowX/mOverflowY).

 (3) Gecko coding style prohibits "else" after return, since it's unnecessary.  So the "else if" should just be replaced with "if" here -- though it's probably going away anyway, as part of the consolidation suggested in (2) here.
Attachment #8811408 - Flags: review?(dholbert) → review-
Comment on attachment 8811908 [details]
Bug 1304636 Part 2: Make GetComputedStyle report 'auto' for min-height:auto flex/grid items.

https://reviewboard.mozilla.org/r/93806/#review94280

r=me, with one commit message nit. Commit message is currently:
> Bug 1304636 Part 2: Similarly handle the min-height case.

The message needs to be a bit clearer -- it doesn't make sense or really explain what it's doing, if you view this commit in isolation (as people often do).

So: let's just copy the commit message from part 1*, but with "min-height" instead of "min-width".

* (with "flex/grid items" instead of just "items", per my previous comment)
Attachment #8811908 - Flags: review?(dholbert) → review+
Comment on attachment 8811909 [details]
Bug 1304636 Part 3: Update the existing test_flexbox_min_size_auto.html test to match new behavior.

https://reviewboard.mozilla.org/r/93808/#review94284

This test-update looks great -- thanks!

Just one nit: s/Part 4/Part 3/ in the commit message (since you removed the old part 3, which means right now this patch-stack is jumping straight from Part 2 to Part 4, which is a bit confusing. :)

r=me with that.
Attachment #8811909 - Flags: review?(dholbert) → review+
Comment on attachment 8811409 [details]
Bug 1304636 Part 4: Rename test_flexbox_min_size_auto.html to a more inclusive name.

https://reviewboard.mozilla.org/r/93514/#review94286

Similarly, please adjust s/Part 5/Part 4/ on this one's commit message.

Thanks! r=me with that
Attachment #8811409 - Flags: review?(dholbert) → review-
Comment on attachment 8811408 [details]
Bug 1304636 Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items.

https://reviewboard.mozilla.org/r/93512/#review94314

Looks good!

r=me with two more comment tweaks. (Sorry for all the editorial nits -- this behavior is complex, so I want to be sure we have it described/explained very clearly.)

::: layout/style/nsComputedDOMStyle.cpp:4886
(Diff revision 5)
> +  // A {flex,grid} item's min-{width|height} "auto" value should be reported
> +  // as "auto" in getComputedStyle().

This first sentence isn't strictly true (since there's more nuance here), and it also seems a bit self-evident (a given keyword gets reported as that same keyword -- big deal! :))

Perhaps adjust like so:
 s/should be reported as "auto"/gets special treatment/

(and then your prose further down goes into what "special treatment" means)

::: layout/style/nsComputedDOMStyle.cpp:4894
(Diff revision 5)
> +  // To simplify the logic of checking overflow:visible, we test arbitrarily
> +  // for overflow on the X axis. This is possible because overflow:visible
> +  // affects both axes together.

This comment isn't quite explaining this correctly. Please replace with something like:

  // Note: We only need to bother checking one "overflow" subproperty for
  // "visible", because a non-"visible" value in either axis would force the
  // other axis to also be non-"visible" as well.


(Your current comment is technically correct in saying that "overflow" affects both values, but that's simply because "overflow" is a shorthand -- and that doesn't explain why we can get away with only checking one property, because authors could set each axis's property independently.  But fortunately "overflow" has special computation behavior (implemented in nsRuleNode.cpp IIRC) which prevents authors from setting only one subproperty to "visible". If you're curious about this, see the spec text for the "Computed Value" column at https://drafts.csswg.org/css-overflow-3/#property-index )
Attachment #8811408 - Flags: review?(dholbert) → review+
Comment on attachment 8811409 [details]
Bug 1304636 Part 4: Rename test_flexbox_min_size_auto.html to a more inclusive name.

https://reviewboard.mozilla.org/r/93514/#review94322

r=me on the file-rename (which I meant to set to r+ in comment 41, but I apparently ticked r- instead. Sorry about that!)
Attachment #8811409 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd077aefefd6
Part 1: Make GetComputedStyle report 'auto' for min-width:auto flex/grid items. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f3d10f92681e
Part 2: Make GetComputedStyle report 'auto' for min-height:auto flex/grid items. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/8b255fe6c5f6
Part 3: Update the existing test_flexbox_min_size_auto.html test to match new behavior. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/0ca927af7f3d
Part 4: Rename test_flexbox_min_size_auto.html to a more inclusive name. r=dholbert
Keywords: checkin-needed
Comment on attachment 8811909 [details]
Bug 1304636 Part 3: Update the existing test_flexbox_min_size_auto.html test to match new behavior.

[Feature/regressing bug #]: -
[User impact if declined]: web-compat risk
[Describe test coverage new/current, TreeHerder]: includes regression tests
[Risks and why]: low risk
[String/UUID change made/needed]: none

It would be nice to have this fix for the CSS Grid release, and also to fix
web-compat problems for Flexbox, so please uplift this to Aurora 52, thanks.
Attachment #8811909 - Flags: approval-mozilla-aurora?
Comment on attachment 8811909 [details]
Bug 1304636 Part 3: Update the existing test_flexbox_min_size_auto.html test to match new behavior.

css-grid fixes for aurora52
Attachment #8811909 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Per CSS WG resolved that we should just preserve "auto" for both min-{width,height}.

See: https://github.com/w3c/csswg-drafts/issues/2230 "Computing min-width/min-height: auto to zero #2230" to compute auto to auto.
I filed bug 1434658 to adapt to the CSSWG resolution that was noted in comment 61.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: