[css-grid] Fix behavior of flexible track breadths

[css-grid] Fix behavior of flexible track breadths

This patch is fixing 2 issues that are interrelated:

* Flex sizes are invalid on the min slot of minmax().

  The syntax has been recently updated on the spec:
    <track-size> =
      <track-breadth> |
      minmax( <inflexible-breadth> , <track-breadth> )

  To fix this, the patch modifies consumeGridBreadth()
  in CSSPropertyParser, adding a new type of restriction
  "InflexibleSizeOnly".

* Flex sizes outside minmax() behave as auto minimum.

  Flex sizes outside minmax() were previously behaving like
  minimum and maximum (e.g. 1fr => minmax(1fr, 1fr)).
  However the spec changed and now this would be invalid,
  so they should behave like auto minimum (e.g. minmax(auto, 1fr)).

  To achieve this the patch adds a new condition in
  LayoutGrid::gridTrackSize();

The patch includes new test cases checking specifically these 2 issues.
In addition several tests results have been updated to reflect
the new behavior. Also, some cases that are now invalid and
were not testing anything new have been removed.

BUG=611407, 611410
TEST=fast/css-grid-layout/flex-content-resolution-columns.html
TEST=fast/css-grid-layout/flex-content-resolution-rows.html
TEST=fast/css-grid-layout/grid-columns-rows-get-set.html

Committed: https://crrev.com/5ab36ed9abb8f19c79d7c9bd09ca02460aad50c2
Cr-Commit-Position: refs/heads/master@{#393846}

[Manuel Rego, 2016-05-13 14:30:53]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, rob.buis@samsung.com, svillar@igalia.com,
timloh@chromium.org

[cbiesinger, 2016-05-13 21:26:45]

lgtm

[Timothy Loh, 2016-05-16 04:57:29]

lgtm, although I only looked at the CSS parser change and not the test changes

[svillar, 2016-05-16 14:27:43]

The patch looks good but I have a suggestion.

https://codereview.chromium.org/1979603002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1979603002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:748: // Flex sizes are
invalid on the min slot of minmax() content.
This is nitpicking but I prefer to talk about min sizing function which is what
the specs say.

https://codereview.chromium.org/1979603002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:752: minTrackBreadth =
Length(Auto);
Why not generating a minmax(auto,fr) in the parser directly ? It has one
downside, for getComputedStyle() it will be impossible to differentiate between
1fr and minmax(auto,1fr) because we'll represent both the same way, but I think
that's positive, because the declaration 1fr is a bit misleading IMO, as we
don't allow it for min function.

[Manuel Rego, 2016-05-16 15:01:49]

Thanks for the reviews.

https://codereview.chromium.org/1979603002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1979603002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:748: // Flex sizes are
invalid on the min slot of minmax() content.
On 2016/05/16 14:27:43, svillar wrote:
> This is nitpicking but I prefer to talk about min sizing function which is
what
> the specs say.

Done.

https://codereview.chromium.org/1979603002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:752: minTrackBreadth =
Length(Auto);
On 2016/05/16 14:27:43, svillar wrote:
> Why not generating a minmax(auto,fr) in the parser directly ? It has one
> downside, for getComputedStyle() it will be impossible to differentiate
between
> 1fr and minmax(auto,1fr) because we'll represent both the same way, but I
think
> that's positive, because the declaration 1fr is a bit misleading IMO, as we
> don't allow it for min function.

First the computed style returns the size of the tracks in pixels.

I guess you were talking about grid.style.gridTemplateColumns,
I don't like your suggestion for that case.
For example we could have in your CSS:
  grid-template-columns: 1fr;
and when you check the value in the inspector you see:
  grid-template-columns: minmax(auto, 1fr);

I'm not sure if this is really common in other properties,
but I don't like a lot the change.

Regarding the definition of "1fr",
I agree that there're some issues on the spec yet [1],
but the <flex> description seems specific enough [2]:
"When appearing outside a minmax() notation,
 implies an automatic minimum (i.e. ''minmax(auto, <flex>)'')."

So, at this moment I'll keep it as is.

[1] https://lists.w3.org/Archives/Public/www-style/2016May/0110.html
[2] https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-flex

[Manuel Rego, 2016-05-16 15:01:56]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2016-05-16 15:01:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1979603002/#ps20001
(title: "New version fixing comment")

[commit-bot: I haz the power, 2016-05-16 15:02:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1979603002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1979603002/20001

[commit-bot: I haz the power, 2016-05-16 16:48:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-16 16:50:08]

Description was changed from

==========
[css-grid] Fix behavior of flexible track breadths

This patch is fixing 2 issues that are interrelated:

* Flex sizes are invalid on the min slot of minmax().

  The syntax has been recently updated on the spec:
    <track-size> =
      <track-breadth> |
      minmax( <inflexible-breadth> , <track-breadth> )

  To fix this, the patch modifies consumeGridBreadth()
  in CSSPropertyParser, adding a new type of restriction
  "InflexibleSizeOnly".

* Flex sizes outside minmax() behave as auto minimum.

  Flex sizes outside minmax() were previously behaving like
  minimum and maximum (e.g. 1fr => minmax(1fr, 1fr)).
  However the spec changed and now this would be invalid,
  so they should behave like auto minimum (e.g. minmax(auto, 1fr)).

  To achieve this the patch adds a new condition in
  LayoutGrid::gridTrackSize();

The patch includes new test cases checking specifically these 2 issues.
In addition several tests results have been updated to reflect
the new behavior. Also, some cases that are now invalid and
were not testing anything new have been removed.

BUG=611407,611410
TEST=fast/css-grid-layout/flex-content-resolution-columns.html
TEST=fast/css-grid-layout/flex-content-resolution-rows.html
TEST=fast/css-grid-layout/grid-columns-rows-get-set.html
==========

to

==========
[css-grid] Fix behavior of flexible track breadths

This patch is fixing 2 issues that are interrelated:

* Flex sizes are invalid on the min slot of minmax().

  The syntax has been recently updated on the spec:
    <track-size> =
      <track-breadth> |
      minmax( <inflexible-breadth> , <track-breadth> )

  To fix this, the patch modifies consumeGridBreadth()
  in CSSPropertyParser, adding a new type of restriction
  "InflexibleSizeOnly".

* Flex sizes outside minmax() behave as auto minimum.

  Flex sizes outside minmax() were previously behaving like
  minimum and maximum (e.g. 1fr => minmax(1fr, 1fr)).
  However the spec changed and now this would be invalid,
  so they should behave like auto minimum (e.g. minmax(auto, 1fr)).

  To achieve this the patch adds a new condition in
  LayoutGrid::gridTrackSize();

The patch includes new test cases checking specifically these 2 issues.
In addition several tests results have been updated to reflect
the new behavior. Also, some cases that are now invalid and
were not testing anything new have been removed.

BUG=611407,611410
TEST=fast/css-grid-layout/flex-content-resolution-columns.html
TEST=fast/css-grid-layout/flex-content-resolution-rows.html
TEST=fast/css-grid-layout/grid-columns-rows-get-set.html

Committed: https://crrev.com/5ab36ed9abb8f19c79d7c9bd09ca02460aad50c2
Cr-Commit-Position: refs/heads/master@{#393846}
==========

[commit-bot: I haz the power, 2016-05-16 16:50:09]

Patchset 2 (id:??) landed as
https://crrev.com/5ab36ed9abb8f19c79d7c9bd09ca02460aad50c2
Cr-Commit-Position: refs/heads/master@{#393846}