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)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
(Keywords: testcase)
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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")
Assignee | ||
Comment 1•9 years ago
|
||
I also posted to www-style about equal start/end lines:
https://lists.w3.org/Archives/Public/www-style/2015Oct/0034.html
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8674301 -
Flags: review?(dholbert)
Comment 3•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8674301 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 5•9 years ago
|
||
I fixed the nits as suggested.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08064bb472dd
Flags: in-testsuite+
Comment 6•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/08064bb472dd
https://hg.mozilla.org/mozilla-central/rev/3143e47d7808
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 7•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/08064bb472dd
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3143e47d7808
status-b2g-v2.5:
--- → fixed
Comment 8•9 years ago
|
||
backed this out on request from mats from b2g v2.5 in http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0869ace8c965
status-b2g-v2.5:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•