Closed Bug 1211260 Opened 9 years ago Closed 9 years ago

[css-grid] Tweak the implementation of Grid Placement Conflict Handling (due to a spec change)

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files)

Attached file Testcase #1 (deleted) —
https://drafts.csswg.org/css-grid/#grid-placement-errors "If the placement for a grid item contains two lines, and the start line is further end-ward than the end line, swap the two lines." (it used to say that the end line "contributes nothing", i.e. it becomes "span 1")
I also posted to www-style about equal start/end lines: https://lists.w3.org/Archives/Public/www-style/2015Oct/0034.html
Blocks: 1000592
Comment on attachment 8674301 [details] [diff] [review] fix Review of attachment 8674301 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGridContainerFrame.cpp @@ +1416,4 @@ > // http://dev.w3.org/csswg/css-grid/#grid-placement-errors > + if (r.second < r.first) { > + Swap(r.first, r.second); > + } else if (r.second == r.first) { Optional extreme-nit: This might be easier to read if we consistently listed ".first" before ".second" in the < and == comparisons here. So, consider replacing with: if (r.first > r.second) { Swap(r.first, r.second); } else if (r.first == r.second) { ...instead of having first/second ordering flip from line-to-line across those 3 lines, as the patch currently has it. This matches the wording of the spec slightly better ("If ... the start line is further end-ward than the end line"), and it reduces cognitive load slightly IMO. @@ +1419,5 @@ > + } else if (r.second == r.first) { > + if (MOZ_UNLIKELY(r.first == nsStyleGridLine::kMaxLine)) { > + r.first = nsStyleGridLine::kMaxLine - 1; > + } > + r.second = r.first + 1; // XXX subgrid explicit size instead of 1? Nit: This whole section (after "else if") needs 1 more space of indentation. ::: layout/reftests/css-grid/grid-placement-definite-implicit-002-ref.html @@ +31,4 @@ > .XsN { width: 80px; } .XsN ~ span { top:20px; left:60px; } > .NeX { left: 20px; width: 80px; } > +.NsX { width: 80px; } .NsX ~ span { left: 60px; top: 20px; } > +.XeA { width: 100px; } .XeA ~ span { top: 20px; } Two nits: - Drop one space after "span" on these 2 lines, for better alignment with the lines around it. - Also, drop the 2nd (extra) space before the ending "}" on both lines. (unless there's a reason for it that I'm missing)
Attachment #8674301 - Flags: review?(dholbert) → review+
Flags: in-testsuite+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
backed this out on request from mats from b2g v2.5 in http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0869ace8c965
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: