534963 - (mathml-dir) [MathML3] Mechanisms for controlling the Directionality of layout

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Frédéric Wang (:fredw)]

http://www.w3.org/TR/MathML3/chapter3.html#presm.bidi
MathML 3 introduces mechanisms for controlling the Directionality of layout.
See also bug 208309.

Comment 1

[Frédéric Wang (:fredw)]

Attachment: Add a directionality flag on MathML frames

Here is a first patch that simply parses the "dir" attribute and allows to set/inherit the directionality flags on each MathML frame. Now we need to take into account this dir flag, as explained in the MathML 3 spec:

- We apply the bidi algorithm on each individual token element. According to the spec, "The important thing to notice is that the bidirectional algorithm is applied independently to the contents of each token element; each token element is an independent run of characters. This is in contrast to the application of bidirectionality to HTML, where the algorithm applies to the entire sequence of characters within each block level element.". A token element can only contain text and possibly <mglyph/> or <malignmark/> that have neutral directionality. I suppose we can use the work that already exists for the bidi algorithm in Gecko. I would appreciate if someone from the internationalization team explains how to do that...

- We must also modify the layout of some frame to change the "overall directionality" from right to left when needed. Maybe one way to do that is to use a shared operation that modify the horizontal position of a component of the frame: given the width w of a component, its left position dx and the width W of the frame, it will return the "mirrored" position W - w - dx. Then use this operation in the Place functions. I think this could work for most presentation markup. However more work will probably be needed: for example mirroring the sqrt symbols, modify css dir for mtable? ...

Comment 2

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (first version)

This patch applies on top of attachment 419831 [details] [diff] [review].

Comment 3

[Frédéric Wang (:fredw)]

Attachment: Screenshot of maghreb test

Comment 4

[Frédéric Wang (:fredw)]

Attachment: Testcase Overall Directionality

Comment 5

[Frédéric Wang (:fredw)]

Attachment: Screenshot of testcase 420054

Comment 6

[Frédéric Wang (:fredw)]

It seems that there are already some things that work for bidi on token elements, as one can see on attachment 420053 [details] (which is the screenshot of the test at http://www.w3.org/Math/testsuite/build/main/Topics/BiDi/Complex/Maghreb1-simple.xhtml). However, we still need to implement mirroring on <mo>'s. For the moment I see two possibilities:
- extend the work of bug 414277 to allow negative scale transform
- add support for arabic fonts. We can probably use the work of Dadzilla: http://www.ucam.ac.ma/fssm/rydarab/dadzilla.htm (I was not able to find their sources, though)

For the overall directionality on formulas, I wrote a testcase (attachment 420054 [details]). I've started a patch (attachment 420052 [details] [diff] [review]) that already adds support for many of the constructions. I'm going to continue to work on this before looking to bidi on token elements.

Comment 7

[Frédéric Wang (:fredw)]

Attachment: Testcase Overall Directionality

Updated testcase

Comment 8

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (second version)

Second version of the patch for overall directionality. Normally, RTL works for all elements. I forgot to say that it depends on other patches, such that the one of bug 118743.

Comment 9

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (third version)

Comment 10

[Frédéric Wang (:fredw)]

I updated the patch for overall directionality (attachment 422393 [details] [diff] [review]). It applies on top of attachment 419831 [details] [diff] [review], which applies itself on top of attachment 421661 [details] [diff] [review]. The work that remains to do is to use bidi for token elements (bug 208309).

Comment 11

[Frédéric Wang (:fredw)]

Attachment: Add a directionality flag on MathML frames (V2)

Just make patches compatible with attachment 441538 [details] [diff] [review].

Comment 12

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (V4)

Comment 13

[Frédéric Wang (:fredw)]

Attachment: Add a directionality flag on MathML frames (V3)

Comment 14

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (V5)

Comment 15

[Frédéric Wang (:fredw)]

(In reply to comment #14)
> Created attachment 465355 [details] [diff] [review] [diff] [details] [review]
> Overall Directionality of formulas (V5)

We don't need to apply MirrorIfRTL in munder/mover/munderover, since the attribute "align" takes values left/center/right which do not depend on directionality.

Comment 16

[Bill Gianopoulos [:WG9s]]

(In reply to Frédéric Wang (:fred) from comment #15)
> (In reply to comment #14)
> > Created attachment 465355 [details] [diff] [review]
> > Overall Directionality of formulas (V5)
> 
> We don't need to apply MirrorIfRTL in munder/mover/munderover, since the
> attribute "align" takes values left/center/right which do not depend on
> directionality.

This patch has bit-rotted.  It references GetLastChild, which no longer exists.

Comment 17

[Bill Gianopoulos [:WG9s]]

Here is the actual error:

/home/wag/mozilla/mozilla2/layout/mathml/nsMathMLmfencedFrame.cpp:406:39: error: no matching function for call to ‘nsMathMLmfencedFrame::GetLastChild(long int)’
/home/wag/mozilla/mozilla2/layout/mathml/../generic/nsIFrame.h:1041:13: note: candidate is: nsIFrame* nsIFrame::GetLastChild(nsIFrame::ChildListID) const <near match>

Comment 18

[Frédéric Wang (:fredw)]

Attachment: Add a directionality flag on MathML frames (V4)

Comment 19

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (V6)

Comment 20

[Frédéric Wang (:fredw)]

I'm not sure how much the patches have changed since the last time I attached them, so in the doubt I attach the last versions from my patch series.

Comment 21

[Bill Gianopoulos [:WG9s]]

The patches here need to be updated because of the elimination of the PRBool data type.

Comment 22

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (V7)

fix conflicts mentioned in comment 21...

Comment 23

[Frédéric Wang (:fredw)]

Attachment: Add a directionality flag on MathML frames (V5)

Comment 24

[Frédéric Wang (:fredw)]

https://raw.github.com/fred-wang/MozillaCentralPatches/master/mathml_overall_directionality.diff
https://raw.github.com/fred-wang/MozillaCentralPatches/master/mathml_directionality.diff

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 560434 [details] [diff] [review]
Add a directionality flag on MathML frames (V4)

This looks like it should work fine, but I suspect that some things are
unnecessary and so can be removed.

The UpdatePresentationData(FromChildAt) and PropagatePresentationData* methods
are needed for displaystyle and compression flags, I suspect, because of some
complex logic in some situations.  However, the directionality attribute is
simpler and handled through InheritAutomaticData only.  These PresentationData methods are not called with the directionality flags.  There is therefore no need to add their element-specific overrides.  An assertion in nsMathMLFrame::UpdatePresentationData that aWhichFlags does not contain any flags beyond displaystyle and compression would help clarify this.

I expect NS_MATHML_EXPLICIT_DIRECTIONALITY would then also be unnecessary.

> nsMathMLmoFrame::InheritAutomaticData(nsIFrame* aParent)
> {
>   // retain our native direction, it only changes if our text content changes
>   nsStretchDirection direction = mEmbellishData.direction;
>   nsMathMLTokenFrame::InheritAutomaticData(aParent);
>   mEmbellishData.direction = direction;
>+
>+  // see if the directionality attribute is there
>+  nsMathMLFrame::FindAttrDirectionality(mContent, mPresentationData);

nsMathMLTokenFrame::InheritAutomaticData() has already called
FindAttrDirectionality(), so this call should be unnecessary.

I wonder whether nsMathMLmstyleFrame should derive from nsMathMLmrowFrame, but
no need to do this now.

Comment 26

[Frédéric Wang (:fredw)]

Attachment: Add a directionality flag on MathML frames (V5)

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 560435 [details] [diff] [review]
Overall Directionality of formulas (V6)

>   if (IsToDraw(NOTATION_RADICAL)) {
>+    nscoord &dx =
>+      NS_MATHML_IS_RTL(mPresentationData.flags) ? dx_right : dx_left;
>+    
>     if (aWidthOnly) {
>       nscoord radical_width = mMathMLChar[mRadicalCharIndex].
>         GetMaxWidth(PresContext(), aRenderingContext);
>       
>       // Update horizontal parameters
>-      dx_left = NS_MAX(dx_left, radical_width);
>+      dx = NS_MAX(dx, radical_width);
>     } else {
>       // Stretch the radical symbol to the appropriate height if it is not
>       // big enough.
>       nsBoundingMetrics contSize = bmBase;
>       contSize.ascent = mRuleThickness;
>       contSize.descent = bmBase.ascent + bmBase.descent + psi;
> 
>       // height(radical) should be >= height(base) + psi + mRuleThickness
>       mMathMLChar[mRadicalCharIndex].Stretch(PresContext(), aRenderingContext,
>                                              NS_STRETCH_DIRECTION_VERTICAL,
>                                              contSize, bmRadicalChar,
>                                              NS_STRETCH_LARGER);
>       mMathMLChar[mRadicalCharIndex].GetBoundingMetrics(bmRadicalChar);
> 
>       // Update horizontal parameters
>-      dx_left = NS_MAX(dx_left, bmRadicalChar.width);
>+      dx = NS_MAX(dx, bmRadicalChar.width);
> 
>       // Update vertical parameters
>       radicalAscent = bmBase.ascent + psi + mRuleThickness;
>       radicalDescent = NS_MAX(bmBase.descent,
>                               (bmRadicalChar.ascent + bmRadicalChar.descent -
>                                radicalAscent));
> 
>       mBoundingMetrics.ascent = NS_MAX(mBoundingMetrics.ascent,

This |dx| doesn't seem to be used, so I'm struggling to work out what is happening here.

Comment 28

[Frédéric Wang (:fredw)]

(In reply to Karl Tomlinson (:karlt) from comment #27)

> This |dx| doesn't seem to be used, so I'm struggling to work out what is
> happening here.

dx is not an nscoord but a reference to an nscoord. It is used to update dx_right or dx_left when we take into account the width of the radical symbol. Maybe a comment describing what it is doing can be added or I can use if statements on NS_MATHML_IS_RTL at the two places where we update dx_right / dx_left.

Comment 29

[Karl Tomlinson (:karlt)]

Oh, I should have noticed that, thanks.

Comment 30

[Karl Tomlinson (:karlt)]

Comment on attachment 560435 [details] [diff] [review]
Overall Directionality of formulas (V6)

Most of this looks good, thanks.

nsMathMLContainerFrame::Place() calculates the bounding metrics of the row,
but currently it is not reflecting the position the children, and so the
bounding metrics of the first and last frames are not considered correctly.

I suspect RowChildFrameIterator::X() should return the reflected position.

(I wonder whether nsMathMLTokenFrame::Place() may also have trouble as
nsBoundingMetrics::operator+= always adds to the right.  I don't know when
token frames may have multiple children though, so no need to sort that out in
this patch.  I wonder whether there are multiple child frames when the text is
of mixed direction.)

>+math[dir="rtl"], mstyle[dir="rtl"], mrow[dir="rtl"],
>+mi[dir="rtl"], mn[dir="rtl"], mo[dir="rtl"],
>+mtext[dir="rtl"], mspace[dir="rtl"], ms[dir="rtl"] {
>+    direction: rtl;
>+}
>+
>+math[dir="ltr"], mstyle[dir="ltr"], mrow[dir="ltr"],
>+mi[dir="ltr"], mn[dir="ltr"], mo[dir="ltr"],
>+mtext[dir="ltr"], mspace[dir="ltr"], ms[dir="ltr"] {
>+    direction: ltr;
>+}

Is this unnecessary for mspace?
The dir attribute "has no effect on mspace".
http://www.w3.org/TR/MathML3/chapter3.html#presm.commatt

>   // Helper method which positions child frames as an <mrow> on given baseline
>   // y = aBaseline starting from x = aOffsetX, calling FinishReflowChild()
>   // on the frames.
>   void
>-  PositionRowChildFrames(nscoord aOffsetX, nscoord aBaseline);
>+  PositionRowChildFrames(nscoord aParentWidth,
>+                         nscoord aOffsetX, nscoord aBaseline);

Can we change the name of aOffsetX, please, to make it clearer that this is a
leading offset rather than a left offset?  Perhaps "aLeadingX".

>+    nscoord &dx =
>+      NS_MATHML_IS_RTL(mPresentationData.flags) ? dx_right : dx_left;

Can we call this something like dx_leading, please?
And how about making it a pointer instead of a reference?
That should generate the same code, and I'd be more likely to understand it
first time.

>+  PRBool aRTL = NS_MATHML_IS_RTL(mPresentationData.flags);

Please call this "rtl" as the a- prefix is used to indicate variables that are
parameters (arguments) passed in to the function.  (Or alternatively, move the
NS_MATHML_IS_RTL() expression to the nsDisplayMathMLSlash constructor
parameter list, on a new line.)

Comment 31

[Frédéric Wang (:fredw)]

> nsMathMLContainerFrame::Place() calculates the bounding metrics of the row,
> but currently it is not reflecting the position the children, and so the
> bounding metrics of the first and last frames are not considered correctly.
> 
> I suspect RowChildFrameIterator::X() should return the reflected position.

I'm not sure how you want to do that? I think we need to know the container width to reflect the child position? Or you want to iterate from the last to the first child?

Comment 32

[Karl Tomlinson (:karlt)]

(In reply to Frédéric Wang from comment #31)
> > I suspect RowChildFrameIterator::X() should return the reflected position.
> 
> I'm not sure how you want to do that? I think we need to know the container
> width to reflect the child position?

An yes, RowChildFrameIterator is used to calculate the width, so we can't provide the width.

> Or you want to iterate from the last to the first child?

Yes, that should work I think.  Is that feasible?

Comment 33

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (V7)

What about this?

Comment 34

[Frédéric Wang (:fredw)]

Comment on attachment 579898 [details] [diff] [review]
Overall Directionality of formulas (V7)

>-    PositionRowChildFrames(dx_left, aDesiredSize.ascent);
>+    PositionRowChildFrames(NS_MATHML_IS_RTL(mPresentationData.flags) ?
>+                           dx_right : dx_left,
>+                           aDesiredSize.ascent);

OK, this should always be dx_left, now. Sorry.

Comment 35

[Karl Tomlinson (:karlt)]

Comment on attachment 579898 [details] [diff] [review]
Overall Directionality of formulas (V7)

Yes, looks good.  In nsMathMLmpaddedFrame::Reflow, lspace should be leading
space.  (mLeftSpace is now a bad name.)
lspace/dx now needs to be mirrored for the PositionRowChildFrames call.

>     // Remove left correction in <msqrt> because the sqrt glyph itself is
>     // there first.
>     if (mParentFrame->GetContent()->Tag() == nsGkAtoms::msqrt_) {
>       mX = 0;
>     }

I think this makes sense only for LTR.
Perhaps remove mItalicCorrection after the last child frame when RTL?

>-      dx_left = NS_MAX(dx_left, radical_width);
>+     *dx_leading = NS_MAX(*dx_leading, radical_width);
>     } else {

Indent but one more space here.

Comment 36

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (V8)

Comment 37

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang from comment #26)
> Created attachment 572961 [details] [diff] [review]
> Add a directionality flag on MathML frames (V5)

It seems the assertion in nsMathMLFrame::FindAttrDirectionality is raised.
https://tbpl.mozilla.org/?tree=Try&rev=da844e5f3f4e

Comment 38

[Frédéric Wang (:fredw)]

Attachment: Add a directionality flag on MathML frames (V6)

The merror element also use nsMathMLmrowFrame but can not have a dir attribute. That's why an assertion was raised.

I've also updated the code nsMathMLToken, to ignore the case of mspace.

Comment 39

[Karl Tomlinson (:karlt)]

Comment on attachment 580110 [details] [diff] [review]
Overall Directionality of formulas (V8)

All looks right to me, thanks.

>-  if (mLeftSpaceSign != NS_MATHML_SIGN_INVALID) { // there was padding on the left
>-    // dismiss the left italic correction now (so that our parent won't correct us)
>-    mBoundingMetrics.leftBearing = 0;
>+  if (mLeadingSpaceSign != NS_MATHML_SIGN_INVALID) {
>+    if (!NS_MATHML_IS_RTL(mPresentationData.flags)) {
>+      // there was padding on the left. dismiss the left italic correction now
>+      // (so that our parent won't correct us)
>+      mBoundingMetrics.leftBearing = 0;
>+    } else {
>+      // there was padding on the right. dismiss the right italic correction now
>+      // (so that our parent won't correct us)
>+      mBoundingMetrics.width = width;
>+      mBoundingMetrics.rightBearing = width;
>+    }

What do you think about altering the condition instead of duplicating the code to make the adjustments? e.g.

  if ((NS_MATHML_IS_RTL(mPresentationData.flags) ?
       mWidthSign : mLeadingSpaceSign) != NS_MATHML_SIGN_INVALID) {
    // dismiss the left italic correction now (so that our parent won't correct us)
    mBoundingMetrics.leftBearing = 0;
  }

Comment 40

[Karl Tomlinson (:karlt)]

Comment on attachment 580161 [details] [diff] [review]
Add a directionality flag on MathML frames (V6)

(In reply to Frédéric Wang from comment #38)
> The merror element also use nsMathMLmrowFrame but can not have a dir
> attribute. That's why an assertion was raised.

It would seem useful to be able to specify dir on an merror element, but perhaps you could say that about other attributes also, and yes, the spec doesn't include merror as an element with a dir attribute.

> I've also updated the code nsMathMLToken, to ignore the case of mspace.

I doubt it would do much harm to check the dir attribute here, but I see the
assertion would have to keep mspace.

>+  if (mContent->Tag() == nsGkAtoms::mi_ ||
>+      mContent->Tag() == nsGkAtoms::mn_ ||
>+      mContent->Tag() == nsGkAtoms::mo_ ||
>+      mContent->Tag() == nsGkAtoms::mtext_ ||
>+      mContent->Tag() == nsGkAtoms::ms_) {

It would be simpler to have (mContent->Tag() != nsGkAtoms::mspace_).
Would that be appropriate?

Comment 41

[Karl Tomlinson (:karlt)]

I guess some changes will be needed to make lspace and rspace on mo behave as leading and trailing space, but the work here does not need to wait for that.

Comment 42

[Frédéric Wang (:fredw)]

(In reply to Karl Tomlinson (:karlt) from comment #39)
> What do you think about altering the condition instead of duplicating the
> code to make the adjustments? e.g.
> 
>   if ((NS_MATHML_IS_RTL(mPresentationData.flags) ?
>        mWidthSign : mLeadingSpaceSign) != NS_MATHML_SIGN_INVALID) {
>     // dismiss the left italic correction now (so that our parent won't
> correct us)
>     mBoundingMetrics.leftBearing = 0;
>   }

I was afraid that it would be less readable, but that's OK.

(In reply to Karl Tomlinson (:karlt) from comment #40)
> >+  if (mContent->Tag() == nsGkAtoms::mi_ ||
> >+      mContent->Tag() == nsGkAtoms::mn_ ||
> >+      mContent->Tag() == nsGkAtoms::mo_ ||
> >+      mContent->Tag() == nsGkAtoms::mtext_ ||
> >+      mContent->Tag() == nsGkAtoms::ms_) {
> 
> It would be simpler to have (mContent->Tag() != nsGkAtoms::mspace_).
> Would that be appropriate?

That should work, but I found it more reliable, for example if someone adds new tags using nsMathMLTokenFrame later.

(In reply to Karl Tomlinson (:karlt) from comment #41)
> I guess some changes will be needed to make lspace and rspace on mo behave
> as leading and trailing space, but the work here does not need to wait for
> that.

Right, I'll do it in a separate patch.

Comment 43

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 44

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 45

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 46

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 47

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 48

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 49

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 50

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 51

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 52

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 53

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 54

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 55

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 56

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 57

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 58

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 59

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 60

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 61

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 62

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 63

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 64

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 65

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 66

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 67

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 68

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 69

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 70

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 71

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 72

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 73

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 74

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 75

[Mozilla RelEng Bot]

Try run for 85e996ab494c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85e996ab494c
Results (out of 125 total builds):
    exception: 1
    success: 94
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-85e996ab494c

Comment 76

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 77

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 78

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 79

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 80

[Frédéric Wang (:fredw)]

Sorry for the bugspams, I didn't expect that so many messages would be send.

I've written reftests, the result is

https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d

Column spacing of mtable is different in rtl and ltr modes. That does not seem to be the case for HTML tables, though.

Comment 81

[Frédéric Wang (:fredw)]

Attachment: reftests

Comment 82

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 83

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 84

[Mozilla RelEng Bot]

Try run for 5590b294619d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5590b294619d
Results (out of 15 total builds):
    success: 8
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5590b294619d

Comment 85

[Frédéric Wang (:fredw)]

Patches for lspace/rspace:

https://raw.github.com/fred-wang/MozillaCentralPatches/master/mathml-dir-mo-spaces.diff
https://raw.github.com/fred-wang/MozillaCentralPatches/master/mathml-dir-mo-spaces-tests.diff

Comment 86

[Frédéric Wang (:fredw)]

Attachment: testcase bug mfrac

Taking leading/trailing space from the core operator in mfrac is wrong, since these spaces can be added later if there is a parent embellished op. See this testcase.

Comment 87

[Frédéric Wang (:fredw)]

Attachment: Make lspace/rspace in mo behave as leading/trailing spaces

Comment 88

[Frédéric Wang (:fredw)]

Attachment: Add MathML reftests for lspace/rspace in dir=rtl. r=karlt

Comment 89

[Frédéric Wang (:fredw)]

Reftests for mtable (dir-1) and mfrac (dir-6) are failing. I can't see where the problem is for mtable. For mfrac, I guess we should rewrite the way leading/trailing space of the core operator is added.

Comment 90

[Frédéric Wang (:fredw)]

Attachment: Add MathML reftests for dir=rtl. r=karlt

Comment 91

[Karl Tomlinson (:karlt)]

Comment on attachment 580838 [details] [diff] [review]
Add MathML reftests for lspace/rspace in dir=rtl. r=karlt

Don't forget to include these in reftest.list.

Comment 92

[Karl Tomlinson (:karlt)]

Comment on attachment 580837 [details] [diff] [review]
Make lspace/rspace in mo behave as leading/trailing spaces

>-    mBoundingMetrics.leftBearing = leftSpace + bmNum.leftBearing;
>-    mBoundingMetrics.rightBearing =
>-      leftSpace + bmNum.width + mLineRect.width + bmDen.rightBearing;
>+    nscoord leadingSpace2 = leadingSpace + bmNum.leftBearing;
>+    nscoord trailingSpace2 =
>+      leadingSpace + bmNum.width + mLineRect.width + bmDen.rightBearing;
>+    if (NS_MATHML_IS_RTL(mPresentationData.flags)) {
>+      mBoundingMetrics.leftBearing = leadingSpace2;
>+      mBoundingMetrics.rightBearing = trailingSpace2;
>+    } else {
>+      mBoundingMetrics.leftBearing = trailingSpace2;
>+      mBoundingMetrics.rightBearing = leadingSpace2;
>+    }

This doesn't look right to me.  Can you look over this again, please?

Comment 93

[Frédéric Wang (:fredw)]

(In reply to Karl Tomlinson (:karlt) from comment #92)
> This doesn't look right to me.  Can you look over this again, please?

What about this?

     // Set horizontal bounding metrics
-    mBoundingMetrics.leftBearing = leftSpace + bmNum.leftBearing;
-    mBoundingMetrics.rightBearing =
-      leftSpace + bmNum.width + mLineRect.width + bmDen.rightBearing;
+    if (NS_MATHML_IS_RTL(mPresentationData.flags)) {
+      mBoundingMetrics.leftBearing = trailingSpace + bmDen.leftBearing;
+      mBoundingMetrics.rightBearing = trailingSpace + bmDen.width + mLineRect.width + bmNum.rightBearing;
+    } else {
+      mBoundingMetrics.leftBearing = leadingSpace + bmNum.leftBearing;
+      mBoundingMetrics.rightBearing = leadingSpace + bmNum.width + mLineRect.width + bmDen.rightBearing;
+    }
     mBoundingMetrics.width =
-      leftSpace + bmNum.width + mLineRect.width + bmDen.width + rightSpace;
+      leadingSpace + bmNum.width + mLineRect.width + bmDen.width +
+      trailingSpace;

Comment 94

[Karl Tomlinson (:karlt)]

Yes, that looks good to me.  r=me with that change.

Comment 95

[Frédéric Wang (:fredw)]

Attachment: Make lspace/rspace in mo behave as leading/trailing spaces (V2)

Comment 96

[Frédéric Wang (:fredw)]

Attachment: Add MathML reftests for lspace/rspace in dir=rtl. (V2)

Comment 97

[Frédéric Wang (:fredw)]

Attachment: Add a directionality flag on MathML frames (V7)

Comment 98

[Frédéric Wang (:fredw)]

Attachment: Overall Directionality of formulas (V9)

Comment 99

[Frédéric Wang (:fredw)]

I've just launched reftests, but I'm afraid there will be issues for mtable and mfrac in dir=rtl.

https://tbpl.mozilla.org/?tree=Try&rev=d05ba75c0a53

(There is also the bug exhibited by attachment 580728 [details], which already exists in left to right directionality)

We can probably mark some reftests as failing and work on these issues in separate bug entries.

Comment 100

[Karl Tomlinson (:karlt)]

(In reply to Frédéric Wang from comment #99)
> We can probably mark some reftests as failing and work on these issues in
> separate bug entries.

Yes.  If you can move the failing parts of the tests to different files and mark them as failing, that would be great.

Comment 101

[Frédéric Wang (:fredw)]

Attachment: Add MathML reftests for dir=rtl. r=karlt

Comment 102

[Frédéric Wang (:fredw)]

Attachment: Add MathML reftests for lspace/rspace in dir=rtl. (V3)

Comment 103

[Frédéric Wang (:fredw)]

Order of patches:

https://bugzilla.mozilla.org/attachment.cgi?id=582510
https://bugzilla.mozilla.org/attachment.cgi?id=582511
https://bugzilla.mozilla.org/attachment.cgi?id=582649
https://bugzilla.mozilla.org/attachment.cgi?id=582508
https://bugzilla.mozilla.org/attachment.cgi?id=582650

Comment 104

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb4dc394060
https://hg.mozilla.org/integration/mozilla-inbound/rev/26968ecd7e6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/05fee5eb0473
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a7965d14389
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ecebafe90b

Comment 105

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4cb4dc394060
https://hg.mozilla.org/mozilla-central/rev/26968ecd7e6c
https://hg.mozilla.org/mozilla-central/rev/05fee5eb0473
https://hg.mozilla.org/mozilla-central/rev/4a7965d14389
https://hg.mozilla.org/mozilla-central/rev/80ecebafe90b