Last Comment Bug 772321 - implement CSS parsing of writing-mode property
: implement CSS parsing of writing-mode property
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla24
Assigned To: Rick Eyre (:reyre)
:
:
Mentors:
http://dev.w3.org/csswg/css3-writing-...
Depends on:
Blocks: writing-mode 789096
  Show dependency treegraph
 
Reported: 2012-07-09 19:40 PDT by John Daggett (:jtd)
Modified: 2013-06-05 03:50 PDT (History)
10 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (8.65 KB, patch)
2013-05-29 07:35 PDT, Rick Eyre (:reyre)
dholbert: feedback+
Details | Diff | Splinter Review
Part 1 v1:support "writing-mode" property in style system (8.98 KB, patch)
2013-05-29 13:28 PDT, Rick Eyre (:reyre)
no flags Details | Diff | Splinter Review
v2: Support "writing-mode" property in style system (8.82 KB, patch)
2013-05-30 10:28 PDT, Rick Eyre (:reyre)
dbaron: review+
Details | Diff | Splinter Review
v3:Support 'writing-mode' property in style system (8.84 KB, patch)
2013-06-03 06:52 PDT, Rick Eyre (:reyre)
no flags Details | Diff | Splinter Review

Description John Daggett (:jtd) 2012-07-09 19:40:01 PDT
Currently defined in CSS3 Writing Modes:

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

writing-mode : horizontal-tb | vertical-rl | vertical-lr

text-orientation : mixed-right | upright | sideways-right | sideways-left | sideways
Comment 1 Daniel Holbert [:dholbert] 2013-05-23 02:45:45 PDT
reyre has expressed interest in working on this. (woot!)

John Daggett suggests focusing just on "writing-mode" for now, since we'll need that sooner and the spec is more stable.

John, would it be all right with you if we just reduced the scope of this bug to just be about writing-mode? (and then spin off a new bug for text-orientation)?

Or would you rather that rick work on writing-mode in a dependent bug?
Comment 2 Daniel Holbert [:dholbert] 2013-05-23 02:48:22 PDT
(In reply to Daniel Holbert [:dholbert] from comment #1)
> John, would it be all right with you if we just reduced the scope of this
> bug to just be about writing-mode? (and then spin off a new bug for
> text-orientation)?
> 
> Or would you rather that rick work on writing-mode in a dependent bug?

Actually, smontagu and I are both leaning towards the former (reduce the scope of this bug, and spin off text-orientation), so I'm just gonna do that.
Comment 3 Daniel Holbert [:dholbert] 2013-05-23 02:52:20 PDT
[spun off Bug 875250 for text-orientation, clarifying summary here, & assigning this to Rick. (Rick, if you change your mind for some reason, no worries -- feel free to unassign yourself. :) )]
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-05-23 03:06:17 PDT
Note that this will need to be behind a pref, off by default (until vertical text is ready to turn on).  Perhaps "layout.vertical-text.enabled"?
Comment 5 Daniel Holbert [:dholbert] 2013-05-23 20:45:26 PDT
make that layout.css.vertical-text.enabled, probably, right? (".css")

Also: What's a reasonable nsStyleStruct to put "writing-mode" in? nsStyleFont?  (note that it's an inherited property)
Comment 6 Daniel Holbert [:dholbert] 2013-05-23 20:52:44 PDT
(In reply to Daniel Holbert [:dholbert] from comment #5)
> Also: What's a reasonable nsStyleStruct to put "writing-mode" in?
> nsStyleFont?

(nsStyleText would probably be better, actually, since I think nsStyleFont tries to map to the "font" shorthand.)
Comment 7 fantasai 2013-05-24 10:14:27 PDT
You want to put writing-mode, direction, and text-orientation all in the same struct, since they all determine the writing mode. Once we do logical margin properties, we'll need them to cascade first before we can cascade the others, so they should be all together, probably in their own struct.
Comment 8 Rick Eyre (:reyre) 2013-05-29 07:35:53 PDT
Created attachment 755377 [details] [diff] [review]
WIP

Current work in progress. Would be helpful to get some feedback if anyone has time.

Daniel suggested that there be two patches. One for adding writing mode to nsStyleText struct and then another to move writing mode to it's own struct, possibly nsStyleWriting? This is a WIP of the first patch.
Comment 9 Daniel Holbert [:dholbert] 2013-05-29 09:39:20 PDT
Comment on attachment 755377 [details] [diff] [review]
WIP

Looks great! Just 2 nits:

>+++ b/layout/style/nsComputedDOMStyle.cpp
>@@ -1,4 +1,4 @@
>-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* -*- ::Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
         ^^
That looks like an unintentional change.

>+++ b/layout/style/nsStyleStruct.cpp
>@@ -2785,7 +2786,8 @@ nsStyleText::nsStyleText(const nsStyleText& aSource)
>     mLetterSpacing(aSource.mLetterSpacing),
>     mLineHeight(aSource.mLineHeight),
>     mTextIndent(aSource.mTextIndent),
>-    mTextShadow(aSource.mTextShadow)
>+    mTextShadow(aSource.mTextShadow),
>+    mWritingMode(aSource.mWritingMode)
> {
>   MOZ_COUNT_CTOR(nsStyleText);
> }
>+++ b/layout/style/nsStyleStruct.h
>@@ -1293,6 +1293,7 @@ struct nsStyleText {
>   uint8_t mHyphens;                     // [inherited] see nsStyleConsts.h
>   uint8_t mTextSizeAdjust;              // [inherited] see nsStyleConsts.h
>   int32_t mTabSize;                     // [inherited] see nsStyleConsts.h
>+  uint8_t mWritingMode;                 // [inherited] see nsStyleConsts.h
> 
>   nscoord mWordSpacing;                 // [inherited]
>   nsStyleCoord  mLetterSpacing;         // [inherited] coord, normal

Compilers (GCC and clang at least) will warn if the constructor init list isn't in the same order as the struct's variable-declarations -- so you should make their ordering match.  (Note that you've currently got mWritingMode several lines after mLetterSpacing in the init list, but several lines before mLetterSpacing in the struct definition.)

It doesn't *hugely* matter, except that this patch will break the build because we have warnings-as-errors turned on for this directory.

So: in the struct definition, I'd bump mWritingMode up one line, to be next to its uint8_t friends (since -- hypothetically if we decide to leave it in this struct, it's better for it to be next to things of the same type, for packing reasons), and then shift it to the corresponding spot in the constructor as well.
Comment 10 Daniel Holbert [:dholbert] 2013-05-29 10:23:44 PDT
Comment on attachment 755377 [details] [diff] [review]
WIP

A few more things I noticed:

>+++ b/layout/base/nsStyleConsts.h
>@@ -542,6 +542,11 @@ static inline mozilla::css::Side operator++(mozilla::css::Side& side, int) {
>+// See nsStyleWriting
>+#define NS_STYLE_WRITING_MODE_HORIZONTAL_TB     0
>+#define NS_STYLE_WRITING_MODE_VERTICAL_LR       1
>+#define NS_STYLE_WRITING_MODE_VERTICAL_RL       2

Technically, as of this patch, that comment should say "see nsStyleText".

(That's pointing to the style struct where those values are used in a member-variable)

>diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h
>@@ -507,6 +508,8 @@ CSS_KEY(upper-roman, upper_roman)
> CSS_KEY(uppercase, uppercase)
> CSS_KEY(vertical, vertical)
> CSS_KEY(vertical-text, vertical_text)
>+CSS_KEY(vertical-lr, vertical_lr)
>+CSS_KEY(vertical-rl, vertical_rl)

Looks like those two new lines actually belong *above* "vertical-text", alphabetically (not below)

>+++ b/layout/style/nsCSSProps.cpp
>@@ -1708,6 +1708,13 @@ const int32_t nsCSSProps::kColumnFillKTable[] = {
>   eCSSKeyword_UNKNOWN, -1
> };
> 
>+const int32_t nsCSSProps::kWritingModeKTable[] = {
>+  eCSSKeyword_horizontal_tb, NS_STYLE_WRITING_MODE_HORIZONTAL_TB,
>+  eCSSKeyword_vertical_lr, NS_STYLE_WRITING_MODE_VERTICAL_LR,
>+  eCSSKeyword_vertical_rl, NS_STYLE_WRITING_MODE_VERTICAL_RL,
>+  eCSSKeyword_UNKNOWN, -1
>+};
[...]
>diff --git a/layout/style/nsCSSProps.h b/layout/style/nsCSSProps.h
>index 32b7d9d..ea194bc 100644
>--- a/layout/style/nsCSSProps.h
>+++ b/layout/style/nsCSSProps.h
>@@ -465,6 +465,7 @@ public:
>   static const int32_t kWindowShadowKTable[];
>   static const int32_t kWordBreakKTable[];
>   static const int32_t kWordWrapKTable[];
>+  static const int32_t kWritingModeKTable[];

So in the .cpp file, this is after kColumnFillKTable, but in the .h file, it's after kWordWrapKTable. Probably worth making those consistent.

Also: I think this patch is missing code in nsRuleNode::ComputeTextData.  That would probably explain some of the mochitest-failures that you're hitting.
Comment 11 Rick Eyre (:reyre) 2013-05-29 13:28:59 PDT
Created attachment 755577 [details] [diff] [review]
Part 1 v1:support "writing-mode" property in style system

This patch implements writing-mode by including it into the nsStyleText struct. I'll be submitting a secondary patch that separates mWritingMode into it's own struct in a few days.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-05-29 23:28:29 PDT
It should just go in nsStyleVisibility along with direction.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-05-29 23:31:02 PDT
(In reply to fantasai from comment #7)
> You want to put writing-mode, direction, and text-orientation all in the
> same struct, since they all determine the writing mode. Once we do logical
> margin properties, we'll need them to cascade first before we can cascade
> the others, so they should be all together, probably in their own struct.

(We already do logical margin properties, and they do fine with direction in nsStyleVisibility, which is the inherited-property partner to nsStyleDisplay.)
Comment 14 Rick Eyre (:reyre) 2013-05-30 10:28:02 PDT
Created attachment 756061 [details] [diff] [review]
v2: Support "writing-mode" property in style system

Switched to using nsStyleVisibility struct.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-05-30 19:57:13 PDT
Did you run the mochitests in layout/style **with the pref enabled**, and do they pass?
Comment 16 Masatoshi Kimura [:emk] 2013-05-30 20:41:23 PDT
SpecialPowers.pushPrevEnv ahould be used rather than depending on the pref being set.
Comment 17 Daniel Holbert [:dholbert] 2013-05-30 21:22:45 PDT
(In reply to Masatoshi Kimura [:emk] from comment #16)
> SpecialPowers.pushPrevEnv ahould be used rather than depending on the pref
> being set.

You mean, we should run the general layout/style mochitests with the pref turned on, by default?  I disagree. (or, at least I think it's a more nuanced question than you're making it out to be)

Suppose we're developing various properties simultaneously, each separately preffed off by default for the time being (with the intention of turning them on down the line, but shipping preffed-off for a while).  Note that this is probably already the case, or will soon be the case.

Is it more worthwhile for TBPL to run our general style tests with all of those prefs turned off (the default, and the configuration that we're shipping to users), or with all of the prefs turned on (a configuration *different* from what we're shipping)?

I'd argue the former is more important to test, so that we don't end up shipping something broken without noticing.  It'd be *nice* to *also* get test-coverage with the pref(s) turned on, but that's less important, and we're likely already getting that from the developers working on the feature testing locally & doing periodic try pushes (with the pref toggled on in a patch at the bottom of their patch-stack).
Comment 18 Daniel Holbert [:dholbert] 2013-05-30 21:34:49 PDT
Of course, for mochitests that are specifically intended to test a particular feature (e.g. the various test_flexbox_* mochitests), it absolutely makes sense to turn on the pref for the duration of that mochitest. But for general-purpose style mochitests, I think we should lean towards testing the configuration that we're shipping. (or at least, ensuring that that configuration gets tested.)

Maybe it'd be worth having the various "test-all-the-properties" mochitests do their work twice; once in the default configuration, and then again in a configuration with a list of specific prefs toggled on (say, all of the current in-developement-but-known-to-parse-correctly properties).

Or we could spin off a new file "preffed_property_database.js" or something, which looks like property_database.js but also includes a pref for each property that needs to be temporarily turned on before we test that property. (And then entries there would eventually graduate to live in property_database.js)

I'm not sure what the correct solution is, but I suspect that just adding a SpecialPowers.pushPrevEnv() isn't what we want.

[This probably should be taken to a dev.platform thread if there's significantly more discussion to be had about it. Alternately, maybe dbaron or someone already has a plan for how this should work]
Comment 19 Daniel Holbert [:dholbert] 2013-05-30 21:35:25 PDT
(er s/pushPrevEnv/pushPrefEnv/)
Comment 20 Masatoshi Kimura [:emk] 2013-05-30 21:46:43 PDT
My intention is "adding" a test with pref enabled. Of course, we should also make sure we don't accidentally ship experimental features.
Comment 21 Daniel Holbert [:dholbert] 2013-05-30 22:11:52 PDT
(In reply to Masatoshi Kimura [:emk] from comment #20)
> My intention is "adding" a test with pref enabled.

Right - so that's not quite what's happening here.  Rick's patch doesn't exactly "add a test" -- it (conditionally) adds some boilerplate, which gets referenced in various *existing* giant tests.  Those existing tests exercise all the boilerplate properties.

I interpreted Comment 16 to be saying "We need to be sure this pref is toggled on for the duration of these giant all-property mochitests, to be sure that this property gets tested". (Please clarify if that's not what you meant.)

And what I'm saying is, I'm not convinced that that's a good idea, in general, because it's possible that in-development properties (when preffed on) could influence the behavior of each other, or influence the behavior of existing properties, in ways that might be desirable but that definitely change behavior.  And so, forcing these prefs on in our tests could lead to us testing different behavior from what we're shipping, and potentially missing bugs/regressions.

I think Rick's current compromise -- adding the boilerplate depending on whether the pref is toggled (with it presumably being toggled in the builds of developers working on this feature) is reasonable.  [It also happens to be the strategy I took for flexbox. :)]

> Of course, we should also
> make sure we don't accidentally ship experimental features.

That's a slightly different concern than the one I was raising, but it's also definitely a valid point.
Comment 22 Daniel Holbert [:dholbert] 2013-05-30 22:52:37 PDT
[Sorry for the distraction; flagging needinfo=reyre to be sure he didn't miss the question in Comment 15]
Comment 23 Rick Eyre (:reyre) 2013-05-31 07:00:24 PDT
Thanks for the needinfo Daniel.

Yep I ran the tests with the pref on and they all seem to pass. Redirected output to a file and searched through it and found all them.
Comment 24 Rick Eyre (:reyre) 2013-06-01 14:15:32 PDT
I'm in the middle of getting try server access. I'll post once I get try push going for this patch.
Comment 25 Siqinbilige 2013-06-01 20:01:40 PDT
I want to write vertical layout testcase for firefox.
So, I want to know howto enable writing-mode in firefox, writing-mode or -moz-writing-mode ? and which version of firefox can be test ?
Comment 26 Daniel Holbert [:dholbert] 2013-06-01 21:56:12 PDT
@Siqinbilige: This bug is _just_ about parsing the CSS property, not about actually making layout actually react to it.

Once this bug gets closed, you'll be able to test the parsing part if you like, by downloading a Firefox nightly build ( http://nightly.mozilla.org/ ) and toggling the about:config pref mentioned in comment 5, which will enable the property (without any -moz prefix) (just the parsing -- no actual layout support yet).

If you want to test the actual layout parts, you'll have to wait until they're implemented, which will happen in a separate bug and will likely take a little while.
Comment 27 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-06-02 18:06:36 PDT
Comment on attachment 756061 [details] [diff] [review]
v2: Support "writing-mode" property in style system

>+// pref to allow vertical text
>+pref("layout.css.vertical-text.enabled", false);
>+

Could you make the comment here more like the comments in the similar preferences above, e.g.:

// Is support for CSS vertical text enabled?


r=dbaron
Comment 28 Siqinbilige 2013-06-02 18:28:13 PDT
(In reply to Daniel Holbert [:dholbert] from comment #26)
> @Siqinbilige: This bug is _just_ about parsing the CSS property, not about
> actually making layout actually react to it.
> 
> Once this bug gets closed, you'll be able to test the parsing part if you
> like, by downloading a Firefox nightly build ( http://nightly.mozilla.org/ )
> and toggling the about:config pref mentioned in comment 5, which will enable
> the property (without any -moz prefix) (just the parsing -- no actual layout
> support yet).
> 
> If you want to test the actual layout parts, you'll have to wait until
> they're implemented, which will happen in a separate bug and will likely
> take a little while.

thank you for you reply.

According your answer and Description, the css command is
writing-mode : horizontal-tb | vertical-rl | vertical-lr; in Firefox.
but, in Microsoft the css command is
writing-mode : lr-tb | tb-rl | tb-lr; in IE

What is the result when put this commands in same css? .
  writing-mode: tb-lr;
  writing-mode: vertical-lr;

The Webkit use it's own keyword -webkit-writing-mode.
Comment 29 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-06-02 19:11:24 PDT
(In reply to Siqinbilige from comment #28)
> What is the result when put this commands in same css? .
>   writing-mode: tb-lr;
>   writing-mode: vertical-lr;

Implementations will skip any declarations they don't understand, so IE will use the one it understands and Gecko will use the one it understands.
Comment 30 Rick Eyre (:reyre) 2013-06-03 06:52:23 PDT
Created attachment 757390 [details] [diff] [review]
v3:Support 'writing-mode' property in style system

Carrying forward r=dbaron.
Comment 31 Rick Eyre (:reyre) 2013-06-03 18:44:44 PDT
https://tbpl.mozilla.org/?tree=Try&rev=82652413c0b5
Comment 32 Rick Eyre (:reyre) 2013-06-04 11:03:23 PDT
And a try with the pref set to enabled:

https://tbpl.mozilla.org/?tree=Try&rev=d59a6ce388e3
Comment 33 Daniel Holbert [:dholbert] 2013-06-04 14:27:24 PDT
Both try pushes seems green (modulo a few unrelated oranges), and you can see writing-mode output in the mochitest-3 output of the one with the pref flipped, e.g.:
{
12:43:01     INFO -  21083 INFO TEST-PASS | /tests/layout/style/test/test_garbage_at_end_of_declarations.html | expected garbage ignored after 'writing-mode: inherit'
12:43:01     INFO -  21084 INFO TEST-PASS | /tests/layout/style/test/test_garbage_at_end_of_declarations.html | expected garbage ignored after 'writing-mode: initial'
12:43:01     INFO -  21085 INFO TEST-PASS | /tests/layout/style/test/test_garbage_at_end_of_declarations.html | expected garbage ignored after 'writing-mode: horizontal-tb'
12:43:01     INFO -  21086 INFO TEST-PASS | /tests/layout/style/test/test_garbage_at_end_of_declarations.html | expected garbage ignored after 'writing-mode: vertical-lr'
12:43:01     INFO -  21087 INFO TEST-PASS | /tests/layout/style/test/test_garbage_at_end_of_declarations.html | expected garbage ignored after 'writing-mode: vertical-rl'
}
from this log: https://tbpl.mozilla.org/php/getParsedLog.php?id=23776158&tree=Try

So, I'll go ahead and land this.
Comment 34 Daniel Holbert [:dholbert] 2013-06-04 14:42:26 PDT
Landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2d3f3235b4

Also -- two nits on the headers of the attached patch, for future reference:
>From f7516652f65dd83cf998ac9a224a2606ee28b662 Mon Sep 17 00:00:00 2001
>From: Rick Eyre <rick.eyre@hotmail.com>

Nit #1: When I hg qimport'ed this patch, it interpreted that top line there ("From f7516...") as being part of the commit message (appended to the "real" commit message, after a newline).

I haven't seen that line in patches before, but it might be worth seeing where that's coming from so that your future patches don't end up with odd cruft at the end of the commit message, when someone else pushes them on your behalf. (I deleted the line in this case, so it didn't end up in this cset.)

>Subject: Bug 772321 -Implement CSS parsing of writing-mode r=dbaron

Nit #2: Theommit message needed a space after the dash there, to be prettier. :)
(I think most gecko commits look like "Bug 123: do stuff" or "Bug 123 - Do stuff", but not "Bug 123 -Do stuff").
Comment 35 Rick Eyre (:reyre) 2013-06-04 17:44:32 PDT
Awesome! Thanks for your help Daniel. I'll make sure to change those things when I make a patch next.
Comment 36 Ed Morley [:emorley] 2013-06-05 03:50:14 PDT
https://hg.mozilla.org/mozilla-central/rev/3e2d3f3235b4

Note You need to log in before you can comment on or make changes to this bug.