Closed Bug 1193519 Opened 5 years ago Closed 4 years ago

implement the sideways-lr value for writing-mode

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(12 files, 7 obsolete files)

13.48 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
6.04 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
6.36 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
6.42 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.96 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.37 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
5.92 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
8.41 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.34 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
26.43 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
5.60 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.70 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Does this need to be behind a separate pref from the rest of vertical text?  Or is it already in shippable or near-shippable condition?
(In reply to David Baron [:dbaron] ⏰UTC-4 (busy Aug. 8-Aug. 30) from comment #6)
> Does this need to be behind a separate pref from the rest of vertical text? 
> Or is it already in shippable or near-shippable condition?

I'm hoping it can land in a pretty-much-shippable condition as soon as the CSS WG confirms the decision on revising writing-mode and text-orientation (bug 1193488). I need to write some testcases, and there are a couple of issues I'm aware of that still need work (text-align is flipped, and text-decoration lines are misplaced) but it's close to ready.
Update to part 1 -- for sideways-lr mode, inline frame reordering needs to happen even if there's no actual bidi.
Attachment #8646588 - Attachment is obsolete: true
Attachment #8646898 - Attachment is obsolete: true
Attachment #8646589 - Attachment is obsolete: true
Attachment #8646590 - Attachment is obsolete: true
Attachment #8646591 - Attachment is obsolete: true
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Rebased part 4 to current mozilla-central tip.
Attachment #8647679 - Attachment is obsolete: true
Attachment #8647676 - Flags: review?(dholbert)
Attachment #8647677 - Flags: review?(dholbert)
Attachment #8647678 - Flags: review?(dholbert)
Attachment #8657050 - Flags: review?(dholbert)
Attachment #8647680 - Flags: review?(dholbert)
Attachment #8647681 - Flags: review?(dholbert)
Attachment #8647682 - Flags: review?(dholbert)
Attachment #8647750 - Flags: review?(dholbert)
Have you tested that float and clear (both physical and logical values) behave sensibly for sideways-lr?  (I remember there were a bunch of issues with that the last time we were dealing with the float manager.  I think many of them have gone away now that it's no longer possible to switch between sideways-rl and sideways-lr within the same block formatting context, but I'm not sure they *all* went away.)
Flags: needinfo?(jfkthame)
Comment on attachment 8647676 [details] [diff] [review]
pt 1 - Update coordinate conversions in WritingModes.h to account for sideways-lr writing mode

Review of attachment 8647676 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8647676 - Flags: review?(dholbert) → review+
Comment on attachment 8647677 [details] [diff] [review]
pt 2 - Handle sideways-left orientation in gfx text-drawing code

Review of attachment 8647677 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +1734,5 @@
>      gfxFloat& inlineCoord = aFontParams.isVerticalFont ? aPt->y : aPt->x;
>  
>      if (aRunParams.spacing) {
> +        inlineCoord += aRunParams.isRTL ? -aRunParams.spacing[0].mBefore
> +                                        : aRunParams.spacing[0].mBefore;

Can you explain the reason for the "direction --> isRTL" conversion here? (Most of the the changes in this patch are in this category.)

Does 'direction' (-1.0 of 1.0) not have the right value for the logic here, if we're sideways-lr? (If so, should we just get rid of .direction entirely? After this patch, I only see one usage remaining in this file, and maybe that needs removing (or at least could be removed) too.)

::: gfx/thebes/gfxFont.h
@@ +966,5 @@
>      }
>  
> +    bool IsSidewaysLeft() const {
> +        return (GetFlags() & gfxTextRunFactory::TEXT_ORIENT_MASK) ==
> +                gfxTextRunFactory::TEXT_ORIENT_VERTICAL_SIDEWAYS_LEFT;

gfxTextRunFactory:: needs to be de-indented by 1 space here, I think. (It's not part of the parenthesized expression, so it shouldn't be aligned inside of the parens.)
(In reply to Daniel Holbert [:dholbert] from comment #22)
> Comment on attachment 8647677 [details] [diff] [review]
> pt 2 - Handle sideways-left orientation in gfx text-drawing code
> 
> Review of attachment 8647677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFont.cpp
> @@ +1734,5 @@
> >      gfxFloat& inlineCoord = aFontParams.isVerticalFont ? aPt->y : aPt->x;
> >  
> >      if (aRunParams.spacing) {
> > +        inlineCoord += aRunParams.isRTL ? -aRunParams.spacing[0].mBefore
> > +                                        : aRunParams.spacing[0].mBefore;
> 
> Can you explain the reason for the "direction --> isRTL" conversion here?
> (Most of the the changes in this patch are in this category.)

In our existing writing-modes, LTR text always runs in the direction of increasing physical coordinates (left-right or top-bottom), and RTL text is the reverse of that. So we don't clearly distinguish between bidi-RTLness (reversal of the inline direction) and inverse direction at logical/physical conversion. But with sideways-lr, we need to make this distinction: so 'isRTL' means we're reversing the progression of glyphs within the line, while 'direction' is applied when converting logical (textrun) to physical coordinates.

So here, when we're adding a spacing amount (or glyph width, etc.) to the inline-axis coordinate, we need to know whether we're dealing with bidi-LTR or bidi-RTL (i.e. .isRTL), not whether that inline-axis will ultimately run "forwards" or "backwards" in physical coordinates (.direction).

Here in gfxFont, we are mostly dealing with the .isRTL kind of directionality, but the .direction field is still used quite a bit by the higher-level code in gfxTextRun, IIRC.
Flags: needinfo?(jfkthame)
Comment on attachment 8647677 [details] [diff] [review]
pt 2 - Handle sideways-left orientation in gfx text-drawing code

OK, thanks. This seems good then; r=me.
Attachment #8647677 - Flags: review?(dholbert) → review+
[Restoring dbaron's ni from comment 20, to be sure that question doesn't get lost]
Flags: needinfo?(jfkthame)
Comment on attachment 8647678 [details] [diff] [review]
pt 3 - Handle writing-mode:sideways-lr in nsTextFrame selection and rendering

>+++ b/layout/generic/nsTextFrame.cpp
>@@ -6582,19 +6583,19 @@ nsTextFrame::GetCharacterOffsetAtFramePo
>-  gfxFloat width = mTextRun->IsVertical() ?
>-    (mTextRun->IsRightToLeft() ? mRect.height - aPoint.y : aPoint.y) :
>-    (mTextRun->IsRightToLeft() ? mRect.width - aPoint.x : aPoint.x);
>+  gfxFloat width = mTextRun->IsVertical()
>+    ? mTextRun->IsInlineReversed() ? mRect.height - aPoint.y : aPoint.y
>+    : mTextRun->IsInlineReversed() ? mRect.width - aPoint.x : aPoint.x;

Could you restore the parens on the last two lines here? (around each operand of the outer ternary statement)

IMO they made this easier to quickly skim.  (There are 3 different ternary statements intertwined here, whose logic could conceivably be mixed in a variety of ways.  Parens make the grouping/structure a little clearer.)

r=me with that.
Attachment #8647678 - Flags: review?(dholbert) → review+
Comment on attachment 8657050 [details] [diff] [review]
pt 4 - Reverse the direction of text-decoration offsets in sideways-lr mode

r=me on part 4, but can you make sure we've got tests for this? (text-decoration drawing with sideways-lr)

I'm not 100% sure where the "0" of our coordinate-system is in this code, so I can't be sure that the "multiply by -1" strategy is sufficient to reverse the decorations. (I can see how it *would* do the right thing, if the coordinate system's choice of "0" is correct, but I also can see how it could easily be broken if "0" is too far away.)

It doesn't look to me like the reftest patches (parts 7 and 8) exercise the various text-decoration values right now. Can you add some tests that do, either here or in a different reftest patch?
Attachment #8657050 - Flags: review?(dholbert) → review+
Comment on attachment 8647681 [details] [diff] [review]
pt 6 - Adjust the position of the caret bidi indicator appropriately for sideways-lr mode

Review of attachment 8647681 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCaret.cpp
@@ +937,5 @@
>        caretBidiLevel = NS_GET_EMBEDDING_LEVEL(aFrame);
>      }
>      bool isCaretRTL = caretBidiLevel % 2;
>      if (isVertical) {
> +      bool isSidewaysLeft = wm.IsVerticalLR() && !wm.IsLineInverted();

Maybe this variable should be named "isSidewaysLR"?

Since...
 (a) I think that's what you're basically checking for here (?)
 (b) "sideways-left" is no longer a term used in CSS (I think?), so it's non-obvious to humans what that term might mean. (whereas "sideways-lr" is defined in the spec)

r=me with that (or with this variable more clearly named/labeled/clarified as you see fit)
Attachment #8647681 - Flags: review?(dholbert) → review+
Comment on attachment 8647682 [details] [diff] [review]
pt 7 - Basic reftests for sideways-lr writing mode

Review of attachment 8647682 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with one nit (below).  (and we also need text-decoration tests here or somewhere, per comment 27)

::: layout/reftests/writing-mode/1193519-sideways-lr-4.html
@@ +18,5 @@
> +
> +<body>
> +
> +<div><span>"One</span> <span>two</span>
> + <span>three</span> <span>four</span> <span>five</span></b>

This </b> tags (and another lower down) are unbalanced -- there are no opening <b> tags for them to be closing. So: please drop them (or balance them if you meant for them to be here).

This applies to the corresponding reference case (4-ref), too.
Attachment #8647682 - Flags: review?(dholbert) → review+
Comment on attachment 8647750 [details] [diff] [review]
pt 8 - Reftest for table row and cell ordering in sideways-lr mode

Review of attachment 8647750 [details] [diff] [review]:
-----------------------------------------------------------------

Side note: should we have tests for sideways-rl row progression, too? (Maybe we already do?)

(I think that's less tricky -- it presumably behaves much like vertical-rl. Still, worth making sure that it actually works with that writing-mode value.)

Anyway, r=me on this patch with the following addressed:

::: layout/reftests/writing-mode/tables/sideways-lr-row-progression-1-ref.html
@@ +29,5 @@
> +  <tr>
> +    <td rowspan=2>
> +    <td class="e">
> +    <td rowspan=2 colspan=2>
> +    <td rowspan=3">

Unbalanced quotes here: 3". Probably drop the quote, since you mostly don't use quotes around rowspan/colspan in this patch?

@@ +32,5 @@
> +    <td rowspan=2 colspan=2>
> +    <td rowspan=3">
> +    <td class="d">
> +  <tr>
> +    <td rowspan=2">

Unbalanced quotes.

@@ +41,5 @@
> +    <td class="g">
> +    <td rowspan=2>
> +  <tr>
> +    <td class="a">
> +    <td colspan=3">

Unbalanced quotes.

::: layout/reftests/writing-mode/tables/sideways-lr-row-progression-1a.html
@@ +37,5 @@
> +      <td class="b">
> +      <td colspan=2>
> +  <tfoot>
> +    <tr>
> +      <td colspan=2> 

Drop trailing whitespace on this ^ line.  (This is present in the "b" variant of this test, too.)

@@ +50,5 @@
> +      <td class="f">
> +      <td rowspan=2 colspan=2>
> +    <tr>
> +      <td class="g">
> +  <tbody>

This table has two <tbody> tags -- is that a mistake? (This is present in the "b" variant of this test, too.)

::: layout/reftests/writing-mode/tables/sideways-lr-row-progression-1b.html
@@ +29,5 @@
> +  .g { background: yellow }
> +  .h { background: orange }
> +</style>
> +
> +<table dir=rtl class="test">

The description of this test (in its <meta> tag) says "...per 'direction'", but we actually end up using "dir" here. (not 'direction')

Can we just add "direction: rtl" to the .test { ... } class in this testcase's <style> block, and drop the dir attribute?
Attachment #8647750 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #30)
> Comment on attachment 8647750 [details] [diff] [review]
> pt 8 - Reftest for table row and cell ordering in sideways-lr mode
> 
> Review of attachment 8647750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Side note: should we have tests for sideways-rl row progression, too? (Maybe
> we already do?)
> 
> (I think that's less tricky -- it presumably behaves much like vertical-rl.
> Still, worth making sure that it actually works with that writing-mode
> value.)

Yes, it's identical to vertical-rl as far as table layout is concerned. But I'll add a testcase with that writing-mode value too, for completeness.


> @@ +50,5 @@
> > +      <td class="f">
> > +      <td rowspan=2 colspan=2>
> > +    <tr>
> > +      <td class="g">
> > +  <tbody>
> 
> This table has two <tbody> tags -- is that a mistake? (This is present in
> the "b" variant of this test, too.)

It's intended to verify that successive table row groups (i.e. tbody elements) are ordered properly, in addition to the ordering of the table rows *within* each group (tbody).

I agree it looks like a possible mistake, though, partly because of all the implicitly-closed elements that are being used. For better clarity, I've added explicit </tr>, </tbody>, </thead> and </tfoot> tags in these tests, leaving only the <td> elements depending on implicit closing.
(In reply to David Baron [:dbaron] ⌚UTC-4 (busy, returning September 22) from comment #20)
> Have you tested that float and clear (both physical and logical values)
> behave sensibly for sideways-lr?  (I remember there were a bunch of issues
> with that the last time we were dealing with the float manager.  I think
> many of them have gone away now that it's no longer possible to switch
> between sideways-rl and sideways-lr within the same block formatting
> context, but I'm not sure they *all* went away.)

I thought I'd seen this work OK, but on further testing it turns out floats don't quite work correctly in sideways-lr: they "float" nicely to the expected side of the containing block, but something's wrong with how we're tracking the available flow area, and the following content fails to wrap beside the float; it's as if it always specified 'clear:both'.

So it looks like we need an additional patch here to fix whatever's confused in the float manager.
Flags: needinfo?(jfkthame)
The problem with floats in sideways-lr is caused by LogicalRect::LineLeft/LineRight returning the wrong values. This was an error in the part 1 patch here; while it's correct to test IsInlineReversed() in place of IsBidiLTR() in physical-coordinate accessors, this isn't true for LineLeft() and LineRight(); line-left in writing-mode:sideways-lr will always be physical bottom, while inline-start depends on bidi directionality. So this reverts one chunk of the part 1 patch (I'll fold it in before landing), and fixes the behavior of floats. Reftests coming up next...
Attachment #8663148 - Flags: review?(dholbert)
These are sideways-lr versions of a set of the float torture-tests originally from bug 1114329.
Attachment #8663150 - Flags: review?(dholbert)
(In reply to Jonathan Kew (:jfkthame) from comment #34)
> These are sideways-lr versions of a set of the float torture-tests
> originally from bug 1114329.

It looks like these are all slightly-tweaked copies of existing test files -- so it probably makes sense to use 'hg cp' (followed by minor tweaks) to generate them, I think?

You could, for example, generate a "hg cp"-based patch by:
  - Backing up all of the new tests somewhere.
  - Pop off this patch (removing these new tests from your tree).
  - Run "hg cp $oldTestFile $newTestFile" to generate all the new test files as copies of existing tests (possibly in a scripted way).
  - ...and then finally, restore all your backup files, stomping on these fresh copies.

At that point, if you generate a new patch file with 'hg commit' / 'hg qnew', it'll only show your actual tweaks.
Flags: needinfo?(jfkthame)
(It looks like the test files, at least, are just tweaked versions of the to the rtl-vlr test files. The reference cases are a bit more different, as one might expect, so 'hg cp' may make less sense for those.)
Yes, exactly: these are copies of the *-vlr tests with writing-mode changed, and references modified as needed. Here's a version of the patch that uses 'hg cp' to track that history better.
Attachment #8663260 - Flags: review?(dholbert)
Attachment #8663150 - Attachment is obsolete: true
Attachment #8663150 - Flags: review?(dholbert)
Flags: needinfo?(jfkthame)
Here's a simple reftest for text-decoration in sideways-lr mode. As it compares sideways-lr with horizontal text rotated using transform(), I was expecting a certain amount of fuzz; a pixel-perfect comparsion seemed unlikely. Quite pleasantly surprised that it only needs an annotation on a couple of platforms, actually.
Attachment #8663550 - Flags: review?(dholbert)
Comment on attachment 8663273 [details] [diff] [review]
pt 10 - Clean up remaining mentions of 'sideways-left' in code comments

r=me, but one nit on the nsImageFrame comment-removal:

>+++ b/layout/generic/nsImageFrame.cpp
>@@ -1182,17 +1182,17 @@ nsImageFrame::DisplayAltText(nsPresConte
[...]
>       if (isVertical) {
>-        x = pt.x + maxDescent; // XXX will need update for sideways-left
>+        x = pt.x + maxDescent;

Does this code need adjustment to work with 'sideways-lr', or does alt-text work correctly in that writing mode?  (IIRC there's overlap between sideways-left & sideways-lr functionality, so there might still be something that needs fixing here.)

If there's still an update needed here (for sideways-lr now), could you either fix the issue as part of this bug, or preserve the XXX comment with updated language and file a bug on taking care of that?  (Also, this comment suggests that we might want reftests for image alt-text in sideways-lr writing-modes. Maybe file a followup on that, if that's not already covered elsewhere.)
Attachment #8663273 - Flags: review?(dholbert) → review+
Comment on attachment 8663260 [details] [diff] [review]
pt 9 - Reftests with floats in writing-mode: sideways-lr

Thanks for recreating this with 'hg cp'!  r=me
Attachment #8663260 - Flags: review?(dholbert) → review+
Comment on attachment 8663550 [details] [diff] [review]
pt 11 - Reftest for sideways-lr writing mode with text-decoration

r=me
Attachment #8663550 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #42)
> Comment on attachment 8663273 [details] [diff] [review]
> pt 10 - Clean up remaining mentions of 'sideways-left' in code comments
> 
> r=me, but one nit on the nsImageFrame comment-removal:
> 
> >+++ b/layout/generic/nsImageFrame.cpp
> >@@ -1182,17 +1182,17 @@ nsImageFrame::DisplayAltText(nsPresConte
> [...]
> >       if (isVertical) {
> >-        x = pt.x + maxDescent; // XXX will need update for sideways-left
> >+        x = pt.x + maxDescent;
> 
> Does this code need adjustment to work with 'sideways-lr', or does alt-text
> work correctly in that writing mode?  (IIRC there's overlap between
> sideways-left & sideways-lr functionality, so there might still be something
> that needs fixing here.)
> 
> If there's still an update needed here (for sideways-lr now), could you
> either fix the issue as part of this bug, or preserve the XXX comment with
> updated language and file a bug on taking care of that?  (Also, this comment
> suggests that we might want reftests for image alt-text in sideways-lr
> writing-modes. Maybe file a followup on that, if that's not already covered
> elsewhere.)

I tried image alt-text in sideways-lr, and AFAICT it worked fine without any adjustment here; the alt-text appeared on the same baseline as surrounding text (including when I gave it a wildly different font size), so I don't think there's anything more needed here.

(The comment was rather speculative in the first place, I suppose, given that we hadn't started on sideways-left implementation and were not entirely clear exactly how it would be handled.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a24bb175f76ea9de2e595b2c187ac2350ac7d81
Bug 1193519 pt 1 - Update coordinate conversions in WritingModes.h to account for sideways-lr writing mode. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f93dd4e09ea999d9e5d12fbe51fa7d028faf3fd
Bug 1193519 pt 2 - Handle sideways-left orientation in gfx text-drawing code. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/1047cad5fcc6c6347ffa1b42dee72ba4d99fa2f5
Bug 1193519 pt 3 - Handle writing-mode:sideways-lr in nsTextFrame selection and rendering. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a33040782452700ea3b3f0bee449567f1d1fc7
Bug 1193519 pt 4 - Reverse the direction of text-decoration offsets in sideways-lr mode. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/73190c69c21068aa4126b93c57fbbdac43e40f4c
Bug 1193519 pt 5 - Expose the sideways-lr value for writing-mode to CSS. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/0975a176af5df03e19bd12257e87d186608291eb
Bug 1193519 pt 6 - Adjust the position of the caret bidi indicator appropriately for sideways-lr mode. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbc088dbeedc63c02e5bf1017644cdeacc520971
Bug 1193519 pt 7 - Basic reftests for sideways-lr writing mode. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/b42f31f18095609bdedd3e0ef9d33da14ec7a77b
Bug 1193519 pt 8 - Reftests for table row and cell ordering in sideways-* writing modes. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4404bee6fa0e97523a6367ef0f4800a163d0d8
Bug 1193519 pt 9 - Reftests with floats in writing-mode: sideways-lr. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/76080d610d00395b0ff8bc2fd3929833c2717f17
Bug 1193519 pt 10 - Clean up remaining mentions of 'sideways-left' in code comments. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/15a1872dd4fd32ecddfc07516fea89328a003505
Bug 1193519 pt 11 - Reftest for sideways-lr writing mode with text-decoration. r=dholbert
Depends on: 1311208
You need to log in before you can comment on or make changes to this bug.