Closed Bug 772321 Opened 12 years ago Closed 11 years ago

implement CSS parsing of writing-mode property

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jtd, Assigned: reyre)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

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
Blocks: 789096
Assignee: jdaggett → nobody
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?
(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.
[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. :) )]
Assignee: nobody → rick.eyre
Summary: implement CSS parsing of writing-mode, text-orientation properties → implement CSS parsing of writing-mode property
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"?
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)
(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.)
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.
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #755377 - Flags: feedback?(dholbert)
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.
Attachment #755377 - Flags: feedback?(dholbert) → feedback+
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.
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.
Attachment #755377 - Attachment is obsolete: true
Attachment #755577 - Attachment description: Implement Writing Mode Patch Version 1 → Part 1 v1:support "writing-mode" property in style system
It should just go in nsStyleVisibility along with direction.
(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.)
Switched to using nsStyleVisibility struct.
Attachment #755577 - Attachment is obsolete: true
Attachment #756061 - Flags: review?(dbaron)
Did you run the mochitests in layout/style **with the pref enabled**, and do they pass?
SpecialPowers.pushPrevEnv ahould be used rather than depending on the pref being set.
(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).
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]
(er s/pushPrevEnv/pushPrefEnv/)
My intention is "adding" a test with pref enabled. Of course, we should also make sure we don't accidentally ship experimental features.
(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.
[Sorry for the distraction; flagging needinfo=reyre to be sure he didn't miss the question in Comment 15]
Flags: needinfo?(rick.eyre)
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.
Flags: needinfo?(rick.eyre)
I'm in the middle of getting try server access. I'll post once I get try push going for this patch.
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 ?
@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 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
Attachment #756061 - Flags: review?(dbaron) → review+
(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.
(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.
Carrying forward r=dbaron.
Attachment #756061 - Attachment is obsolete: true
And a try with the pref set to enabled:

https://tbpl.mozilla.org/?tree=Try&rev=d59a6ce388e3
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.
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").
Status: NEW → ASSIGNED
Flags: in-testsuite+
Awesome! Thanks for your help Daniel. I'll make sure to change those things when I make a patch next.
https://hg.mozilla.org/mozilla-central/rev/3e2d3f3235b4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: