Bug 144449

Summary: [CSS Grid Layout] Wrong computed style for named grid lines in implicit tracks
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch none

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.