Bug 107044

Summary: [CSS Grid Layout] Add grid.css to hold the common grid testing code
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Tools / TestsAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, ojan, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Proposed change 1: Added grid.css and move most files to it.
none
Proposed change 2: Updated the class names per Tony's request.
none
Proposed change 3: s/id/class/g. none

Description Julien Chaffraix 2013-01-16 12:41:10 PST
Good suggestion from Tony!

This should be similar to what is done in flexbox.css. The idea is to centralize some of the definitions to avoid having to touch lots of files when unprefixing or updating the syntax (e.g. see bug 103336 and bug 103338).
Comment 1 Julien Chaffraix 2013-01-18 17:10:07 PST
Created attachment 183578 [details]
Proposed change 1: Added grid.css and move most files to it.
Comment 2 Julien Chaffraix 2013-01-20 14:03:17 PST
*** Bug 104869 has been marked as a duplicate of this bug. ***
Comment 3 Tony Chang 2013-01-22 10:17:29 PST
Comment on attachment 183578 [details]
Proposed change 1: Added grid.css and move most files to it.

View in context: https://bugs.webkit.org/attachment.cgi?id=183578&action=review

> LayoutTests/fast/css-grid-layout/resources/grid.css:11
> +#a {

I think using single letter ids seems like it could cause problems and weird side effects in the future.  Can we make these classes with descriptive names?
Comment 4 Julien Chaffraix 2013-01-22 12:58:34 PST
Created attachment 184037 [details]
Proposed change 2: Updated the class names per Tony's request.
Comment 5 Tony Chang 2013-01-22 13:48:34 PST
Comment on attachment 184037 [details]
Proposed change 2: Updated the class names per Tony's request.

View in context: https://bugs.webkit.org/attachment.cgi?id=184037&action=review

> LayoutTests/fast/css-grid-layout/resources/grid.css:11
> +#firstRowFirstColumn {

These should be class names, no? You have test cases that have multiple divs with the same id, which is confusing.
Comment 6 Julien Chaffraix 2013-01-22 14:42:13 PST
Comment on attachment 184037 [details]
Proposed change 2: Updated the class names per Tony's request.

View in context: https://bugs.webkit.org/attachment.cgi?id=184037&action=review

>> LayoutTests/fast/css-grid-layout/resources/grid.css:11
>> +#firstRowFirstColumn {
> 
> These should be class names, no? You have test cases that have multiple divs with the same id, which is confusing.

Not really, the tests have had several divs with the same id for some time. From a CSS selector perspective, it is fine. Where it will get hairy is if you try to query the id from JavaScript.

I agree that this should be changed and I might as well pile it on this patch.
Comment 7 Julien Chaffraix 2013-01-22 14:49:49 PST
Created attachment 184051 [details]
Proposed change 3: s/id/class/g.
Comment 8 WebKit Review Bot 2013-01-22 15:34:47 PST
Comment on attachment 184051 [details]
Proposed change 3: s/id/class/g.

Clearing flags on attachment: 184051

Committed r140480: <http://trac.webkit.org/changeset/140480>
Comment 9 WebKit Review Bot 2013-01-22 15:34:51 PST
All reviewed patches have been landed.  Closing bug.