Bug 127837

Summary: [CSS Grid Layout] getComputedStyle() not using author's order when showing named grid lines
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, commit-queue, darin, ddkilzer, dino, esprehn+autocc, glenn, gyuyoung.kim, hyatt, jfernandez, kling, kondapallykalyan, macpherson, menard, rego, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 129041    
Attachments:
Description Flags
Patch hyatt: review+

Sergio Villar Senin
Reported 2014-01-29 03:15:16 PST
It's easy to check that with a declaration like: -webkit-grid-definition-columns: "first" "nav" 10px "last"; the return value of getComputedStyle() is "nav" "first" 10px "last" It isn't really critical because the name refers to the same position independently of the order, but it looks weird from the author's POV as the names are not in the order they were written. The reason why it happens is because we're just taking the names from a HashMap where we store the information about the named grid lines, so the insertion order is not guaranteed at all.
Attachments
Patch (33.20 KB, patch)
2014-02-12 04:39 PST, Sergio Villar Senin 		hyatt: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2014-02-12 04:39:22 PST
Created attachment 223959 [details] Patch
Andreas Kling
Comment 2 2014-02-19 10:18:12 PST
Comment on attachment 223959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223959&action=review > Source/WebCore/rendering/style/RenderStyle.h:1313 > + void setOrderedNamedGridColumnLines(const OrderedNamedGridLinesMap& orderedNamedGridColumnLines) { SET_VAR(rareNonInheritedData.access()->m_grid, m_orderedNamedGridColumnLines, orderedNamedGridColumnLines); } > + void setOrderedNamedGridRowLines(const OrderedNamedGridLinesMap& orderedNamedGridRowLines) { SET_VAR(rareNonInheritedData.access()->m_grid, m_orderedNamedGridRowLines, orderedNamedGridRowLines); } SET_VAR(foo.access()->bar, ...) is a bad pattern since it will always cause a copy-on-write of the "foo" object. The point of the SET_VAR macro is to avoid creating objects when overwriting with identical values, and we're losing that optimization here (and in the code surrounding it too!) > Source/WebCore/rendering/style/StyleGridData.h:66 > NamedGridLinesMap m_namedGridColumnLines; > NamedGridLinesMap m_namedGridRowLines; > > + OrderedNamedGridLinesMap m_orderedNamedGridColumnLines; > + OrderedNamedGridLinesMap m_orderedNamedGridRowLines; This object is starting to look really heavy. Are we sure this is the right direction to be going?
Sergio Villar Senin
Comment 3 2014-02-19 10:29:55 PST
(In reply to comment #2) > (From update of attachment 223959 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223959&action=review > > Source/WebCore/rendering/style/StyleGridData.h:66 > > NamedGridLinesMap m_namedGridColumnLines; > > NamedGridLinesMap m_namedGridRowLines; > > > > + OrderedNamedGridLinesMap m_orderedNamedGridColumnLines; > > + OrderedNamedGridLinesMap m_orderedNamedGridRowLines; > > This object is starting to look really heavy. Are we sure this is the right direction to be going? That's indeed a good question. I tried to include the ordering information in the already available structures (mainly because the vector of ordered named grid lines is just used for getcomputedstyle() but the resulting code was much more complicated. Perhaps I could try again, even penalizing a bit the readability of that uncommon use case, in exchange of a lighter object. Sounds like a plan?
Sergio Villar Senin
Comment 4 2014-03-17 09:19:06 PDT
(In reply to comment #2) > (From update of attachment 223959 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223959&action=review > > > Source/WebCore/rendering/style/StyleGridData.h:66 > > NamedGridLinesMap m_namedGridColumnLines; > > NamedGridLinesMap m_namedGridRowLines; > > > > + OrderedNamedGridLinesMap m_orderedNamedGridColumnLines; > > + OrderedNamedGridLinesMap m_orderedNamedGridRowLines; > > This object is starting to look really heavy. Are we sure this is the right direction to be going? BTW I'm going to remove the NamedGridLinesMap objects in a follow up patch (for bug 130010) so this wouldn't be a big deal.
Dave Hyatt
Comment 5 2014-03-17 10:16:24 PDT
Comment on attachment 223959 [details] Patch r=me
Sergio Villar Senin
Comment 6 2014-03-17 10:31:08 PDT
Committed r165742: <http://trac.webkit.org/changeset/165742>
Note You need to log in before you can comment on or make changes to this bug.