Closed Bug 1141931 Opened 5 years ago Closed 5 years ago

Bidi reordering is not handled properly for ruby

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 2 open bugs, Regressed 1 open bug, )

Details

Attachments

(11 files, 2 obsolete files)

4.61 KB, patch
heycam
: review+
Details | Diff | Splinter Review
6.24 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
13.46 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
5.03 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
19.68 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
14.02 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.43 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
9.41 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
10.55 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
13.11 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
525 bytes, patch
Details | Diff | Splinter Review
No description provided.
Duplicate of this bug: 1141892
Depends on: 1141928
Depends on: 1143513
No longer depends on: 1143513
Blocks: 1144465
Assignee: nobody → quanxunzhen
Attachment #8579052 - Flags: review?(cam)
Attachment #8579055 - Flags: review?(jfkthame)
Attached patch patch 8 - reorder ruby frames (obsolete) — Splinter Review
Attachment #8579059 - Flags: review?(jfkthame)
Attachment #8579060 - Flags: review?(jfkthame)
Comment on attachment 8579052 [details] [diff] [review]
patch 1 - compute unicode-bidi property

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

::: layout/reftests/w3c-css/submitted/ruby/support/rbc.css
@@ +1,5 @@
>  rbc {
>    display: ruby-base-container;
> +  unicode-bidi: -moz-isolate;
> +  unicode-bidi: -webkit-isolate;
> +  unicode-bidi: isolate;

I'm not certain prefixes other than -moz should be used in submitted tests.  I have a feeling that David's submission script will strip -moz, but I don't remember for sure.

::: layout/style/html.css
@@ +822,5 @@
>    rtc > rt {
>      font-size: inherit;
>    }
> +  ruby, rb, rt, rtc {
> +    unicode-bidi: -moz-isolate;

Oh, that was confusing; we have a -moz-isolate keyword and an isolate keyword (for the isolation property).

Why remove rbc from the rule here?  The UA style sheet in the spec still has it.

::: layout/style/ua.css
@@ +96,1 @@
>    }

Is the addition of these rules just to avoid the style fixup you've added?
(In reply to Cameron McCormack (:heycam) from comment #11)
> Comment on attachment 8579052 [details] [diff] [review]
> patch 1 - compute unicode-bidi property
> 
> Review of attachment 8579052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/html.css
> @@ +822,5 @@
> >    rtc > rt {
> >      font-size: inherit;
> >    }
> > +  ruby, rb, rt, rtc {
> > +    unicode-bidi: -moz-isolate;
> 
> Oh, that was confusing; we have a -moz-isolate keyword and an isolate
> keyword (for the isolation property).
> 
> Why remove rbc from the rule here?  The UA style sheet in the spec still has
> it.

Yes, it is in the spec, but we do not support rbc. rbc is an XHTML tag, and is not defined in any of HTML5 spec. Our html parser doesn't parse that tag properly, and we don't have other rules for rbc in html.css, hence I think it'd better remove it here as well. Actually, it should have been removed when I moved this code here.

> ::: layout/style/ua.css
> @@ +96,1 @@
> >    }
> 
> Is the addition of these rules just to avoid the style fixup you've added?

Yes. If we do not add these rules, the style fixup will happen for every ruby annoymous box, which is kind of unfortunate because it wastes memory. The fixup should now happens only when the author explicitly specifies unsupported values.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #12)
> Yes, it is in the spec, but we do not support rbc. rbc is an XHTML tag, and
> is not defined in any of HTML5 spec. Our html parser doesn't parse that tag
> properly, and we don't have other rules for rbc in html.css, hence I think
> it'd better remove it here as well. Actually, it should have been removed
> when I moved this code here.

Ah, right, I keep forgetting that.  Presumably we don't support rbc in XHTML documents either.

> > ::: layout/style/ua.css
> > @@ +96,1 @@
> > >    }
> > 
> > Is the addition of these rules just to avoid the style fixup you've added?
> 
> Yes. If we do not add these rules, the style fixup will happen for every
> ruby annoymous box, which is kind of unfortunate because it wastes memory.
> The fixup should now happens only when the author explicitly specifies
> unsupported values.

Great, sounds good.
Comment on attachment 8579052 [details] [diff] [review]
patch 1 - compute unicode-bidi property

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

r=me if you confirm with David whether it's appropriate to include those prefixes in the CSS-submitted test.
Attachment #8579052 - Flags: review?(cam) → review+
dbaron, should I include the prefixed values like -moz-isolate and -webkit-isolate in W3C submitted tests?
Flags: needinfo?(dbaron)
We probably don't want them.

But if you need -moz-isolate in the checked-in version in our repository, I can add substitutions to https://hg.csswg.org/test/file/0549a1debf5e/vendor-imports/mozilla/mozilla-central-reftests/sync-tests.sh
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (UTC-7) from comment #16)
> We probably don't want them.
> 
> But if you need -moz-isolate in the checked-in version in our repository, I
> can add substitutions to
> https://hg.csswg.org/test/file/0549a1debf5e/vendor-imports/mozilla/mozilla-
> central-reftests/sync-tests.sh

Actually I don't think it would make any difference to the result whether we use prefixed value or not. The current code just don't rely on that property at all. If you think we'd better revmoe all prefixes, I can do that.
Comment on attachment 8579053 [details] [diff] [review]
patch 2 - eliminate writing-mode mismatch

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

r=me, with nits addressed as you see fit:

::: layout/generic/nsRubyFrame.cpp
@@ +208,5 @@
>  {
>    WritingMode lineWM = aReflowState.mLineLayout->GetWritingMode();
>    LogicalSize availSize(lineWM, aReflowState.AvailableISize(),
>                          aReflowState.AvailableBSize());
> +  WritingMode frameWM = GetWritingMode();

"frameWM" is probably a bit too generic of a variable-name, in a frame method that iterates over other frames and uses 'frameWM' while doing so. [further down].

Maybe call this "rubyWM"? (for symmetry with "rtcWM" which you declare further down)

(Or "rubyContainerWM", if "rubyWM" is still to generic?)

@@ +210,5 @@
>    LogicalSize availSize(lineWM, aReflowState.AvailableISize(),
>                          aReflowState.AvailableBSize());
> +  WritingMode frameWM = GetWritingMode();
> +  NS_ASSERTION(!frameWM.IsOrthogonalTo(lineWM), "Ruby frame should not "
> +               "have orthogonal writing-mode to its line.");

Nit: The indentation is a little off here -- the two string-pieces are all part of the same function-argument, but they're indented by different amounts.

Consider rewriting with the message starting on the 2nd line (and being a tad shorter to all fit in 80 chars), e.g.:
  NS_ASSERTION(!frameWM.IsOrthogonalTo(lineWM),
               "Ruby frame writing-mode should not be orthogonal to its line");

@@ +321,5 @@
>      // they have continuations, because the breaking has already been
>      // handled when reflowing the base containers.
>      NS_ASSERTION(textReflowStatus == NS_FRAME_COMPLETE,
>                   "Ruby text container must not break itself inside");
> +    LogicalSize size = textMetrics.Size(frameWM).ConvertTo(lineWM, frameWM);

It took me a bit to figure out why "frameWM" was the right thing to pass here (to textMetrics.Size).

I think this would be clearer if you declared textMetrics using the other constructor -- the one that takes a writing-mode, instead of a reflow state -- and pass in "frameWM" to that constructor. Then, it'd be clearer that "frameWM" is inextricably tied to textMetrics.

(And as noted above, "frameWM" probably wants to be renamed to "rubyWM" or something else that's more specific.)
Attachment #8579053 - Flags: review?(dholbert) → review+
Attachment #8579056 - Flags: review?(dholbert) → review+
Comment on attachment 8579057 [details] [diff] [review]
patch 6 - move two ruby enumerators to RubyUtils

>+++ b/layout/generic/nsRubyBaseContainerFrame.h
>+#include "RubyUtils.h"
> #include "nsContainerFrame.h"
> 
>-#define RTC_ARRAY_SIZE 1
>-
[...]
>-namespace mozilla {
>-struct RubyColumn;
>-}
>-
>-class nsRubyTextContainerFrame;
>-

It looks like the changes to this file are just replacing some forward-decls (including one added in the previous patch) with an #include here.

That seems like a step in the wrong direction -- we should generally prefer forward-decls to #includes, in .h files, to reduce interdependencies. (I assume you agree, given patch 5 here.)

So: do we really need this #include, or can we revert the changes* here? aside from RTC_ARRAY_SIZE, which is indeed moving & hence needs to be removed.

(Actually -- I think I see why you might need the #include. This header has this, further down:
  typedef nsAutoTArray<nsRubyTextContainerFrame*, RTC_ARRAY_SIZE> AutoTextContainerArray;
which needs to see the RTC_ARRAY_SIZE define, which now lives elsewhere. So as long as that typedef is here, we need RubyUtils.h. The good news, though, is that we don't really need that typedef here -- we can just put it in nsRubyBaseContainerFrame.cpp, which is the only place it's used. With that, I think you should be able to avoid adding an #include here in the header.)

r=me with this addressed.
Attachment #8579057 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #19)
> It looks like the changes to this file are just replacing some forward-decls
> (including one added in the previous patch) with an #include here.
[...]
> r=me with this addressed.

Oh, I guess the next patch removes this #include. Seems a bit odd that we're adding it here, only to remove it in the next patch, but I guess that's fine.  So, r=me on part 6.
Comment on attachment 8579058 [details] [diff] [review]
patch 7 - move rtc array types to RubyUtils with merging code

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

r=me with the following addressed:

::: layout/generic/RubyUtils.h
@@ +21,4 @@
>  
>  namespace mozilla {
>  
> +typedef nsTArray<nsRubyTextContainerFrame*> RubyTextContainerArray;

Is RubyTextContainerArray just used as a more-abstract synonym for AutoRubyTextContainerArray?  Or do we actually instantiate RubyTextContainerArray anywhere?

If we never instantiate it, I'd kinda rather we just use the same type (AutoRubyTextContainerArray) everywhere, to reduce the mental-burden of distinguishing between these different types here.  (Unless that makes things messier somehow; but it doesn't look to me like it would.)

@@ +74,3 @@
>  {
>  public:
> +  AutoRubyTextContainerArray(nsRubyBaseContainerFrame* aBaseContainer);

I'm not sure how I feel about deriving from nsTArray/nsAutoTArray, but I suppose there's a certain elegance to it, and it does make this code cleaner, so it seems good to me.

This class definitely needs some documentation, though. Right now its behavior is a bit magic, unless (or even if) you dive into the .cpp file and read the code to find out what its constructor actually does.
Attachment #8579058 - Flags: review?(dholbert) → review+
Attachment #8579054 - Flags: review?(jfkthame) → review+
Comment on attachment 8579055 [details] [diff] [review]
patch 4 - adjust reorder algorithm

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

This looks OK to me. I'm flagging :smontagu as well, since I think he knows this code better; I'd like to make sure he sees this and has a chance to comment if he wishes.

Simon, feel free to simply clear the flag if you don't feel it's needed, or re-review if you're more comfortable with that. I just didn't want it to bypass you entirely.
Attachment #8579055 - Flags: review?(smontagu)
Attachment #8579055 - Flags: review?(jfkthame)
Attachment #8579055 - Flags: review+
Comment on attachment 8579059 [details] [diff] [review]
patch 8 - reorder ruby frames

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

I feel like nsBidiPresUtils::RepositionFrame is getting quite unwieldy here; can we split the new ruby-frame code out into a subsidiary method? Maybe do something like

  if (IsBidiLeaf(aFrame)) {
    icoord = aFrame->ISize(aContainerWM);
  } else if (aFrame->StyleDisplay()->IsRubyDisplayType()) {
    icoord = RepositionRubyFrames(....);
  } else {
    ...the existing code to handle child frames...
  }

where RepositionRubyFrames() is a new method that basically contains the big block of new code here.

The compiler might decide to inline the RepositionRubyFrames() method anyway, but I think this would make the code more readable.

::: layout/base/nsBidiPresUtils.cpp
@@ +19,5 @@
>  #include "nsBlockFrame.h"
>  #include "nsIFrameInlines.h"
> +#include "RubyUtils.h"
> +#include "nsRubyFrame.h"
> +#include "nsRubyBaseFrame.h"

Is this include required? Offhand I don't see a need for it.

@@ +1460,5 @@
>      icoord += reverseDir ?
>        borderPadding.IStart(frameWM) : borderPadding.IEnd(frameWM);
>    } else {
> +    nsIAtom* frameType = aFrame->GetType();
> +    bool isLTR = frameWM.IsBidiLTR();

Two of the four branches below, including the overwhelmingly more common (non-ruby) case, don't use this; so I'd prefer to do it inside the if-branches that actually want it.
Comment on attachment 8579059 [details] [diff] [review]
patch 8 - reorder ruby frames

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

::: layout/base/nsBidiPresUtils.cpp
@@ +1510,5 @@
> +    } else if (frameType == nsGkAtoms::rubyBaseFrame ||
> +               frameType == nsGkAtoms::rubyTextFrame) {
> +      // Reorder the children.
> +      // XXX It currently doesn't work properly because we do not
> +      // resolve frames inside ruby content frames.

Not only that, but we also seem to render simple RTL text backwards (i.e. LTR) within individual ruby frames. Is there a separate bug for that?
(In reply to Jonathan Kew (:jfkthame) from comment #24)
> Not only that, but we also seem to render simple RTL text backwards (i.e.
> LTR) within individual ruby frames. Is there a separate bug for that?

Never mind, I see it's already filed - sorry for the noise.
Those suggestions sound reasonable. I'll fix them when I'm back from my PTO.
Attachment #8579059 - Attachment is obsolete: true
Attachment #8579059 - Flags: review?(jfkthame)
Attachment #8585235 - Flags: review?(jfkthame)
Comment on attachment 8585235 [details] [diff] [review]
patch 8 - reorder ruby frames

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

Thanks, I like this better.

::: layout/base/nsBidiPresUtils.cpp
@@ +1413,5 @@
> +  MOZ_ASSERT(frameType == nsGkAtoms::rubyFrame ||
> +             frameType == nsGkAtoms::rubyBaseFrame ||
> +             frameType == nsGkAtoms::rubyTextFrame ||
> +             frameType == nsGkAtoms::rubyBaseContainerFrame ||
> +             frameType == nsGkAtoms::rubyTextContainerFrame);

At first glance, it seems odd that this method accepts frames of type rubyTextContainerFrame, but then the conditionals below don't handle them in any way.

I think I understand why this is correct, but it'd be nice to have an explicit comment making it clear that it wasn't just forgotten.
Attachment #8585235 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #23)
> Comment on attachment 8579059 [details] [diff] [review]
> patch 8 - reorder ruby frames
> 
> Review of attachment 8579059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsBidiPresUtils.cpp
> @@ +19,5 @@
> >  #include "nsBlockFrame.h"
> >  #include "nsIFrameInlines.h"
> > +#include "RubyUtils.h"
> > +#include "nsRubyFrame.h"
> > +#include "nsRubyBaseFrame.h"
> 
> Is this include required? Offhand I don't see a need for it.

Forgot to address this. Yes, it is required, because type of |column.mBaseFrame| is |nsRubyBaseFrame*|, and without this include, it is unknown that the type is a subclass of |nsIFrame*| which is required by function |RepositionFrame|.
Attachment #8579060 - Flags: review?(jfkthame) → review+
Comment on attachment 8579054 [details] [diff] [review]
patch 3 - adjust parameters of functions in nsBidiPresUtils

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

The change of aContinuationStates to const pointer is fine, but I would rather not make the change from aContainerSize to aContainerISize. This changed from an nscoord to an nsSize in bug 1131013 as a "temporary fix until bug 1131451 is fixed" and when that fix happens we will need to change all this back because we will need an nsSize to pass to SetRect.
Comment on attachment 8579055 [details] [diff] [review]
patch 4 - adjust reorder algorithm

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

This looks OK to me, assuming it passes try of course
Attachment #8579055 - Flags: review?(smontagu) → review+
(In reply to Simon Montagu :smontagu from comment #30)
> Comment on attachment 8579054 [details] [diff] [review]
> patch 3 - adjust parameters of functions in nsBidiPresUtils
> 
> Review of attachment 8579054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The change of aContinuationStates to const pointer is fine, but I would
> rather not make the change from aContainerSize to aContainerISize. This
> changed from an nscoord to an nsSize in bug 1131013 as a "temporary fix
> until bug 1131451 is fixed" and when that fix happens we will need to change
> all this back because we will need an nsSize to pass to SetRect.

I don't see any reason why we need bsize of the container. Could you explain that? AFAICS, we only need the isize of the line to compute position of frames in rtl direction. We can simply pass the original bsize of the frames to form the rect.
Flags: needinfo?(smontagu)
(In reply to Simon Montagu :smontagu from comment #31)
> Comment on attachment 8579055 [details] [diff] [review]
> patch 4 - adjust reorder algorithm
> 
> Review of attachment 8579055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK to me, assuming it passes try of course

Hmmm... It doesn't pass...
The original patch causes reftest failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c65d9fe2feae
It is fixed in this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cda6abae152a
Attachment #8579055 - Attachment is obsolete: true
Attachment #8588271 - Flags: review?(jfkthame)
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #32)
> I don't see any reason why we need bsize of the container. Could you explain
> that? AFAICS, we only need the isize of the line to compute position of
> frames in rtl direction. We can simply pass the original bsize of the frames
> to form the rect.

The point is that the original argument (before bug 1131013) was container *width*, not container *isize*, which was passed to WritingMode methods like GetPhysicalRect which use an aContainerWidth to calculate coordinates in axes which progress in the opposite direction from normal -- i.e. the block axis in vertical-rl, or the inline axis in rtl. For full support of rtl in vertical writing modes, because the inline axis is the physical y-axis that calculation needs to use the container height, we are going to have to rewrite all those methods to take an |nsSize aContainerSize| so that we can use either the height or width or both as necessary.

It's true that here in RepositionFrame we only need the inline size so we could get away with special-casing it, but when we have the full support for rtl in vertical I'd rather go back to using the standard methods.
Flags: needinfo?(smontagu)
OK, I'll add a followup patch to get it back to nsSize. I'm not going to modify patch 3 directly since the later patches have been depending on that change.
Comment on attachment 8588782 [details] [diff] [review]
patch 10 - Revert to pass container size instead of isize

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

Thanks!
Attachment #8588782 - Flags: review?(smontagu) → review+
Comment on attachment 8588271 [details] [diff] [review]
patch 4 - adjust reorder algorithm

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

::: layout/base/nsBidiPresUtils.cpp
@@ +1432,3 @@
>    LogicalMargin frameMargin = aFrame->GetLogicalUsedMargin(frameWM);
>    LogicalMargin borderPadding = aFrame->GetLogicalUsedBorderAndPadding(frameWM);
> +  // Since the visual order of frame could be difference with the

s/difference with/different from/
Attachment #8588271 - Flags: review?(jfkthame) → review+
Approval Request Comment
[Feature/regressing bug #]: bug 1039006
[User impact if declined]: See "Error in parsing value for 'unicode-bidi'.  Declaration dropped." in browser console
[Describe test coverage new/current, TreeHerder]: not applied
[Risks and why]: no risk, since it just fixes an unsupported value in html.css
[String/UUID change made/needed]: n/a

It is part of patch 1 which has been landed on trunck, hence I don't think it needs any review in addition.
Attachment #8593078 - Flags: approval-mozilla-beta?
Attachment #8593078 - Flags: approval-mozilla-aurora?
Comment on attachment 8593078 [details] [diff] [review]
patch 0 - fix unicode-bidi value in html.css

I approve this single patch. Not all of them.
Should be in 38 beta 5.
Attachment #8593078 - Flags: approval-mozilla-beta?
Attachment #8593078 - Flags: approval-mozilla-beta+
Attachment #8593078 - Flags: approval-mozilla-aurora?
Attachment #8593078 - Flags: approval-mozilla-aurora+
Obviously the status flags for the release branches have a different meaning in this case since only Part 0 is landing on them, but leaving them set to fixed for tracking purposes.
Depends on: 1181890
Regressions: 1569107
You need to log in before you can comment on or make changes to this bug.