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

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase #1

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")

Comment 1

[Mats Palmgren (inactive)]

I also posted to www-style about equal start/end lines:
https://lists.w3.org/Archives/Public/www-style/2015Oct/0034.html

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f472b12532

Comment 3

[Daniel Holbert [:dholbert]]

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)

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3143e47d7808

Comment 5

[Mats Palmgren (inactive)]

I fixed the nits as suggested.

https://hg.mozilla.org/integration/mozilla-inbound/rev/08064bb472dd

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08064bb472dd
https://hg.mozilla.org/mozilla-central/rev/3143e47d7808

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/08064bb472dd
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3143e47d7808

Comment 8

[Carsten Book [:Tomcat]]

backed this out on request from mats from b2g v2.5 in http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0869ace8c965