Summary: | [CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||||
Component: | Layout and Rendering | Assignee: | Manuel Rego Casasnovas <rego> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, jfernandez, svillar | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 60731 | ||||||||||||
Attachments: |
|
Description
Manuel Rego Casasnovas
2015-04-30 05:31:46 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. Created attachment 252051 [details]
Patch
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> 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.
Created attachment 252084 [details]
Patch
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". 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. Created attachment 252298 [details]
Patch
(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. 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 Created attachment 252300 [details]
Patch
Comment on attachment 252300 [details] Patch Clearing flags on attachment: 252300 Committed r183739: <http://trac.webkit.org/changeset/183739> All reviewed patches have been landed. Closing bug. |