522320 - Put auto/none/normal keywords in CSS keyword tables, for properties with enumerated values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Currently, in properties that take enumerated values, we use special-cases everywhere to handle the "none", "auto", and "normal" keywords.

As dbaron suggests at the end of bug 504652 comment 0, we can just put these keywords in the property tables, for properties where the keyword ends up getting mapped to an enumerated value.

Patch coming up soon.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Here's a patch for this, from my patch queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/a355eaa740ad/put_special_enums_in_keyword_tables

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix (single large patch)

I've just added one minor bit of cleanup I that missed before.  New version is from this revision in my patch queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/6fc15a749a0a/put_special_enums_in_keyword_tables

PATCH SUMMARY
This patch makes the following changes:
  - Remove special-casing of none/auto/normal, for properties that map these keywords to enumerated values.  This includes changes to nsCSSParser.cpp, nsRuleNode.cpp, and nsComputedDOMStyle.cpp
  - Add none/auto/normal keywords to tables in nsCSSProps.cpp, where applicable.
  - Extend enumerated-unit special-cases for "text-decoration" and "marks" in nsCSSDeclaration::AppendCSSValueToString, to make them support "none" values.
  - Create "nsStyleUtil::AppendBitmaskCSSValue" method, to share similar code between nsComputedDOMStyle::GetTextDecoration and nsCSSDeclaration::AppendCSSValueToString (for "text-decoration" and "marks").

Here's a list of the properties modified by this patch (from glancing over nsCSSParser.cpp):
 * border
 * border-[side]
 * clear, float
 * color-interpolation, color-interpolation-filters
 * cursor
 * display
 * dominant-baseline
 * image-rendering, shape-rendering, text-rendering
 * ime-mode
 * list-style, list-style-type
 * marks
 * outline
 * outline-style
 * overflow
 * overflow-x, overflow-y
 * page-break-before, page-break-after, page-break-inside
 * pointer-events
 * speak, speak-punctuation
 * table-layout
 * text-decoration
 * text-transform
 * unicode-bidi
 * user-focus, user-input, user-select
 * white-space
 * word-wrap
 * -moz-border-start,-moz-border-end
 * -moz-column-rule
 * -moz-window-shadow
 * The counter/counters sup-part of the 'content' property

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 406308 [details] [diff] [review]
fix (single large patch)

Canceling review-request.  Since this patch is so large, I'm going to break it into two patches -- one for the properties recognized by SVG (for bug 520486), and one for the rest.

This should hopefully make the first patch more bite-size, in the interests of landing bug 520486 (which will also be pretty bite-size) soon.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch 1: for single-valued SVG-animatable properties

Here's the chunk that's needed for bug 520486.  This only includes the single-valued properties that are defined as animatable in the SVG spec -- namely:
 * color-interpolation, color-interpolation-filters
 * display
 * dominant-baseline
 * image-rendering, shape-rendering, text-rendering
 * pointer-events
 * text-decoration

Notably, this patch excludes:
 * 'unicode-bidi', which is recognized in SVG but is explicitly non-animatable.
 * 'overflow', which is SVG-animatable but is represented as a shorthand, and and is hence beyond the scope of bug 520486.
 * 'cursor', which is SVG-animatable, but takes list-values, and is hence beyond the scope of bug 520486.

These excluded properties (along with the rest from comment 2) will be handled in the forthcoming "patch 2".

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch 2: handle the rest of the properties

This patch takes care of the properties excluded in patch 1.

NOTE: When combined, patch 1 & patch 2 are exactly equivalent to attachment 406308 [details] [diff] [review] (the single large patch).

patch 1 is:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/6ade800239e8/put_special_enums_in_keyword_tables_SMIL

patch 2 is:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/6ade800239e8/put_special_enums_in_keyword_tables_OTHER

Comment 6

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

Comment on attachment 406336 [details] [diff] [review]
patch 1: for single-valued SVG-animatable properties

r=dbaron

However, note that bug 520486 also depends on fixing font-variant:normal and font-style:normal in addition to this patch.

Comment 7

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

(In reply to comment #6)
> However, note that bug 520486 also depends on fixing font-variant:normal and
> font-style:normal in addition to this patch.

(And those aren't in patch 2, either.)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> However, note that bug 520486 also depends on fixing font-variant:normal and
> font-style:normal in addition to this patch.

Ah, thanks for catching those!

Incidentally, it turns out that bug 520486 actually usually does the right thing even without fixing "font-style/variant:normal" here.  When we call UncomputeValue on a serialized "normal" enum-value for these properties, nsCSSDeclaration::AppendCSSValueToString appends the empty string instead of "normal" (since 'normal' isn't in the keyword table), and so we end up effectively clearing the SMILOverrideStyle for that property and using the inherited value.  And the inherited value is usually "normal".

In any case, I've got a patch to fix this -- I'll post it shortly.  I'm going to move "overflow-*' into that patch, too, to prepare for Bug 520239.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch 1 addendum: for font-style, font-variant, overflow[-x/y]

Here's the patch that fixes this for 'font-style' and 'font-variant'.  It also steals the overflow / overflow-x / overflow-y fixes from the old version of "patch 2".

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch 2: handle the rest of the properties (now without 'overflow')

Here's "patch 2", now without the fix for the overflow properties (which are now handled in "patch 1 addendum").

Comment 11

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

Comment on attachment 407463 [details] [diff] [review]
patch 1 addendum: for font-style, font-variant, overflow[-x/y]

r=dbaron

Comment 12

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

Comment on attachment 407464 [details] [diff] [review]
patch 2: handle the rest of the properties (now without 'overflow')

Please fix the comments in nsRuleNode.cpp, to change things that say:
// text-transform: enum, none, inherit, initial
to:
// text-transform: enum, inherit, initial

(That applies to the other patches too, actually.)

In nsRuleNode::ComputeUserInterfaceData for the cursor calculation, you
can just write:

  NS_ASSERTION(list->mValue.GetUnit() == eCSSUnit_Enumerated,
               "Unexpected fallback value at end of cursor list");
  ui->mCursor = list->mValue.GetIntValue();


You should also change nsCounterUseNode::GetText (just excess code
removal) to match the changes you made to CSSParserImpl::ParseCounter
(and nsComputedDOMStyle::GetContent).

In theory, it's probably also worth filing a followup bug on making the
'marks' property reject duplicate values just like 'text-decoration'
does.

r=dbaron with that

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> (From update of attachment 407464 [details] [diff] [review])
> Please fix the comments in nsRuleNode.cpp
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ffdb980bfb70

> In nsRuleNode::ComputeUserInterfaceData for the cursor calculation
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/a2263cc2a952

> You should also change nsCounterUseNode::GetText
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/c9fad97deb85

> In theory, it's probably also worth filing a followup bug on making the
> 'marks' property reject duplicate values just like 'text-decoration'

Filed bug 523713 for that.

> r=dbaron with that

Great, thanks!

Comment 14

[Daniel Holbert [:dholbert]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/ee9613776b06
http://hg.mozilla.org/mozilla-central/rev/cb5433f96bfb
http://hg.mozilla.org/mozilla-central/rev/2f6628e89f0d

Comment 15

[Daniel Holbert [:dholbert]]

"patch 1 addendum" caused some orange with the font-variant & font-style as sub-parts of the "font" shorthand:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1256165871.1256167753.1552.gz

I pushed this bustage fix, which I've confirmed fixes the problem in my own build:
http://hg.mozilla.org/mozilla-central/rev/a8dacfc7dfcc
(Thanks to dbaron for quickly pointing out the places that needed patching!)

Comment 16

[Daniel Holbert [:dholbert]]

...and one follow-up to the bustage fix:
http://hg.mozilla.org/mozilla-central/rev/2b9f7c28c481

Comment 17

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

Attachment: patch 3: also do font-weight and font-stretch

This changes the serialization of 'normal', but I'm going to change it back in a patch on top of this one in bug 173331.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 411997 [details] [diff] [review]
patch 3: also do font-weight and font-stretch

r=dholbert

Comment 19

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

http://hg.mozilla.org/mozilla-central/rev/40713937700b