[css-align] Don't resolve 'auto' values for computed style.

[css-align] Don't resolve 'auto' values for computed style.

The CSS Box Alignment specification has been changed recently so that
now all the propeties have the specificed value as computed value. The
rationale of this change are at the associated W3C github issue [1].

This change implies that we don't need to execute the StyleAdjuter
logic we implemented specifically for supporting 'auto' values
resolution for computed style. We can live now with resolution at
layout time only.

[1] https://github.com/w3c/csswg-drafts/issues/440

BUG=725489
Review-Url: https://codereview.chromium.org/2455093002
Cr-Commit-Position: refs/heads/master@{#475400}
Committed: https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c8190720f411a8a7

[jfernandez, 2017-05-23 13:50:33]

Description was changed from

==========
[css-align] Don't resolve 'auto' values for computed style.

The CSS Box Alignment specification has been changed recently so that
now all the propeties have the specificed value as computed value. The
rationale of this change are at the associated W3C github issue [1].

This change implies that we don't need to execute the StyleAdjuter
logic we implemented specifically for supporting 'auto' values
resolution for computed style. We can live now with resolution at
layout time only.

[1] https://github.com/w3c/csswg-drafts/issues/440
==========

to

==========
[css-align] Don't resolve 'auto' values for computed style.

The CSS Box Alignment specification has been changed recently so that
now all the propeties have the specificed value as computed value. The
rationale of this change are at the associated W3C github issue [1].

This change implies that we don't need to execute the StyleAdjuter
logic we implemented specifically for supporting 'auto' values
resolution for computed style. We can live now with resolution at
layout time only.

[1] https://github.com/w3c/csswg-drafts/issues/440

BUG=725489
==========

[jfernandez, 2017-05-23 13:50:34]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org, meade@chromium.org, rego@igalia.com,
suzyh@chromium.org, svillar@igalia.com

[jfernandez, 2017-05-23 13:52:54]

Patch ready for review.

[cbiesinger, 2017-05-24 16:54:15]

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt
(right):

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt:32: FAIL
window.getComputedStyle(flexbox, null).alignSelf should be normal. Was auto.
That doesn't seem right. Shouldn't you change css-properties.html instead?

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/alignment/alignment-and-anomymous-boxes-expected.html
(right):

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/alignment/alignment-and-anomymous-boxes-expected.html:24:
<div class="selfCenter">foobar</div>
I think it would be easier to understand if you only apply align-self to flex,
not both.

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/alignment/parse-place-self.html
(right):

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/alignment/parse-place-self.html:211:
checkPlaceSelfValues(placeSelfStartSafe, "", "auto", "auto");
(unrelated, but placeSelfStartSafe does not look correct for this test :( )

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:156: // relayed out so we
can compute new available space.
This comment is not quite correct -- in the row axis you check if it changed
either to or from stretching. In the column axis you only check if it changed
from stretching.

And why don't you need to relayout a child if it now stretching but didn't use
to?

[jfernandez, 2017-05-24 22:46:58]

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt
(right):

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt:32: FAIL
window.getComputedStyle(flexbox, null).alignSelf should be normal. Was auto.
On 2017/05/24 16:54:15, cbiesinger wrote:
> That doesn't seem right. Shouldn't you change css-properties.html instead?

Acknowledged.

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/alignment/alignment-and-anomymous-boxes-expected.html
(right):

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/alignment/alignment-and-anomymous-boxes-expected.html:24:
<div class="selfCenter">foobar</div>
On 2017/05/24 16:54:15, cbiesinger wrote:
> I think it would be easier to understand if you only apply align-self to flex,
> not both.

The issue is present in both layout models. Do you mean we should define two
different tests for each one ?

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/alignment/parse-place-self.html
(right):

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/alignment/parse-place-self.html:211:
checkPlaceSelfValues(placeSelfStartSafe, "", "auto", "auto");
On 2017/05/24 16:54:15, cbiesinger wrote:
> (unrelated, but placeSelfStartSafe does not look correct for this test :( )

Acknowledged.

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:156: // relayed out so we
can compute new available space.
On 2017/05/24 16:54:15, cbiesinger wrote:
> This comment is not quite correct -- in the row axis you check if it changed
> either to or from stretching. In the column axis you only check if it changed
> from stretching.

The reason is that such case, to-stretch in the column axis, is already handled
by the regular Grid layout logic (applyStetchAlignmentIfNeeded). The item will
have a new overrideHeight and will be marked for layout (if needed). 

In case of from-stretch in the column axis, the function mentioned before is not
executed because he item has not stretch alignment anymore. We just clear the
overrideHeight. This new code ensures the item is marked for layout as well.

In the case of the changes applying to the row-axis properties, stretching
is not handled by the Grid logic. The general LayoutBox width computation
stretches the item (by default), so we only avoid that by applying the
fit-content logic in case of no-stretch alignment. In both cases we need to
ensure the item will be laid out.

> 
> And why don't you need to relayout a child if it now stretching but didn't use
> to?

As I commented before, this is already done by the Grid layout logic, as part of
the applyStetchAlignmentIfNeeded function.

[meade_UTC10, 2017-05-25 05:22:27]

lgtm for core/css and core/style + style team relevant layout test directories.
I didn't look at anything in the other Source/ directories.

[svillar, 2017-05-25 08:13:09]

https://codereview.chromium.org/2455093002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2455093002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:140: }
Very compact but impossible to read. Could we add some variable or do something
different. Also is it possible to refactor them?

https://codereview.chromium.org/2455093002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:167:
child->SetNeedsLayout(LayoutInvalidationReason::kGridChanged);
Shouldn't we clear the stretch overrides here?

[jfernandez, 2017-05-25 11:25:15]

PTAL

https://codereview.chromium.org/2455093002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2455093002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:140: }
On 2017/05/25 08:13:09, svillar wrote:
> Very compact but impossible to read. Could we add some variable or do
something
> different. Also is it possible to refactor them?

Done.

https://codereview.chromium.org/2455093002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:167:
child->SetNeedsLayout(LayoutInvalidationReason::kGridChanged);
On 2017/05/25 08:13:09, svillar wrote:
> Shouldn't we clear the stretch overrides here?

Well, I'd rather let the Grid layout logic do it when consider it more
convenient.

[cbiesinger1, 2017-05-25 18:10:00]

cbiesinger@google.com changed reviewers:
+ cbiesinger@google.com

[cbiesinger1, 2017-05-25 18:10:02]

Thanks, lgtm as long as svillar is happy. see also comment below.

https://codereview.chromium.org/2455093002/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2455093002/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:140: // relayed out so we
can compute new available space.
OK, thanks for the explanation. I think this comment could be improved with some
of the details you mentioned. Also, the comment only talks about "row changed-to
stretch" and "column changed-from stretch", whereas the code also tests for "row
changed-from stretch". The comment should probably talk about that too.

[suzyh_UTC10 (ex-contributor), 2017-05-25 23:46:16]

suzyh@chromium.org changed reviewers:
- suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2017-05-25 23:46:37]

Removing myself as a reviewer since you've already got an OK from meade.

[svillar, 2017-05-26 10:51:06]

lgtm for the grid part. Remember to improve the comment we talked about.

[jfernandez, 2017-05-29 22:25:07]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2017-05-29 22:25:07]

The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org,
cbiesinger@google.com, svillar@igalia.com
Link to the patchset: https://codereview.chromium.org/2455093002/#ps210001
(title: "Patch for landing.")

[commit-bot: I haz the power, 2017-05-29 22:25:22]

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, 2017-05-30 00:56:55]

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1496096707237630,
"parent_rev": "9abc1e4d296152077d667de7864bc580eccb0638", "commit_rev":
"5389373c6dec30d783eb46b4c8190720f411a8a7"}

[commit-bot: I haz the power, 2017-05-30 00:57:04]

Description was changed from

==========
[css-align] Don't resolve 'auto' values for computed style.

The CSS Box Alignment specification has been changed recently so that
now all the propeties have the specificed value as computed value. The
rationale of this change are at the associated W3C github issue [1].

This change implies that we don't need to execute the StyleAdjuter
logic we implemented specifically for supporting 'auto' values
resolution for computed style. We can live now with resolution at
layout time only.

[1] https://github.com/w3c/csswg-drafts/issues/440

BUG=725489
==========

to

==========
[css-align] Don't resolve 'auto' values for computed style.

The CSS Box Alignment specification has been changed recently so that
now all the propeties have the specificed value as computed value. The
rationale of this change are at the associated W3C github issue [1].

This change implies that we don't need to execute the StyleAdjuter
logic we implemented specifically for supporting 'auto' values
resolution for computed style. We can live now with resolution at
layout time only.

[1] https://github.com/w3c/csswg-drafts/issues/440

BUG=725489

Review-Url: https://codereview.chromium.org/2455093002
Cr-Commit-Position: refs/heads/master@{#475400}
Committed:
https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c819...
==========

[commit-bot: I haz the power, 2017-05-30 00:57:06]

Committed patchset #12 (id:210001) as
https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c819...

[Manuel Rego, 2017-05-30 07:24:05]

@jeffcarp and/or @qyearsley this CL has a small change in one WPT test
but the PR to export the change hasn't been created yet.

Before this was usually an automatic process once the CL landed,
do we need to do anything special now? Thanks.

[qyearsley, 2017-05-30 19:05:15]

On 2017/05/30 at 07:24:05, rego wrote:
> @jeffcarp and/or @qyearsley this CL has a small change in one WPT test
> but the PR to export the change hasn't been created yet.
> 
> Before this was usually an automatic process once the CL landed,
> do we need to do anything special now? Thanks.

Nope, it should still be an automatic process. If a PR isn't made just after
landing, then that generally means that the wpt-exporter
(https://build.chromium.org/p/chromium.infra.cron/builders/wpt-exporter) is
stuck, or that there's some other bug in the exporter.

In this case, the exporter prints:
Commit details:
5389373c6dec30d783eb46b4c8190720f411a8a7
[css-align] Don't resolve 'auto' values for computed style.
Patch did not apply cleanly, skipping.

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fwp...

Jeff, what are the possible reasons for the patch not applying cleanly again?

[Manuel Rego, 2017-05-30 19:10:16]

On 2017/05/30 19:05:15, qyearsley wrote:
> On 2017/05/30 at 07:24:05, rego wrote:
> > @jeffcarp and/or @qyearsley this CL has a small change in one WPT test
> > but the PR to export the change hasn't been created yet.
> > 
> > Before this was usually an automatic process once the CL landed,
> > do we need to do anything special now? Thanks.
> 
> Nope, it should still be an automatic process. If a PR isn't made just after
> landing, then that generally means that the wpt-exporter
> (https://build.chromium.org/p/chromium.infra.cron/builders/wpt-exporter) is
> stuck, or that there's some other bug in the exporter.
> 
> In this case, the exporter prints:
> Commit details:
> 5389373c6dec30d783eb46b4c8190720f411a8a7
> [css-align] Don't resolve 'auto' values for computed style.
> Patch did not apply cleanly, skipping.
> 
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fwp...
> 
> Jeff, what are the possible reasons for the patch not applying cleanly again?

Note that this was exported later and merged in WPT:
https://github.com/w3c/web-platform-tests/pull/6088

[blink-reviews, 2017-05-30 19:17:13]

Hi, the initial delay was due to the exporter being flaky much of this
weekend due to crbug.com/725959

Is everything resolved? I can look into specific patches manually if not.

Thanks for bearing with the exporter as I work out these hiccups.

On Tue, May 30, 2017 at 3:10 PM <rego@igalia.com> wrote:

> On 2017/05/30 19:05:15, qyearsley wrote:
> > On 2017/05/30 at 07:24:05, rego wrote:
> > > @jeffcarp and/or @qyearsley this CL has a small change in one WPT test
> > > but the PR to export the change hasn't been created yet.
> > >
> > > Before this was usually an automatic process once the CL landed,
> > > do we need to do anything special now? Thanks.
> >
> > Nope, it should still be an automatic process. If a PR isn't made just
> after
> > landing, then that generally means that the wpt-exporter
> > (https://build.chromium.org/p/chromium.infra.cron/builders/wpt-exporter)
> is
> > stuck, or that there's some other bug in the exporter.
> >
> > In this case, the exporter prints:
> > Commit details:
> > 5389373c6dec30d783eb46b4c8190720f411a8a7
> > [css-align] Don't resolve 'auto' values for computed style.
> > Patch did not apply cleanly, skipping.
> >
> >
>
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fwp...
> >
> > Jeff, what are the possible reasons for the patch not applying cleanly
> again?
>
> Note that this was exported later and merged in WPT:
> https://github.com/w3c/web-platform-tests/pull/6088
>
>
> https://codereview.chromium.org/2455093002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-05-30 19:17:14]

Hi, the initial delay was due to the exporter being flaky much of this
weekend due to crbug.com/725959

Is everything resolved? I can look into specific patches manually if not.

Thanks for bearing with the exporter as I work out these hiccups.

On Tue, May 30, 2017 at 3:10 PM <rego@igalia.com> wrote:

> On 2017/05/30 19:05:15, qyearsley wrote:
> > On 2017/05/30 at 07:24:05, rego wrote:
> > > @jeffcarp and/or @qyearsley this CL has a small change in one WPT test
> > > but the PR to export the change hasn't been created yet.
> > >
> > > Before this was usually an automatic process once the CL landed,
> > > do we need to do anything special now? Thanks.
> >
> > Nope, it should still be an automatic process. If a PR isn't made just
> after
> > landing, then that generally means that the wpt-exporter
> > (https://build.chromium.org/p/chromium.infra.cron/builders/wpt-exporter)
> is
> > stuck, or that there's some other bug in the exporter.
> >
> > In this case, the exporter prints:
> > Commit details:
> > 5389373c6dec30d783eb46b4c8190720f411a8a7
> > [css-align] Don't resolve 'auto' values for computed style.
> > Patch did not apply cleanly, skipping.
> >
> >
>
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fwp...
> >
> > Jeff, what are the possible reasons for the patch not applying cleanly
> again?
>
> Note that this was exported later and merged in WPT:
> https://github.com/w3c/web-platform-tests/pull/6088
>
>
> https://codereview.chromium.org/2455093002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jfernandez, 2017-05-30 20:20:36]

Hi, 

Everything is fine now. 
Thanks.

On 2017/05/30 19:17:14, chromium-reviews wrote:
> Hi, the initial delay was due to the exporter being flaky much of this
> weekend due to crbug.com/725959
> 
> Is everything resolved? I can look into specific patches manually if not.
> 
> Thanks for bearing with the exporter as I work out these hiccups.
>

[jfernandez, 2017-05-30 20:20:41]

Hi, 

Everything is fine now. 
Thanks.

On 2017/05/30 19:17:14, chromium-reviews wrote:
> Hi, the initial delay was due to the exporter being flaky much of this
> weekend due to crbug.com/725959
> 
> Is everything resolved? I can look into specific patches manually if not.
> 
> Thanks for bearing with the exporter as I work out these hiccups.
>

[pfeldman, 2017-05-30 23:30:56]

A revert of this CL (patchset #12 id:210001) has been created in
https://codereview.chromium.org/2913093002/ by pfeldman@chromium.org.

The reason for reverting is: This patch breaks DevTools toolbars (Console gear
icon, checkbox labels are centered), it also seems to break the Welcome to
Chrome page layout..

[pfeldman, 2017-05-30 23:44:52]

Before this lands again, I'd expect chrome UI to be fixed to not break as a
result of this change. DevTools would require CSS / JS injection to support
backwards compatibility of older front-end versions as well.

[jfernandez, 2017-05-31 14:34:46]

On 2017/05/30 23:44:52, pfeldman wrote:
> Before this lands again, I'd expect chrome UI to be fixed to not break as a
> result of this change. DevTools would require CSS / JS injection to support
> backwards compatibility of older front-end versions as well.

The regression affecting DevTools was caused by this patch, indeed. 
I fixed it in the new CL, which I'll land as soon as it gets reviewed:

 https://codereview.chromium.org/2915773002/

I think there is no need of additional changes in DevTools.