Bug 107044 - [CSS Grid Layout] Add grid.css to hold the common grid testing code
Summary: [CSS Grid Layout] Add grid.css to hold the common grid testing code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
: 104869 (view as bug list)
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2013-01-16 12:41 PST by Julien Chaffraix
Modified: 2013-01-22 15:34 PST (History)
5 users (show)

See Also:


Attachments
Proposed change 1: Added grid.css and move most files to it. (36.12 KB, patch)
2013-01-18 17:10 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed change 2: Updated the class names per Tony's request. (75.08 KB, patch)
2013-01-22 12:58 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed change 3: s/id/class/g. (75.30 KB, patch)
2013-01-22 14:49 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.