144449 – [CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks

RESOLVED FIXED 144449

[CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks

https://bugs.webkit.org/show_bug.cgi?id=144449

Summary [CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks

Manuel Rego Casasnovas

Reported 2015-04-30 05:31:46 PDT

[CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks

Attachments
Patch (7.57 KB, patch) 2015-04-30 05:35 PDT, Manuel Rego Casasnovas	no flags	Details Formatted Diff Diff
Patch (10.42 KB, patch) 2015-04-30 13:38 PDT, Manuel Rego Casasnovas	no flags	Details Formatted Diff Diff
Patch (10.30 KB, patch) 2015-05-03 22:28 PDT, Manuel Rego Casasnovas	no flags	Details Formatted Diff Diff
Patch (10.32 KB, patch) 2015-05-04 00:48 PDT, Manuel Rego Casasnovas	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Manuel Rego Casasnovas

Comment 1 2015-04-30 05:35:01 PDT

The computed style for grid-template-columns|rows is appending the last named grid line after the implicit tracks which shouldn't happen. For example: <div style="display: -webkit-grid; -webkit-grid-template-columns: (a) 100px (b);"> <div style="-webkit-grid-row: 1; width: 100px;">item1</div> <div style="-webkit-grid-row: 1; width: 100px;">item2</div> </div> If you check the computed style of grid-template-columns you'll get: (a) 100px (b) 100px (b) But actually it should be: (a) 100px (b) 100px The same happens for grid-template-rows.

Manuel Rego Casasnovas

Comment 2 2015-04-30 05:35:37 PDT

Created attachment 252051 [details] Patch

Sergio Villar Senin

Comment 3 2015-04-30 06:19:24 PDT

Comment on attachment 252051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252051&action=review Almost there but r- due to some missing test coverage. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1076 > return WTF::move(list); This is quite confusing, I'd better leave the loops as they are, and just change the index here. You can write it like this: unsigned insertionIndex = isRenderGrid ? trackPositions.size() - 1 : trackSizes.size(); addValuesForNamedGridLinesAtIndex(orderedNamedGridLines, insertionIndex, list.get()); > LayoutTests/fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks-expected.txt:4 > + Extra line here? > LayoutTests/fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks-expected.txt:14 > +PASS successfullyParsed is true It'd be nice to add also some tests with multiple names per track line > LayoutTests/fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks.html:13 > +</head> You can omit the <head> tag. Even <body>

Sergio Villar Senin

Comment 4 2015-04-30 06:22:05 PDT

Comment on attachment 252051 [details] Patch I've forgotten to mention that we need test coverage for the non-grid (i.e. no "display: -webkit-grid") cases.

Manuel Rego Casasnovas

Comment 5 2015-04-30 13:38:08 PDT

Created attachment 252084 [details] Patch

Manuel Rego Casasnovas

Comment 6 2015-04-30 13:43:12 PDT

Thanks for the review! Uploaded new version of the patch. (In reply to comment #3) > Comment on attachment 252051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252051&action=review > > Almost there but r- due to some missing test coverage. > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1076 > > return WTF::move(list); > > This is quite confusing, I'd better leave the loops as they are, and just > change the index here. You can write it like this: > > unsigned insertionIndex = isRenderGrid ? trackPositions.size() - 1 : > trackSizes.size(); > addValuesForNamedGridLinesAtIndex(orderedNamedGridLines, insertionIndex, > list.get()); I also thought about doing it that way. But the problem is that we don't have trackPositions here. We only have trackPositions if isLayoutGrid, so we'd need a bigger change in the code, and IMHO it would be more complex than current patch. > > LayoutTests/fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks-expected.txt:4 > > + > > Extra line here? The -expected.txt file is generated, so I think we can ignore this. :-) > > LayoutTests/fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks-expected.txt:14 > > +PASS successfullyParsed is true > > It'd be nice to add also some tests with multiple names per track line Added tests for multiple named grid lines. > > LayoutTests/fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks.html:13 > > +</head> > > You can omit the <head> tag. Even <body> Done (removed <html> too). (In reply to comment #4) > Comment on attachment 252051 [details] > Patch > > I've forgotten to mention that we need test coverage for the non-grid (i.e. > no "display: -webkit-grid") cases. I don't think we need it as we already have non-named-grid-line-get-set.html and it's not possible to have implicit grids in a "non-grid".

Sergio Villar Senin

Comment 7 2015-05-01 02:22:07 PDT

Comment on attachment 252051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252051&action=review >>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1076 >>> return WTF::move(list); >> >> This is quite confusing, I'd better leave the loops as they are, and just change the index here. You can write it like this: >> >> unsigned insertionIndex = isRenderGrid ? trackPositions.size() - 1 : trackSizes.size(); >> addValuesForNamedGridLinesAtIndex(orderedNamedGridLines, insertionIndex, list.get()); > > I also thought about doing it that way. But the problem is that we don't have trackPositions here. > We only have trackPositions if isLayoutGrid, so we'd need a bigger change in the code, and IMHO it would be more complex than current patch. No, you don't need a bigger change, just store the size in a variable as I'm doing here but inside the if-else block. But use a different variable and not the one used inside the loop. It's very important to keep the locality of the variables, and "i" should only be used inside the loop, it makes no sense to depend on a side effect (the loop) to get a value for a variable. If a break or return is eventually added to those loops then the code rottens.

Manuel Rego Casasnovas

Comment 8 2015-05-03 22:28:41 PDT

Created attachment 252298 [details] Patch

Manuel Rego Casasnovas

Comment 9 2015-05-03 22:29:30 PDT

(In reply to comment #7) > Comment on attachment 252051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252051&action=review > > >>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1076 > >>> return WTF::move(list); > >> > >> This is quite confusing, I'd better leave the loops as they are, and just change the index here. You can write it like this: > >> > >> unsigned insertionIndex = isRenderGrid ? trackPositions.size() - 1 : trackSizes.size(); > >> addValuesForNamedGridLinesAtIndex(orderedNamedGridLines, insertionIndex, list.get()); > > > > I also thought about doing it that way. But the problem is that we don't have trackPositions here. > > We only have trackPositions if isLayoutGrid, so we'd need a bigger change in the code, and IMHO it would be more complex than current patch. > > No, you don't need a bigger change, just store the size in a variable as I'm > doing here but inside the if-else block. But use a different variable and > not the one used inside the loop. It's very important to keep the locality > of the variables, and "i" should only be used inside the loop, it makes no > sense to depend on a side effect (the loop) to get a value for a variable. > If a break or return is eventually added to those loops then the code > rottens. Ok, uploaded a new version following this suggestion.

Sergio Villar Senin

Comment 10 2015-05-04 00:35:01 PDT

Comment on attachment 252298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252298&action=review > Source/WebCore/ChangeLog:8 > + If there're implicit tracks trackPositions is bigger than trackSizes, so Nit: If there're implicit tracks **then** trackPositions is > Source/WebCore/ChangeLog:9 > + we need to use the proper index to append the trailing strings in Nit: trailing <ident>s

Manuel Rego Casasnovas

Comment 11 2015-05-04 00:48:00 PDT

Created attachment 252300 [details] Patch

WebKit Commit Bot

Comment 12 2015-05-04 01:38:23 PDT

Comment on attachment 252300 [details] Patch Clearing flags on attachment: 252300 Committed r183739: <http://trac.webkit.org/changeset/183739>

WebKit Commit Bot

Comment 13 2015-05-04 01:38:26 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Manuel Rego Casasnovas

Reported

2015-04-30 05:31 PDT

Modified

2015-05-04 01:38 PDT History

CC List

3 users Show

URL

Keywords

Depends on

Blocks

60731

Dependencies

tree graph