[css-align] Initial value of align-content should be 'stretch'.

[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

Since 'stretch' is the default value for the align-content property, this
issue implies that any flexbox line with an undefined height will be
laid out incorrectly, if not explicitly set via CSS, because flex items
can't use the available height, even though they use 'stretch' for their
'align-self' properties.

BUG=647694
Committed: https://crrev.com/eeac3e6a187e362d796fa01489927e773be705ac
Cr-Commit-Position: refs/heads/master@{#419737}

[jfernandez, 2016-09-19 13:13:07]

Description was changed from

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, butusing the values defined in
the old syntax (Flexbox specification) at computed style resolution.

It's worth mentioning that the current layout code already address this
issue, so it shouldn't have effect besides computed style resolution.

BUG=647694
==========

to

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, butusing the values defined in
the old syntax (Flexbox specification) at computed style resolution.

It's worth mentioning that the current layout code already address this
issue, so it shouldn't have effect besides computed style resolution.

BUG=647694
==========

[jfernandez, 2016-09-19 13:13:07]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org

[jfernandez, 2016-09-19 13:14:20]

Description was changed from

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, butusing the values defined in
the old syntax (Flexbox specification) at computed style resolution.

It's worth mentioning that the current layout code already address this
issue, so it shouldn't have effect besides computed style resolution.

BUG=647694
==========

to

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

It's worth mentioning that the current layout code already address this
issue, so it shouldn't have effect besides computed style resolution.

BUG=647694
==========

[cbiesinger, 2016-09-19 20:17:35]

lgtm but I think your description is misleading. The description says "it
shouldn't have effect besides computed style resolution" but as the testcase in
the bug shows (and due to your change to the initial value), there is in fact a
change in layout behavior. Right?

Could you also add a layout test for the testcase from the bug?

[jfernandez, 2016-09-19 20:52:06]

On 2016/09/19 20:17:35, cbiesinger wrote:
> lgtm but I think your description is misleading. The description says "it
> shouldn't have effect besides computed style resolution" but as the testcase
in
> the bug shows (and due to your change to the initial value), there is in fact
a
> change in layout behavior. Right?
> 

I meant that we do the proper layout considering the initial value we have set. 

> Could you also add a layout test for the testcase from the bug?

I knew you were going to request it :) but I didn't consider that testcase
useful as regression test; it's just a case were we need stretch as default
for content alignment. Actually, it's strange none of the current flexbox tests 
detected the issue before. 

Anyway, I'll do what you suggest of using the testcase defined in the
bug report.

[cbiesinger, 2016-09-19 22:32:42]

On 2016/09/19 20:52:06, jfernandez wrote:
> On 2016/09/19 20:17:35, cbiesinger wrote:
> > lgtm but I think your description is misleading. The description says "it
> > shouldn't have effect besides computed style resolution" but as the testcase
> in
> > the bug shows (and due to your change to the initial value), there is in
fact
> a
> > change in layout behavior. Right?
> > 
> 
> I meant that we do the proper layout considering the initial value we have
set. 

Yeah, but maybe you can try to make it clearer that this fixes a bug in layout
also and not just in computed style?

Thanks!

[jfernandez, 2016-09-20 07:31:57]

Description was changed from

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

It's worth mentioning that the current layout code already address this
issue, so it shouldn't have effect besides computed style resolution.

BUG=647694
==========

to

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

Since 'stretch' is the default value for the align-content property, this
issue implies that any flexbox line with an undefined height will be
laid out incorrectly, if not explicitly set via CSS, because flex items 
can't use the available height, even though they use 'stretch' for their
'align-self' properties. 
 
BUG=647694
==========

[jfernandez, 2016-09-20 12:05:27]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2016-09-20 12:05:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2348273002/#ps20001
(title: "Added layout test based on the test case in the bug report.")

[commit-bot: I haz the power, 2016-09-20 12:05:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 13:17:37]

Description was changed from

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

Since 'stretch' is the default value for the align-content property, this
issue implies that any flexbox line with an undefined height will be
laid out incorrectly, if not explicitly set via CSS, because flex items 
can't use the available height, even though they use 'stretch' for their
'align-self' properties. 
 
BUG=647694
==========

to

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

Since 'stretch' is the default value for the align-content property, this
issue implies that any flexbox line with an undefined height will be
laid out incorrectly, if not explicitly set via CSS, because flex items 
can't use the available height, even though they use 'stretch' for their
'align-self' properties. 
 
BUG=647694
==========

[commit-bot: I haz the power, 2016-09-20 13:17:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-20 13:19:41]

Description was changed from

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

Since 'stretch' is the default value for the align-content property, this
issue implies that any flexbox line with an undefined height will be
laid out incorrectly, if not explicitly set via CSS, because flex items 
can't use the available height, even though they use 'stretch' for their
'align-self' properties. 
 
BUG=647694
==========

to

==========
[css-align] Initial value of align-content should be 'stretch'.

We only allow the new CSS Box Alignment syntax when the Grid Layout
feature is enabled. Due to flexbox backward compatibility we have
implemented a different code path for the style initial/default values
assignment. However, we have incorrectly resolved both align-content
and justify-content to 'flex-start' when grid layout is disabled.

This patch changes the approach, so we set 'normal' (the value specified
by the new syntax) for both properties, but using the values defined in
the old syntax (Flexbox specification) at computed style resolution.

Since 'stretch' is the default value for the align-content property, this
issue implies that any flexbox line with an undefined height will be
laid out incorrectly, if not explicitly set via CSS, because flex items
can't use the available height, even though they use 'stretch' for their
'align-self' properties.

BUG=647694

Committed: https://crrev.com/eeac3e6a187e362d796fa01489927e773be705ac
Cr-Commit-Position: refs/heads/master@{#419737}
==========

[commit-bot: I haz the power, 2016-09-20 13:19:42]

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