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

RESOLVED FIXED in mozilla26

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: dholbert, Assigned: jtd)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla26
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Updated

4 years ago
Blocks: 145503
(Assignee)

Comment 1

4 years ago
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.
Summary: implement CSS parsing of text-orientation property → implement CSS parsing of text-orientation, text-combine-horizontal properties
(Assignee)

Comment 2

4 years ago
Created attachment 783004 [details] [diff] [review]
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.
(Assignee)

Comment 3

4 years ago
Created attachment 783006 [details] [diff] [review]
patch, parse text-orientation, text-combine-horizontal

Fix constant double-defn.
Attachment #783004 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: nobody → jdaggett
(Assignee)

Updated

4 years ago
Blocks: 789104
(Assignee)

Comment 4

4 years ago
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.
Attachment #783006 - Flags: review?(dholbert)
(Reporter)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Created attachment 790631 [details] [diff] [review]
patch, parse text-orientation, text-combine-horizontal

Update to trunk and remove unnecessary 'break;'
Attachment #783006 - Attachment is obsolete: true
Attachment #783006 - Flags: review?(dholbert)
Attachment #790631 - Flags: review?(dholbert)
(Reporter)

Updated

4 years ago
(Reporter)

Comment 7

4 years ago
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.
(Reporter)

Comment 8

4 years ago
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]
(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.)
(Reporter)

Comment 10

4 years ago
(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?)
(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>
(Reporter)

Comment 12

4 years ago
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
(Reporter)

Comment 13

4 years ago
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]
Attachment #790631 - Flags: review?(dholbert) → feedback+
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.
(Assignee)

Comment 15

4 years ago
(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
(Assignee)

Comment 16

4 years ago
(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.
I'd prefer nsChangeHint_ReconstructFrame over NS_STYLE_HINT_FRAMECHANGE, actually; the NS_STYLE_HINT_* values are really only there for backwards-compat.
(Assignee)

Comment 18

4 years ago
Created attachment 791183 [details] [diff] [review]
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.
Attachment #790631 - Attachment is obsolete: true
Attachment #791183 - Flags: review?(dholbert)
(Reporter)

Comment 19

4 years ago
(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.)
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Comment 21

4 years ago
Created attachment 791983 [details] [diff] [review]
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
Attachment #791183 - Attachment is obsolete: true
Attachment #791183 - Flags: review?(dholbert)
Attachment #791983 - Flags: review?(dholbert)
(Reporter)

Comment 22

4 years ago
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.
Attachment #791983 - Flags: review?(dholbert) → review+
(Assignee)

Comment 23

4 years ago
Pushed to inbound, including changes based on review comments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc04a661a1a
https://hg.mozilla.org/mozilla-central/rev/dbc04a661a1a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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)
(Assignee)

Comment 26

4 years ago
Created attachment 792673 [details] [diff] [review]
patch, add extra invalid test value

As suggested above.  r=dbaron
Attachment #792673 - Flags: review+
(Assignee)

Comment 27

4 years ago
Pushed follow-on patch to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d65f8868640
https://hg.mozilla.org/mozilla-central/rev/1d65f8868640
Thanks.
(Assignee)

Updated

3 years ago
Depends on: 997006
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete

Comment 31

2 years ago
Update: https://developer.mozilla.org/en-US/docs/Web/CSS/text-combine-upright
You need to log in before you can comment on or make changes to this bug.