Closed Bug 1193488 Opened 8 years ago Closed 8 years ago

Update the values of writing-mode and text-orientation to follow the latest CSS WG consensus


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox44 --- fixed


(Reporter: jfkthame, Assigned: jfkthame)


(Blocks 1 open bug)


(Keywords: dev-doc-complete)


(2 files)

It looks like the WG is converging on Koji/Fantasai's proposal to eliminate text-orientation:sideways-left, and introduce new sideways-rl and sideways-lr values for writing-mode. So we'll want to update Gecko to match this.
Here's a patch to update the values; not flagging for review just yet, pending confirmation of the CSS WG decision.
Assignee: nobody → jfkthame
Blocks: 1193519
At the Paris F2F last week the CSS WG resolved to do this:
Attachment #8646572 - Flags: review?(dholbert)
Attachment #8646573 - Flags: review?(dholbert)
Note: it looks like the spec has already been updated with this change. The "Changes" section now has:
> Removed sideways-left value of text-orientation and added
> sideways-lr and sideways-rl values to writing-mode.

Here's a link to the current 'writing-mode' property definition with the new values:
Comment on attachment 8646572 [details] [diff] [review]
Update values of writing-mode and text-orientation to reflect the CSS WG decision to revise them in the Writing Modes spec

Review of attachment 8646572 [details] [diff] [review]:

::: layout/base/nsLayoutUtils.cpp
@@ +6613,5 @@
>  {
> +  uint8_t writingMode = aStyleContext->StyleVisibility()->mWritingMode;
> +  switch (writingMode) {
> +    return 0;

So, from looking at the documentation* for the other flags returned by this function, I wonder if we should really be returning TEXT_ORIENT_HORIZONTAL instead of 0 here, to be a bit more explicit & consistent? (Its value is 0, so it's the same thing -- but it's a bit more explicit perhaps, and makes this case statement a bit more grokkable perhaps.)

Then, the only cases here that would explicitly return the unnamed value "0" would be NS_NOTREACHED cases -- and everything else would return a human-readable named bitmask.  That seems nice.


::: layout/generic/WritingModes.h
@@ +478,5 @@
> +        break;
> +
> +        mWritingMode = eOrientationMask |
> +                       eSidewaysMask;

The documentation for "eSidewaysMask" (used here) is becoming out of date. The documentation (elsewhere in this file) currently says:
     eSidewaysMask    = 0x20, // true means text-orientation is sideways-*,
                              // which means we'll use alphabetic instead of
                              // centered default baseline for vertical text

The bit about "text-orientation" there is not necessarily true now, since here we're setting this bit without even inspecting 'text-orientation'. Could you update the documentation to match reality?

::: layout/style/nsCSSDataBlock.cpp
@@ +202,5 @@
>      // unless we have an inline axis property.
>      mozilla::css::Side side;
>      if (isBlock) {
>        uint8_t wm = aRuleData->mStyleContext->StyleVisibility()->mWritingMode;

This bit-clearing is a little mysterious here. It looks like we're only doing it to satisfy an assertion inside of WritingMode::PhysicalSideForBlockAxis (which is what we call right after this line).  And really, that assertion is just no longer valid (given that we have a broader range of possible mWritingMode values now).

Could you just move this bit-clearing to happen *inside* of WritingMode::PhysicalSideForBlockAxis, so that it doesn't make special restrictive requirements about mWritingMode values that are passed to it? (and adjust/drop its assertion as-needed)

::: layout/style/nsCSSKeywordList.h
@@ +483,5 @@
>  CSS_KEY(sepia, sepia)
>  CSS_KEY(serif, serif)
>  CSS_KEY(show, show)
> +CSS_KEY(sideways, sideways)
> +/*CSS_KEY(sideways-lr, sideways_lr)*/

Is there a bug filed on adding "sideways-lr" support? If not, could you file one and include "// XXX bug NNN" (or similar) in the commented-out sideways-lr-related lines in this patch?

(e.g. here, nsLayoutUtils.cpp, nsCSSProps.cpp, and the property_database.js chunk that adds this value to invalid_values since it's not-yet-supported)

::: layout/style/nsStyleConsts.h
@@ +404,5 @@
>  // #define NS_STYLE_WRITING_MODE_HORIZONTAL_BT  2  // hypothetical
> +                                           NS_STYLE_WRITING_MODE_SIDEWAYS_MASK)

This numbering (enum values 0, 1, 2, 3, 4-which-is-not-an-enum-but-actually-a-mask) looks a little fragile -- probably merits a disclaimer of some sort. In particular: if we get a new 'writing-mode' value sometime in the future, it might initially look to the patch-author like they should give the thing an enum value of "5" or larger. But that will potentially cause weird behavior, depending on their choice of value. (Really, this future patch-author should increase the value of this _MASK #define to a higher bit, like 8, I think.)

So: could you add a comment before NS_STYLE_WRITING_MODE_SIDEWAYS_MASK, clarifying this with something like the following:
  // Single-bit flag, used in combination with VERTICAL_LR and _RL.
  // (To avoid ambiguity, this bit must be high enough such that no other
  // values here accidentally use it in their binary representation.)

::: layout/style/test/test_logical_properties.html
@@ +44,5 @@
>  };
> +// Six unique overall writing modes for property-mapping purposes.
> +// Note that text-orientation does not affect these mappings, now that
> +// the sideways-left value no longer exists (superseded in CSS Writing

Maybe s/the sideways-left value/the proposed sideways-left value/?

(to be a little clearer that it's not something we actually implemented, and this is just talking about "exists" in a spec sense, not in an in-our-implementation sense.)

@@ +144,5 @@
>      });
>    });
> +  // Assume that sideways-lr keywords is still not parsed yet
> +  // for writing-mode.  When we start supporting this keyword, the

typo: s/keywords/keyword/

(Might also be worth referencing the bug for sideways-lr here so we'll be more likely to notice & remove this comment when that bug is fixed.)
Attachment #8646572 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #6)

Thanks for the good comments, will update the patch accordingly.

> Is there a bug filed on adding "sideways-lr" support? If not, could you file
> one and include "// XXX bug NNN" (or similar) in the commented-out
> sideways-lr-related lines in this patch?

That's bug 1193519, already in your review queue. :)
Comment on attachment 8646573 [details] [diff] [review]
patch 2 - Update the writing-mode representation printed by debugging code such as DumpFrameTree to reflect the new property values

Review of attachment 8646573 [details] [diff] [review]:

::: layout/generic/WritingModes.h
@@ +536,5 @@
> +    return IsVertical()
> +      ? IsVerticalLR()
> +        ? IsBidiLTR()
> +          ? IsSideways() ? "sw-lr-ltr" : "v-lr-ltr"
> +          : IsSideways() ? "sw-lr-rtl" : "v-lr-rtl"

Maybe it's just me, but I find this many-levels-deep ternary statement (spread out across 9 lines) a bit hard to follow... But on the other hand, I guess writing this as an if/else cascade will either produce a bunch of else-after-return, which violates the style guide -- or (to avoid that else-after-return) it'll end up with a bunch of conceptually-similar cases at different indentation levels, which is also hard-ish to read. So maybe the deeply-nested-ternary is at least as good as any other way of formulating this.

So: *shrug*, despite my initial gut-reaction, I guess this is OK. :)
Attachment #8646573 - Flags: review?(dholbert) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> That's bug 1193519, already in your review queue. :)

Oho! Feel free to dispense with my XXX suggestions, then, if you're confident that bug 1193519 does already update/remove/etc. all the appropriate code/comments.
Also: it looks like there may be a few other pieces of documentation that mention "text-orientation: sideways-left" that still need updating. e.g. I just ran across this:
> 227   /**
> 228    * True if line-over/line-under are inverted from block-start/block-end.
> 229    * This is true when
> 230    *   - writing-mode is vertical-rl && text-orientation is sideways-left
> 231    *   - writing-mode is vertical-lr && text-orientation is not sideways-left
> 232    */
> 233   bool IsLineInverted() const { return !!(mWritingMode & eLineOrientMask); }

Could you do a search for "sideways-left" and make sure to clean up any other stale mentions? This might be easiest as a followup that layers on top of bug 1193519, to avoid bitrotting your patch-stack there. (Sorry if you already caught these somewhere & I missed it.)
Bug 1193488 - Update values of writing-mode and text-orientation to reflect the CSS WG decision to revise them in the Writing Modes spec. r=dholbert
Bug 1193488 - patch 2 - Update the writing-mode representation printed by debugging code such as DumpFrameTree to reflect the new property values. r=dholbert
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1208786
(In reply to Jean-Yves Perrier [:teoli] from comment #14)
> Updated 
> [I tried to
> make an example demonstrating the different writing-mode values live]
> and

Awesome, thanks!

One added detail you might want to include is that the newly-spec'd sideways-{rl,lr} values are supported beginning in Firefox 44 (they were not present in 43 or earlier).

And those values should presumably be included in the /* Keyword values */ list in the Syntax section.
You need to log in before you can comment on or make changes to this bug.