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

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? ...

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

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.

Updated testcase

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.

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).

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

(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.

(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.

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>

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.

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

fix conflicts mentioned in comment 21...

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

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 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.

(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.

Oh, I should have noticed that, thanks.

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.)

> 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?

(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?

What about this?

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 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.

(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

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 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 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?

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.

(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.

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-85e996ab494c

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-5590b294619d

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.

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-5590b294619d

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/fred.wang@free.fr-5590b294619d

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

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.

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 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 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?

(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;

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

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.

(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.

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

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

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