1339619 - text-size-adjust should just use a keyword table

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

neerja discovered that text-size-adjust doesn't use a keyword table, but instead uses VARIANT_AUTO and VARIANT_NONE, and then has to hardcode specified-style-to-computed-style conversion code in nsRuleNode.cpp (and the reverse in nsComputedDOMStyle.cpp)

It should just use a keyword table, for consistency.

(It's a keyword-valued property; might as well just use our standard keyword-valued-property codepaths, rather than trying to be tricky and hardcoding something more direct and more verbose/error-prone than the normal codepath.)

I think the files to change here are nsCSSPropList.h, nsRuleNode.cpp, and nsComputedDOMStyle.cpp, and nsCSSProps.*

Comment 1

[Daniel Holbert [:dholbert]]

I suspect the code exists in its current state (eschewing a keyword table) because it's probably a bit faster this way.

However, I claim the perf benefit here doesn't matter too much, since this is a prefixed property which likely doesn't get much usage.  So, better IMO to optimize for code-comprehensibility & maintainability in this case. (Particularly in the nsComputedDOMStyle::DoGetTextSizeAdjust[1] impl -- we can make that a nsCSSProps::ValueToKeywordEnum() invocation, rather than needing a switch statement. And particularly because this seems to be the *only* strictly-keyword-valued-property that does not use a keyword table.[2])

dbaron: are you OK with this change? If you'd prefer to leave this as-is, I'll defer to your judgement.  (It looks like this is code that you originally wrote back in bug 627842.)

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#4205
[2] I skimmed nsComputedDOMStyle for explicit "SetIdent(eCSSKeyword" calls and didn't find any others for pure-keyword-properties-with-no-keyword-table.

Comment 2

[Daniel Holbert [:dholbert]]

(I filed this bug because our current implementation created a bit of confusion, by accident -- neerja mistakenly (but entirely understandably) assumed that she could implement "column-span:all|none" in exactly the same way that we implement "-moz-text-size-ajust:auto|none".  Both properties have 2 possible keyword values, one of which is "none"; seems reasonable that they could have a very similar implementation. But, they can't, because we don't have special "all" handling like we do for "auto".)

So: in the interests of avoiding similar pitfalls in the future, I'd like to either:
 (1) bring us into alignment with all the other keyword-valued properties here
...OR
 (2) add a comment to the -moz-text-size-adjust section in nsCSSPropList.h to explicitly call out that it's different & hint at why.

I lean towards (1) but we could do (2) instead if you prefer -- dbaron, what do you recommend?

Comment 3

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

I'm fine with (1); I don't think I have particularly strong preferences here (but in the past I've tried to avoid mixing VARIANT_AUTO and VARIANT_NONE with things that have keyword tables).

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #3)
> I'm fine with (1);

OK, thanks!

> (but in the past I've tried to avoid mixing VARIANT_AUTO and VARIANT_NONE
> with things that have keyword tables).

Agreed that that's desirable, and the end result of (1) will still adhere to that. (We'll shift from VARIANT_AUTO|VARIANT_NONE to VARIANT_HK, with eCSSKeyword_None & eCSSKeyword_Auto being the two keywords in the keyword table.)

Comment 5

[Neerja Pancholi[:neerja]]

Attachment: Bug 1339619 - Refactor parsing code for '-moz-text-size-adjust' to use the (more conventional) keyword table approach to calculate computed style.

Review commit: https://reviewboard.mozilla.org/r/113180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113180/

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8838224 [details]
Bug 1339619 - Refactor parsing code for '-moz-text-size-adjust' to use the (more conventional) keyword table approach to calculate computed style.

https://reviewboard.mozilla.org/r/113180/#review114678

Thanks! Looks good -- r=me with 2 nits addressed:

::: layout/style/nsComputedDOMStyle.h:438
(Diff revision 1)
>    already_AddRefed<CSSValue> DoGetTextOverflow();
> +  already_AddRefed<CSSValue> DoGetTextSizeAdjust();
>    already_AddRefed<CSSValue> DoGetTextTransform();
>    already_AddRefed<CSSValue> DoGetTextShadow();
>    already_AddRefed<CSSValue> DoGetLetterSpacing();

Nit: move this a few lines further down, to be just after DoGetTextShadow.  That way, it'll be in alphabetical order, & it'll be consistent with the ordering you've used in the nsComputedDOMStyle.cpp file.

::: layout/style/nsRuleNode.cpp:4934
(Diff revision 1)
>    // text-size-adjust: none, auto, inherit, initial
> -  SetValue(*aRuleData->ValueForTextSizeAdjust(), text->mTextSizeAdjust,
> +  SetValue(*aRuleData->ValueForTextSizeAdjust(),

Comment here needs updating.  It should now say "enum, inherit, initial" I guess (matching the comments for the neighboring SetValue calls).

Comment 7

[Neerja Pancholi[:neerja]]

Comment on attachment 8838224 [details]
Bug 1339619 - Refactor parsing code for '-moz-text-size-adjust' to use the (more conventional) keyword table approach to calculate computed style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113180/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb0f9802f0d0
Refactor parsing code for '-moz-text-size-adjust' to use the (more conventional) keyword table approach to calculate computed style. r=dholbert

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bb0f9802f0d0