875250 - implement CSS parsing of text-orientation, text-combine-horizontal properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Spinning this off from bug 772321.

http://dev.w3.org/csswg/css3-writing-modes/

Notably, this is currently at the top of that page:
>  The following features are at-risk and may be dropped during CR:
>    The ‘use-glyph-orientation’ of ‘text-orientation’ 
so it might be wise to hold off on this at the moment)

The property is currently specced as:

text-orientation:
 mixed | upright | sideways-right | sideways-left | sideways | use-glyph-orientation

Comment 1

[John Daggett (:jtd)]

Since it's in the same part of the code, add in parsing of text-combine-horizontal:

  text-combine-horizontal: none | all | digits <integer>?

where <integer> is either 2, 3, or 4.  If omitted, 2 is implied.

Comment 2

[John Daggett (:jtd)]

Attachment: patch, parse text-orientation, text-combine-horizontal

First pass at parsing these properties.  For text-orientation, support a simpler version which will suffice for now:

  text-orientation: auto | upright | sideways;

where 'auto' implies that the orientation is determined by the codepoint (the 'mixed' value from the spec).

Since the digits value for text-combine-horizontal is limited to 2, 3, or 4, values are stored as a single enumerated set of values.

Comment 3

[John Daggett (:jtd)]

Attachment: patch, parse text-orientation, text-combine-horizontal

Fix constant double-defn.

Comment 4

[John Daggett (:jtd)]

Comment on attachment 783006 [details] [diff] [review]
patch, parse text-orientation, text-combine-horizontal

The spec is still somewhat in flux but I think this is a good first cut of the functionality in the spec needed to work on the implementation backend within layout/gfx.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 783006 [details] [diff] [review]
patch, parse text-orientation, text-combine-horizontal

Haven't looked in detail yet, but this stood out to me:

>+    if (mToken.mType == eCSSToken_Number && mToken.mIntegerValid) {
>+      switch(mToken.mInteger) {
[...]
>+        default:
>+          // invalid digits value
>+          return false;
>+          break;

Pretty sure that "break" is unreachable. :)

I'll do a more complete review tomorrow.

Comment 6

[John Daggett (:jtd)]

Attachment: patch, parse text-orientation, text-combine-horizontal

Update to trunk and remove unnecessary 'break;'

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 790631 [details] [diff] [review]
patch, parse text-orientation, text-combine-horizontal

>+++ b/layout/style/nsComputedDOMStyle.cpp
> CSSValue*
>+nsComputedDOMStyle::DoGetTextCombineHorizontal()
>+{
>+  if (tch <= NS_STYLE_TEXT_COMBINE_HORIZ_DIGITS_2) {
>+    val->SetIdent(
>+      nsCSSProps::ValueToKeywordEnum(tch,
>+                                     nsCSSProps::kTextCombineHorizontalKTable));
>+  } else if (tch <= NS_STYLE_TEXT_COMBINE_HORIZ_DIGITS_3) {
>+    val->SetString(NS_LITERAL_STRING("digits 3"));
>+  } else {
>+    val->SetString(NS_LITERAL_STRING("digits 4"));
>+  }

So, if the user specifies "digits 2" and then queries getComputedStyle(), they'll just get the string "digits" back.

But the spec says "Computed value: specified value" for this property.  Doesn't that mean we need to preserve the distinction between "digits 2" and "digits" in our internal representation, so that we can return the right thing from getComputedStyle?

>+++ b/layout/style/nsStyleConsts.h
> // See nsStyleText
>+#define NS_STYLE_TEXT_ORIENTATION_AUTO          0
>+#define NS_STYLE_TEXT_ORIENTATION_UPRIGHT       1
>+#define NS_STYLE_TEXT_ORIENTATION_SIDEWAYS      2

The spec has several other values for this property:
 #  mixed | upright | sideways-right | sideways-left |
 #  sideways | use-glyph-orientation

What's the plan for the other ones not included in this patch?  (If we're anticipating that they'll be in the spec in some form, but we're not sure about the names, it'd be good to call that out in a code-comment somewhere.)

>+// See nsStyleText
>+#define NS_STYLE_TEXT_COMBINE_HORIZ_NONE        0
>+#define NS_STYLE_TEXT_COMBINE_HORIZ_ALL         1
>+#define NS_STYLE_TEXT_COMBINE_HORIZ_DIGITS_2    2
>+#define NS_STYLE_TEXT_COMBINE_HORIZ_DIGITS_3    3
>+#define NS_STYLE_TEXT_COMBINE_HORIZ_DIGITS_4    4

Shouldn't we also have a constant (and parsing code, etc) for "DIGITS_1"? The spec says "Integers outside the range 1-4 are invalid", which I'd think means that "1" should be an allowed digits-suffix.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 790631 [details] [diff] [review]
patch, parse text-orientation, text-combine-horizontal

>+		"text-combine-horizontal": {
>+			domProp: "textCombineHorizontal",
>+			inherited: true,
>+			type: CSS_TYPE_LONGHAND,
>+			initial_values: [ "none" ],
>+			other_values: [ "all", "digits", "digits 2", "digits 3", "digits 4" ],
>+			invalid_values: [ "auto", "all 2", "none all", "digits -3", "digits 0",
>+			                  "digits 12", "none 3", "digits 3.1415" ]

Some other values worth testing:
 valid:
    "digits 1"      [per above, I think "1" should be valid]
    "digits     3"  [extra whitespace between token and num]
 invalid:
    "digits3"       [no whitespace]
    "digits 3 all"  [valid value, with bogus token at the end]

Comment 9

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

(In reply to Daniel Holbert [:dholbert] from comment #7)
> So, if the user specifies "digits 2" and then queries getComputedStyle(),
> they'll just get the string "digits" back.
> 
> But the spec says "Computed value: specified value" for this property. 
> Doesn't that mean we need to preserve the distinction between "digits 2" and
> "digits" in our internal representation, so that we can return the right
> thing from getComputedStyle?

Sounds like a bug in the spec.

(Though that isn't strictly what the spec's "Computed value" line means, but it still seems like a bug in the spec.)

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
>  invalid:
>     "digits3"       [no whitespace]
>     "digits 3 all"  [valid value, with bogus token at the end]

also "digits foo"     [to make sure we do the right thing when the mToken.mType == eCSSToken_Number check fails, in CSSParserImpl::ParseTextCombineHorizontal]

and maybe also "digits 3.0" (Is that invalid?)

Comment 11

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

(In reply to Daniel Holbert [:dholbert] from comment #10)
> and maybe also "digits 3.0" (Is that invalid?)

yes, it's invalid, since the spec requires <integer>

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 790631 [details] [diff] [review]
patch, parse text-orientation, text-combine-horizontal

>+bool
>+CSSParserImpl::ParseTextCombineHorizontal(nsCSSValue& aValue)
[...]
>+    if (mToken.mType == eCSSToken_Number && mToken.mIntegerValid) {
>+      switch(mToken.mInteger) {

nit: insert a space between "switch" and "("
[We're mostly, but not entirely, consistent on this -- MXR says we have 8 instances of "switch(" vs. 159 instances of "switch (" in layout/style.]

>diff --git a/layout/style/nsCSSPropList.h b/layout/style/nsCSSPropList.h
>+CSS_PROP_TEXT(
>+    text-combine-horizontal,
>+    text_combine_horizontal,
>+    TextCombineHorizontal,
>+    CSS_PROPERTY_PARSE_VALUE |
>+        CSS_PROPERTY_VALUE_PARSER_FUNCTION,
>+    "layout.css.vertical-text.enabled",
>+    VARIANT_HK,

s/VARIANT_HK/0/

(That field isn't used for properties that provide their own parser function, and so most properties with PARSER_FUNCTION just have "0" there.  I think it's worth following that pattern, both for consistency and also to avoid the appearance that changing it from VARIANT_HK to something else would actually have an effect.)

>+  // text-orientation: enum, inherit, initial
>+  SetDiscrete(*aRuleData->ValueForTextOrientation(), text->mTextOrientation,
>+              canStoreInRuleTree, SETDSC_ENUMERATED,
>+              parentText->mTextOrientation,
>+              NS_STYLE_TEXT_ORIENTATION_AUTO, 0, 0, 0, 0);

It looks like the spec doesn't actually mention any "auto" value for text-orientation[1] -- it instead has "mixed" as the initial value. Is that something that's changed on the mailing list but hasn't made it into the spec yet?

[1] http://dev.w3.org/csswg/css-writing-modes/#text-orientation0

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 790631 [details] [diff] [review]
patch, parse text-orientation, text-combine-horizontal

[done reviewing; this is basically r+ with the above addressed, but not setting r+ yet since I'd like to know about the spec-conformance questions and see the updated patch]

Comment 14

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

One other question I have is whether NS_STYLE_HINT_REFLOW is really sufficient (in nsStyleText::CalcDifference), especially for text-combine-horizontal, which seems to me likely to need a reframe.

Comment 15

[John Daggett (:jtd)]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Comment on attachment 790631 [details] [diff] [review]
> patch, parse text-orientation, text-combine-horizontal
> 
> So, if the user specifies "digits 2" and then queries getComputedStyle(),
> they'll just get the string "digits" back.

Yes.

> But the spec says "Computed value: specified value" for this
> property. Doesn't that mean we need to preserve the distinction
> between "digits 2" and "digits" in our internal representation, so
> that we can return the right thing from getComputedStyle?

I guess we could but that seems kind of silly.  I'd prefer to get the
spec changed to not require this.

Issue raised on www-style:

http://lists.w3.org/Archives/Public/www-style/2013Aug/0262.html

> The spec has several other values for this property:
>  #  mixed | upright | sideways-right | sideways-left |
>  #  sideways | use-glyph-orientation
> 
> What's the plan for the other ones not included in this patch?  (If
> we're anticipating that they'll be in the spec in some form, but
> we're not sure about the names, it'd be good to call that out in a
> code-comment somewhere.)

Yeah, this is a spec issue I think.  I realized while coding this
patch that the 'mixed' value is really just the equivalent of 'auto'
behavior in other properties.  And the other values of 'sideways' are
redundant and just add noise.  We can certainly add them before
enabling vertical text but to begin to experiment with vertical text
implementation they aren't needed.  I'd really prefer to start with
the minimal required set of values and add others as needed.  The
'use-glyph-orientation' is for use with SVG and doesn't make sense
given that we don't support glyph orientation in SVG.

Issues raised on www-style:

http://lists.w3.org/Archives/Public/www-style/2013Aug/0256.html

> Shouldn't we also have a constant (and parsing code, etc) for
> "DIGITS_1"? The spec says "Integers outside the range 1-4 are
> invalid", which I'd think means that "1" should be an allowed
> digits-suffix.

This issue has been raised on www-style, I don't think the 'digits 1'
value makes any sense.  The whole point of 'text-combine-horizontal'
is to *combine* things, so 'digits 1' behavior just reduces to
'text-orientation: upright'. In which case, authors should be using
'text-orientation: upright' instead... ;)

http://lists.w3.org/Archives/Public/www-style/2013Jul/0699.html
http://lists.w3.org/Archives/Public/www-style/2013Aug/0039.html

Comment 16

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #14)
> One other question I have is whether NS_STYLE_HINT_REFLOW is really
> sufficient (in nsStyleText::CalcDifference), especially for
> text-combine-horizontal, which seems to me likely to need a reframe.

Yeah, switching this to NS_STYLE_HINT_FRAMECHANGE instead.

Comment 17

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

I'd prefer nsChangeHint_ReconstructFrame over NS_STYLE_HINT_FRAMECHANGE, actually; the NS_STYLE_HINT_* values are really only there for backwards-compat.

Comment 18

[John Daggett (:jtd)]

Attachment: patch v2, parse text-orientation, text-combine-horizontal

Updated based on comments.  For areas related to spec issues (e.g. 'digits 1', 'mixed' vs. 'auto', etc.) I didn't make any changes.  But since this is behind a pref that's disabled by default I don't think it's imperative to wait for their resolution.  We can update the code as those are resolved.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to John Daggett (:jtd) from comment #18)
> For areas related to spec issues (e.g. 'digits 1'

Is the validity of "digits 1" up in the air at all at the csswg? (are there any proposals that it not be included?)  If so, fine; but otherwise, I'd rather we include support for that value up-front.  It seems silly to support every other digits value *except* for "digits 1", and punt to a followup on just that value.  (unless its validity is unclear)

RE the "mixed" vs "auto" - if an "auto" value has actually been suggested for this property, then that's fine for now I suppose, given that it's turned off. (I haven't been following www-style on text stuff, but I assume "auto" must've been suggested or you wouldn't have included it here.)

(I did see that you emailed www-style to ask about the computed value of "digits" vs "digits 2" -- thanks for that.)

Comment 20

[John Daggett (:jtd)]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Is the validity of "digits 1" up in the air at all at the csswg?
> (are there any proposals that it not be included?)  If so, fine; but
> otherwise, I'd rather we include support for that value up-front. 
> It seems silly to support every other digits value *except* for
> "digits 1", and punt to a followup on just that value.  (unless its
> validity is unclear)

Richard Ishida asked about this on www-style recently which got me to thinking about the semantics of 'digits 1'.  His post and my post are the last two URL's in comment 15.  I doubt most folks in the CSSWG care, but I do think it's an issue... :P

Basically, I'm baffled by the use case.  Since the digits value is an *upper* limit, 'digits 1' is saying "combine all sequences of 1 or less digits in an inline horizontal run and treat it as a single upright glyph".  Which effectively means only isolated, single ASCII digits within text will be "combined".  Since "combining" here is a no-op in terms of the glyph selected the only change in behavior is the orientation.

But if you're uncomfortable with ignoring this case, it's not a big deal to include support for it and remove it later.

> RE the "mixed" vs "auto" - if an "auto" value has actually been
> suggested for this property, then that's fine for now I suppose,
> given that it's turned off. (I haven't been following www-style on
> text stuff, but I assume "auto" must've been suggested or you
> wouldn't have included it here.)

Yeah, as I described in the post to www-style linked in comment 15, I don't see a reason to add a new keyword for the initial value of "user agents determines" in this case.  But if the CSSWG decides otherwise, it's a trivial to add.

Comment 21

[John Daggett (:jtd)]

Attachment: patch v2a, parse text-orientation, text-combine-horizontal

Updated based on current spec definition of computed value - "specified keyword, plus integer if digits".

http://lists.w3.org/Archives/Public/www-style/2013Aug/0278.html

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 791983 [details] [diff] [review]
patch v2a, parse text-orientation, text-combine-horizontal

Oops -- I somehow missed the last chunk of comment 15.  Looks like that would've answered most of my questions after that point -- sorry about that. (and thanks for the additional explanation).

Just one nit on the "digits 2" change in the final patch, then:

>+++ b/layout/style/nsCSSValue.cpp
>+    case eCSSProperty_text_combine_horizontal:
>+      if (intValue <= NS_STYLE_TEXT_COMBINE_HORIZ_DIGITS_2) {
>+        aResult.AppendLiteral("digits 2");

Before, you had a "nsCSSProps::LookupPropertyValue(aProperty, intValue)" call here.  You still need that, for the values less than DIGITS_2 (for "none" and "all").

You probably need to preserve your old patch's LookupPropertyValue call, but make it a check for "<= NS_STYLE_TEXT_COMBINE_HORIZ_ALL" instead of "<= NS_STYLE_TEXT_COMBINE_HORIZ_DIGITS_2", and add a new case for DIGITS_2.

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+  if (tch <= NS_STYLE_TEXT_COMBINE_HORIZ_DIGITS_2) {
>+    val->SetString(NS_LITERAL_STRING("digits 2"));

Same here.

r=me with that fixed.

Comment 23

[John Daggett (:jtd)]

Pushed to inbound, including changes based on review comments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc04a661a1a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

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

Might make sense to put "digits 1" in the invalid_values list in property_database.js.  (r=me on that if you want to do it)

Comment 26

[John Daggett (:jtd)]

Attachment: patch, add extra invalid test value

As suggested above.  r=dbaron

Comment 27

[John Daggett (:jtd)]

Pushed follow-on patch to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d65f8868640

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d65f8868640

Comment 29

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

Thanks.

Comment 30

[Sebastian Zartner [:sebo]]

Both properties are now documented:
https://developer.mozilla.org/en-US/docs/Web/CSS/text-orientation
https://developer.mozilla.org/en-US/docs/Web/CSS/text-combine-upright (see bug 997006)

Sebastian

Comment 31

[yisibl]

Update: https://developer.mozilla.org/en-US/docs/Web/CSS/text-combine-upright