[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
Description was changed from ========== [css-align] Don't resolve 'auto' values for computed style. The CSS ...
3 years, 9 months ago
(2017-05-23 13:50:33 UTC)
#1
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
==========
3 years, 9 months ago
(2017-05-23 13:52:54 UTC)
#3
Patch ready for review.
cbiesinger
https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt File third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt (right): https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt#newcode32 third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt:32: FAIL window.getComputedStyle(flexbox, null).alignSelf should be normal. Was auto. That ...
3 years, 9 months ago
(2017-05-24 16:54:15 UTC)
#4
https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt File third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt (right): https://codereview.chromium.org/2455093002/diff/120001/third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt#newcode32 third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt:32: FAIL window.getComputedStyle(flexbox, null).alignSelf should be normal. Was auto. On ...
3 years, 9 months ago
(2017-05-24 22:46:58 UTC)
#5
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
lgtm for core/css and core/style + style team relevant layout test directories. I didn't look ...
3 years, 9 months ago
(2017-05-25 05:22:27 UTC)
#6
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
https://codereview.chromium.org/2455093002/diff/160001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2455093002/diff/160001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode140 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:140: } Very compact but impossible to read. Could we ...
3 years, 9 months ago
(2017-05-25 08:13:09 UTC)
#7
Thanks, lgtm as long as svillar is happy. see also comment below. https://codereview.chromium.org/2455093002/diff/190001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp ...
3 years, 9 months ago
(2017-05-25 18:10:02 UTC)
#10
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.
CQ is committing da patch. Bot data: {"patchset_id": 210001, "attempt_start_ts": 1496096707237630, "parent_rev": "9abc1e4d296152077d667de7864bc580eccb0638", "commit_rev": "5389373c6dec30d783eb46b4c8190720f411a8a7"}
3 years, 9 months ago
(2017-05-30 00:56:55 UTC)
#17
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
Description was changed from ========== [css-align] Don't resolve 'auto' values for computed style. The CSS ...
3 years, 9 months ago
(2017-05-30 00:57:04 UTC)
#18
Message was sent while issue was closed.
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
Committed patchset #12 (id:210001) as https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c8190720f411a8a7
3 years, 9 months ago
(2017-05-30 00:57:06 UTC)
#19
@jeffcarp and/or @qyearsley this CL has a small change in one WPT test but the ...
3 years, 9 months ago
(2017-05-30 07:24:05 UTC)
#20
Message was sent while issue was closed.
@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
On 2017/05/30 at 07:24:05, rego wrote: > @jeffcarp and/or @qyearsley this CL has a small ...
3 years, 9 months ago
(2017-05-30 19:05:15 UTC)
#21
Message was sent while issue was closed.
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
On 2017/05/30 19:05:15, qyearsley wrote: > On 2017/05/30 at 07:24:05, rego wrote: > > @jeffcarp ...
3 years, 9 months ago
(2017-05-30 19:10:16 UTC)
#22
Message was sent while issue was closed.
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
Hi, the initial delay was due to the exporter being flaky much of this weekend ...
3 years, 9 months ago
(2017-05-30 19:17:13 UTC)
#23
Message was sent while issue was closed.
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
Hi, the initial delay was due to the exporter being flaky much of this weekend ...
3 years, 9 months ago
(2017-05-30 19:17:14 UTC)
#24
Message was sent while issue was closed.
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
Hi, Everything is fine now. Thanks. On 2017/05/30 19:17:14, chromium-reviews wrote: > Hi, the initial ...
3 years, 9 months ago
(2017-05-30 20:20:36 UTC)
#25
Message was sent while issue was closed.
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
Hi, Everything is fine now. Thanks. On 2017/05/30 19:17:14, chromium-reviews wrote: > Hi, the initial ...
3 years, 9 months ago
(2017-05-30 20:20:41 UTC)
#26
Message was sent while issue was closed.
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
A revert of this CL (patchset #12 id:210001) has been created in https://codereview.chromium.org/2913093002/ by pfeldman@chromium.org. ...
3 years, 9 months ago
(2017-05-30 23:30:56 UTC)
#27
Message was sent while issue was closed.
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
Before this lands again, I'd expect chrome UI to be fixed to not break as ...
3 years, 9 months ago
(2017-05-30 23:44:52 UTC)
#28
Message was sent while issue was closed.
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
On 2017/05/30 23:44:52, pfeldman wrote: > Before this lands again, I'd expect chrome UI to ...
3 years, 9 months ago
(2017-05-31 14:34:46 UTC)
#29
Message was sent while issue was closed.
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.
Issue 2455093002: [css-align] Don't resolve 'auto' values for computed style.
(Closed)
Created 4 years, 4 months ago by jfernandez
Modified 3 years, 9 months ago
Reviewers: cbiesinger, cbiesinger1, meade_UTC10, Manuel Rego, svillar
Base URL:
Comments: 13