Bug 144449 - [CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks
Summary: [CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2015-04-30 05:31 PDT by Manuel Rego Casasnovas
Modified: 2015-05-04 01:38 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2015-04-30 05:31:46 PDT
[CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks
Comment 1 Manuel Rego Casasnovas 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.
Comment 2 Manuel Rego Casasnovas 2015-04-30 05:35:37 PDT
Created attachment 252051 [details]
Patch
Comment 3 Sergio Villar Senin 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>
Comment 4 Sergio Villar Senin 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.
Comment 5 Manuel Rego Casasnovas 2015-04-30 13:38:08 PDT
Created attachment 252084 [details]
Patch
Comment 6 Manuel Rego Casasnovas 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".
Comment 7 Sergio Villar Senin 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.
Comment 8 Manuel Rego Casasnovas 2015-05-03 22:28:41 PDT
Created attachment 252298 [details]
Patch
Comment 9 Manuel Rego Casasnovas 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.
Comment 10 Sergio Villar Senin 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
Comment 11 Manuel Rego Casasnovas 2015-05-04 00:48:00 PDT
Created attachment 252300 [details]
Patch
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2015-05-04 01:38:26 PDT
All reviewed patches have been landed.  Closing bug.