1313254 - [css-align] Change "baseline|last-baseline" to "[ first | last ]? baseline"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

https://lists.w3.org/Archives/Public/www-style/2016Oct/0130.html
- RESOLVED: Change "baseline|last-baseline" to "[ first | last ]? baseline"

================
https://github.com/w3c/csswg-drafts/issues/210

  fantasai: So for consistency in the box alignment we'd drop the
            hyphen so authors don't have to remember. You can do
            first|last or leave it off which implies first.
  dbaron: I'm okay if it's first followed by baseline and not
          anything in the middle.
  fantasai: Immediately adjacent would be the requirement.
================

So it seems to me that it's not a semantic change, just syntax.

Comment 1

[Mats Palmgren (inactive)]

Looks like an easy fix, I'll take a stab at this...

I think it should block since we shouldn't ship with obsolete CSS syntax if we can avoid it.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-align] Change "baseline|last-baseline" to "[ first | last ]? baseline".

Note: I'm keeping the eCSSKeyword_last_baseline, although it's now only
needed for DevTools auto-completion:
http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/layout/style/nsCSSProps.cpp#1405,1423,1440

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-align] Change "last-baseline" to "last baseline" in layout/ (scripted change).

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-align] Change "last-baseline" to "last baseline" in devtools/ (scripted change).

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-align] Change "last-baseline" to "last baseline" in testing/ (scripted change).

"hg log" tells me we've made local changes to this file in the past, although I guess this is a file we sync with some upstream repo maybe?

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Add a couple of uses of 'first baseline' to ensure it's a synonym for 'baseline'.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f2b5b81c203&exclusion_profile=false

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8805295 [details] [diff] [review]
part 3 - [css-align] Change "last-baseline" to "last baseline" in devtools/ (scripted change).

Review of attachment 8805295 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Greg, would you mind looking at this change. Seems simple enough, but it's in the properties-db.js file, so I'd prefer if you checked the potential implications with compatibility/tests/...

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-align] Change "last-baseline" to "last baseline" in layout/ (scripted change).

(rebased)

Comment 9

[Greg Tatum [:gregtatum]]

Comment on attachment 8805295 [details] [diff] [review]
part 3 - [css-align] Change "last-baseline" to "last baseline" in devtools/ (scripted change).

Review of attachment 8805295 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me, thanks!

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8805292 [details] [diff] [review]
part 1 - [css-align] Change "baseline|last-baseline" to "[ first | last ]? baseline".

Review of attachment 8805292 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +7564,5 @@
> +    if (!ident) {
> +      return false;
> +    }
> +    baselinePrefix = keyword;
> +    keyword = nsCSSKeywords::LookupKeyword(*ident);

If you like: IMO, this would be easier to follow/maintain if you moved this assignment...
  baselinePrefix = keyword;
...a few lines further up, to be the first thing in this clause.

That more clearly separates the steps here into...
 (1) Process the token we just parsed (first|last) (and stomp over the bogus initial baselinePrefix value ASAP, if appropriate)
 (2) Parse the next token.

(It's nice to have a clearer separation between these steps. The logic in this function is kinda hard to skim through, so the more-clearly-separated distinct steps are, the better.)

(I suspect you put it down here so that we could optimize away the baselinePrefix assignment when NextIdent() returns null.  I think that's an extremely edge case [stylesheet terminating midway through a "baseline" value], and the assignment is extremely cheap, and the compiler might even do this optimization on your behalf anyway -- so it's better to optimize on the side of readability/maintainability/separation-of-concerns here, IMO.)

::: layout/style/nsCSSProps.cpp
@@ -1308,5 @@
>    { eCSSKeyword_center,        NS_STYLE_ALIGN_CENTER },
>    { eCSSKeyword_left,          NS_STYLE_ALIGN_LEFT },
>    { eCSSKeyword_right,         NS_STYLE_ALIGN_RIGHT },
>    { eCSSKeyword_baseline,      NS_STYLE_ALIGN_BASELINE },
> -  { eCSSKeyword_last_baseline, NS_STYLE_ALIGN_LAST_BASELINE },

Might be worth leaving behind a comment for "NS_STYLE_ALIGN_LAST_BASELINE" here, so that other clients of these keyword-tables are clued in to the fact that there's some extra subtlety here.

E.g. maybe insert this line here:
  // Also "first baseline" & "last baseline"; see CSSParserImpl::ParseAlignEnum

(same goes for the other tweaked keyword tables)

::: layout/style/test/property_database.js
@@ +4485,5 @@
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "normal" ],
>      other_values: [ "start", "end", "flex-start", "flex-end", "center", "left",
>                      "right", "space-between", "space-around", "space-evenly",
> +                    "first baseline", "last baseline", "stretch", "start safe",

In most of these properties, you're only testing 2 out of the 3 possibilities -- e.g. here, just "first baseline" and "last baseline" but not bare "baseline".

Let's just consistently test all 3 spellings, for consistency/robustness.  (The 1 extra value doesn't make the list much longer -- this isn't like "safe"/"unsafe" modifiers & alignment fallback values, where we justifiably only test a few combinations because testing the full value-combination-space would be prohibitively huge.)

@@ +4512,5 @@
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "auto" ],
>      other_values: [ "normal", "start", "flex-start", "flex-end", "center", "stretch",
> +                    "first baseline", "last baseline", "right safe", "unsafe center",

Same here.

@@ +4523,5 @@
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "normal" ],
>      other_values: [ "start", "end", "flex-start", "flex-end", "center", "left",
>                      "right", "space-between", "space-around", "space-evenly",
> +                    "baseline", "last baseline", "stretch", "start safe",

Same here. (here, "first baseline" is the one that we're missing".)

@@ +4537,5 @@
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "auto", "normal" ],
>      other_values: [ "end", "flex-start", "flex-end", "self-start", "self-end",
> +                    "center", "left", "right", "first baseline", "stretch", "start",

Same here. (We're only testing one baseline value here; let's test all three.)

@@ +4554,5 @@
>      initial_values: [ "auto" ],
>      other_values: [ "normal", "start", "end", "flex-start", "flex-end", "self-start",
> +                    "self-end", "center", "left", "right", "first baseline",
> +                    "last baseline", "stretch", "left unsafe", "unsafe right",
> +                    "safe right", "center safe", "baseline" ],

(Here you are actually testing all three, but they're split up with "baseline" at the very end for some reason. Probably cleaner to group them together, but not a huge deal.)

@@ +4555,5 @@
>      other_values: [ "normal", "start", "end", "flex-start", "flex-end", "self-start",
> +                    "self-end", "center", "left", "right", "first baseline",
> +                    "last baseline", "stretch", "left unsafe", "unsafe right",
> +                    "safe right", "center safe", "baseline" ],
> +    invalid_values: [ "space-between", "abc", "30px", "none", "first", "last",

In one of these properties (maybe just this one), please also test that "baseline first" and/or "baseline last" are rejected, as a sanity-check that we're not parsing these as keywords-that-can-occur-in-any-order.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8806020 [details] [diff] [review]
part 2 - [css-align] Change "last-baseline" to "last baseline" in layout/ (scripted change).

Review of attachment 8806020 [details] [diff] [review]:
-----------------------------------------------------------------

r=me -- one request for a followup patch:

::: layout/generic/nsGridContainerFrame.cpp
@@ +167,5 @@
>  // this is the end edge of the border box.  For a central baseline it's
>  // the center of the border box.
>  // https://drafts.csswg.org/css-align-3/#synthesize-baselines
>  // For a first-baseline the measure is from the border-box start edge and
> +// for a last baseline the measure is from the border-box end edge.

Hmm -- nsGridContainerFrame has a few comments like this one that contrast "first-baseline" with "last-baseline" (both hyphenated).  Of course, "first-baseline" was never an actual value, but we were adding "first-" just for clarity, and using a hyphen for symmetry with the former spelling of "last-baseline".

After this patch, we'll have these straggling "first-baseline" comments lying around, as not-quite-valid CSS & inconsistent with the "last baseline" comment-text that comes right after them.

SO: consider adding a final patch here to clean these few "first-baseline" usages up, in nsGridContainerFrame at least. (I suspect that's the only/primarily affected file.)

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8805296 [details] [diff] [review]
part 4 - [css-align] Change "last-baseline" to "last baseline" in testing/ (scripted change).

Review of attachment 8805296 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> SO: consider adding a final patch here to clean these few "first-baseline"
> usages up, in nsGridContainerFrame at least. (I suspect that's the
> only/primarily affected file.)

(preemptive rs=me on these requested changes; no need for review.)

Comment 14

[Daniel Holbert [:dholbert]]

nsGridContainerFrame.cpp also has 7 comments that mention "[last-]baseline":
https://dxr.mozilla.org/mozilla-central/search?q=%5Blast-%5Dbaseline&redirect=true

Those need fixup at some point, too -- just replacing the hyphen with a space, I suppose?

Comment 15

[Mats Palmgren (inactive)]

Yeah, I've edited the remaining cases of hyphenated "last-" and "first-" to use space instead.
https://hg.mozilla.org/try/rev/ff0017a99a4bcf4b562eca95ed0215bb26553f6c

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28219b1ad298
part 1 - [css-align] Change "baseline|last-baseline" to "[ first | last ]? baseline".  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/06769bb604a3
part 2 - [css-align] Change "last-baseline" to "last baseline" in layout/ (scripted change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ded8afadf519
part 3 - [css-align] Change "last-baseline" to "last baseline" in devtools/ (scripted change).  r=gregtatum
https://hg.mozilla.org/integration/mozilla-inbound/rev/a65e8975511b
part 4 - [css-align] Change "last-baseline" to "last baseline" in testing/ (scripted change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/255168adca1c
part 5 - [css-grid] Add a couple of uses of 'first baseline' to ensure it's a synonym for 'baseline'.  r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb521e9bf8f8
part 6 - [css-grid] A few comment tweaks.  rs=dholbert

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/28219b1ad298
https://hg.mozilla.org/mozilla-central/rev/06769bb604a3
https://hg.mozilla.org/mozilla-central/rev/ded8afadf519
https://hg.mozilla.org/mozilla-central/rev/a65e8975511b
https://hg.mozilla.org/mozilla-central/rev/255168adca1c
https://hg.mozilla.org/mozilla-central/rev/fb521e9bf8f8