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.

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

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

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

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

(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.)

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

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

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

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

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

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

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

"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!)

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

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 on attachment 411997 [details] [diff] [review] patch 3: also do font-weight and font-stretch r=dholbert