1230398 - [css-align] "unsafe start" (formerly "true start") should serialize to "start" etc

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

https://lists.w3.org/Archives/Public/www-style/2015Dec/0067.html

>> Does the above mean that "true start" should serialize to "start" now?
> 
> Yes, that's a side-effect of making the omission of 'true' equivalent
> to leaving it in -- since properties always serialize to their shortest
> representation (modulo any back-compat issues).
> 
> ~fantasai

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Mats, are you planning to look at this, or did you want someone else to?

Comment 2

[Mats Palmgren (inactive)]

Attachment: [css-align] Don't output 'unsafe' in serialization because it's the default

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d1f38013893

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8702163 [details] [diff] [review]
[css-align] Don't output 'unsafe' in serialization because it's the default

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

r=me, with the following addressed as you see fit:

::: layout/style/nsCSSValue.cpp
@@ +1031,3 @@
>    const auto& kwtable(nsCSSProps::kAlignAllKeywords);
>    AppendASCIItoUTF16(nsCSSProps::ValueToKeyword(aValue, kwtable), aResult);
> +  if (MOZ_UNLIKELY(overflowPos == NS_STYLE_ALIGN_SAFE)) {

Two nits:
 (1) The reason for this special-case is non-obvious.  Please add a brief explanatory comment, e.g.
  // Don't bother serializing 'unsafe' keyword; it's the default.

 (2) This would perhaps be clearer and more future-proof (for future new <overflow-position> keywords) if we formulated this check as:
    if (overflowPos != NS_STYLE_ALIGN_UNSAFE)
...i.e. if we specifically exclude the value that we know we should drop, instead of explicitly including the other value.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38258df6f6d4

Comment 5

[Mats Palmgren (inactive)]

> i.e. if we specifically exclude the value that we know we should drop, instead of explicitly including the other value.

We can't do that since it would break the case where neither SAFE/UNSAFE is set.
It's currently a ternary, where both bits being zero meaning "neither 'safe'
nor 'unsafe' was specified".  This state isn't needed anymore since 'unsafe'
is now the default for all box types.  It's probably wise to wait a while
before doing that change though, until the spec stabilizes a bit more.
I filed bug 1237150 about it.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38258df6f6d4