Closed Bug 1230398 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Mats, are you planning to look at this, or did you want someone else to?
Flags: needinfo?(mats)
Summary: [css-align] "true start" should serialize to "start" etc → [css-align] "unsafe start" (formerly "true start") should serialize to "start" etc
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.
Attachment #8702163 - Flags: review?(dholbert) → review+
> 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.
https://hg.mozilla.org/mozilla-central/rev/38258df6f6d4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.