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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
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.
Assignee: nobody → mats
Blocks: 1217086
Assignee

Comment 5

3 years ago
"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?
Attachment #8805296 - Flags: review?(dholbert)
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/...
Attachment #8805295 - Flags: review?(pbrosset) → review?(gtatum)
Assignee

Comment 8

3 years ago
(rebased)
Attachment #8805293 - Attachment is obsolete: true
Attachment #8805293 - Flags: review?(dholbert)
Attachment #8806020 - Flags: review?(dholbert)
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!
Attachment #8805295 - Flags: review?(gtatum) → review+
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.
Attachment #8805292 - Flags: review?(dholbert) → review+
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.)
Attachment #8806020 - Flags: review?(dholbert) → review+
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
Attachment #8805296 - Flags: review?(dholbert) → review+
(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.)
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?
Assignee

Comment 15

3 years ago
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

3 years ago
Pushed by mpalmgren@mozilla.com:
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
Assignee

Updated

3 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.