Summary: | [CSS Grid Layout] Add grid.css to hold the common grid testing code | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||||
Component: | Tools / Tests | Assignee: | 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
Julien Chaffraix
2013-01-16 12:41:10 PST
Created attachment 183578 [details]
Proposed change 1: Added grid.css and move most files to it.
*** Bug 104869 has been marked as a duplicate of this bug. *** 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? Created attachment 184037 [details]
Proposed change 2: Updated the class names per Tony's request.
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 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. Created attachment 184051 [details]
Proposed change 3: s/id/class/g.
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> All reviewed patches have been landed. Closing bug. |