Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(62)

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
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-html_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-style_chromium.org, cbiesinger, chromium-reviews, dglazkov+blink, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, svillar, szager+layoutwatch_chromium.org, zoltan1, jeffcarp
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Patch Set 2 : fixed layout tests. #

Patch Set 3 : Removed the annymous items hack. #

Patch Set 4 : Pathc rebased. #

Patch Set 5 : Fixed layout tests. #

Patch Set 6 : Removed test-harness expected file. #

Patch Set 7 : Removed some outdated comments. #

Total comments: 8

Patch Set 8 : Applied suggested changes. #

Patch Set 9 : 'auto' is not valid for justify-items. #

Total comments: 4

Patch Set 10 : Patch rebased #

Patch Set 11 : Applied some reafactoring. #

Total comments: 1

Patch Set 12 : Patch for landing. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+292 lines, -282 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/css3/flexbox/css-properties.html View 1 2 3 4 5 6 7 8 9 3 chunks +12 lines, -12 lines 0 comments Download
M third_party/WebKit/LayoutTests/css3/flexbox/css-properties-expected.txt View 1 2 3 4 5 6 7 8 9 2 chunks +15 lines, -15 lines 0 comments Download
M third_party/WebKit/LayoutTests/external/wpt/css/css-align-3/default-alignment/place-items-shorthand-006.html View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/alignment-and-anomymous-boxes.html View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/alignment-and-anomymous-boxes-expected.html View 1 chunk +8 lines, -8 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/ensure-flexbox-compatibility-with-initial-values-expected.txt View 1 chunk +8 lines, -8 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/new-alignment-values-invalid-if-grid-not-enabled.html View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/parse-align-self.html View 1 2 3 4 8 chunks +24 lines, -11 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/parse-alignment-of-root-elements.html View 1 16 chunks +27 lines, -27 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/alignment/parse-alignment-of-root-elements-expected.txt View 1 2 3 4 5 1 chunk +0 lines, -30 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/parse-justify-self.html View 1 2 3 4 8 chunks +24 lines, -11 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/parse-place-content.html View 1 2 3 4 5 6 7 2 chunks +5 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/parse-place-items.html View 1 2 3 4 5 6 7 3 chunks +15 lines, -12 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/alignment/parse-place-self.html View 1 2 3 4 5 6 7 5 chunks +33 lines, -29 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/computed-style-listing-expected.txt View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/computed-style-without-renderer-listing-expected.txt View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/svg/css/getComputedStyle-listing-expected.txt View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -3 lines 0 comments Download
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp View 1 2 3 2 chunks +5 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleAdjuster.h View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp View 1 2 3 3 chunks +8 lines, -33 lines 0 comments Download
M third_party/WebKit/Source/core/html/forms/TextControlInnerElements.cpp View 1 2 3 2 chunks +0 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBox.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +18 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +72 lines, -29 lines 0 comments Download
M third_party/WebKit/Source/core/style/ComputedStyle.cpp View 1 2 3 4 5 6 7 8 9 4 chunks +0 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/core/style/ComputedStyleConstants.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 29 (8 generated)
jfernandez
Patch ready for review.
3 years, 9 months ago (2017-05-23 13:52:54 UTC) #3
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
jfernandez
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
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
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
jfernandez
PTAL 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: } On 2017/05/25 08:13:09, svillar wrote: > Very ...
3 years, 9 months ago (2017-05-25 11:25:15 UTC) #8
cbiesinger1
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
suzyh_UTC10 (ex-contributor)
Removing myself as a reviewer since you've already got an OK from meade.
3 years, 9 months ago (2017-05-25 23:46:37 UTC) #12
svillar
lgtm for the grid part. Remember to improve the comment we talked about.
3 years, 9 months ago (2017-05-26 10:51:06 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2455093002/210001
3 years, 9 months ago (2017-05-29 22:25:22 UTC) #16
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
Manuel Rego
@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
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
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
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
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
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
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
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
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
jfernandez
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.

Powered by Google App Engine
This is Rietveld 408576698