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.

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.

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

"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 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/...

(rebased)

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

(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?

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

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

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