Last Comment Bug 534963 - (mathml-dir) [MathML3] Mechanisms for controlling the Directionality of layout
(mathml-dir)
: [MathML3] Mechanisms for controlling the Directionality of layout
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Frédéric Wang (:fredw)
:
: Anthony Jones (:kentuckyfriedtakahe, :k17e)
Mentors:
http://www.w3.org/TR/MathML3/chapter3...
Depends on:
Blocks: mathml-3 208309
  Show dependency treegraph
 
Reported: 2009-12-15 13:06 PST by Frédéric Wang (:fredw)
Modified: 2011-12-25 06:30 PST (History)
4 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add a directionality flag on MathML frames (27.79 KB, patch)
2010-01-03 11:47 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Overall Directionality of formulas (first version) (22.96 KB, patch)
2010-01-05 02:22 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Screenshot of maghreb test (18.35 KB, image/png)
2010-01-05 02:24 PST, Frédéric Wang (:fredw)
no flags Details
Testcase Overall Directionality (13.06 KB, application/xhtml+xml)
2010-01-05 02:25 PST, Frédéric Wang (:fredw)
no flags Details
Screenshot of testcase 420054 (63.13 KB, image/png)
2010-01-05 02:26 PST, Frédéric Wang (:fredw)
no flags Details
Testcase Overall Directionality (13.29 KB, application/xhtml+xml)
2010-01-06 08:10 PST, Frédéric Wang (:fredw)
no flags Details
Overall Directionality of formulas (second version) (31.11 KB, patch)
2010-01-06 08:13 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Overall Directionality of formulas (third version) (31.12 KB, patch)
2010-01-19 13:20 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add a directionality flag on MathML frames (V2) (26.84 KB, patch)
2010-06-12 06:11 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Overall Directionality of formulas (V4) (29.26 KB, patch)
2010-06-12 06:12 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add a directionality flag on MathML frames (V3) (26.85 KB, patch)
2010-08-12 13:20 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Overall Directionality of formulas (V5) (29.29 KB, patch)
2010-08-12 13:20 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add a directionality flag on MathML frames (V4) (26.69 KB, patch)
2011-09-15 12:48 PDT, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Splinter Review
Overall Directionality of formulas (V6) (26.11 KB, patch)
2011-09-15 12:49 PDT, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Splinter Review
Overall Directionality of formulas (V7) (26.12 KB, patch)
2011-10-13 00:52 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add a directionality flag on MathML frames (V5) (26.69 KB, patch)
2011-10-13 00:55 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add a directionality flag on MathML frames (V5) (13.88 KB, patch)
2011-11-08 12:45 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Overall Directionality of formulas (V7) (25.68 KB, patch)
2011-12-07 16:46 PST, Frédéric Wang (:fredw)
karlt: feedback+
Details | Diff | Splinter Review
Overall Directionality of formulas (V8) (34.53 KB, patch)
2011-12-08 11:13 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Add a directionality flag on MathML frames (V6) (14.10 KB, patch)
2011-12-08 13:03 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
reftests (21.03 KB, patch)
2011-12-10 11:33 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
testcase bug mfrac (986 bytes, text/html)
2011-12-11 02:41 PST, Frédéric Wang (:fredw)
no flags Details
Make lspace/rspace in mo behave as leading/trailing spaces (29.74 KB, patch)
2011-12-11 23:43 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add MathML reftests for lspace/rspace in dir=rtl. r=karlt (2.60 KB, patch)
2011-12-11 23:44 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Add MathML reftests for dir=rtl. r=karlt (23.10 KB, patch)
2011-12-11 23:48 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Make lspace/rspace in mo behave as leading/trailing spaces (V2) (29.72 KB, patch)
2011-12-17 03:03 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Add MathML reftests for lspace/rspace in dir=rtl. (V2) (3.28 KB, patch)
2011-12-17 03:05 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add a directionality flag on MathML frames (V7) (13.93 KB, patch)
2011-12-17 03:08 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Overall Directionality of formulas (V9) (34.00 KB, patch)
2011-12-17 03:09 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add MathML reftests for dir=rtl. r=karlt (23.68 KB, patch)
2011-12-18 00:28 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Add MathML reftests for lspace/rspace in dir=rtl. (V3) (3.90 KB, patch)
2011-12-18 00:30 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2009-12-15 13:06:57 PST
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) 2010-01-03 11:47:20 PST
Created attachment 419831 [details] [diff] [review]
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) 2010-01-05 02:22:48 PST
Created attachment 420052 [details] [diff] [review]
Overall Directionality of formulas (first version)

This patch applies on top of attachment 419831 [details] [diff] [review].
Comment 3 Frédéric Wang (:fredw) 2010-01-05 02:24:17 PST
Created attachment 420053 [details]
Screenshot of maghreb test
Comment 4 Frédéric Wang (:fredw) 2010-01-05 02:25:40 PST
Created attachment 420054 [details]
Testcase Overall Directionality
Comment 5 Frédéric Wang (:fredw) 2010-01-05 02:26:32 PST
Created attachment 420055 [details]
Screenshot of testcase 420054
Comment 6 Frédéric Wang (:fredw) 2010-01-05 02:42:42 PST
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) 2010-01-06 08:10:10 PST
Created attachment 420334 [details]
Testcase Overall Directionality

Updated testcase
Comment 8 Frédéric Wang (:fredw) 2010-01-06 08:13:35 PST
Created attachment 420335 [details] [diff] [review]
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) 2010-01-19 13:20:58 PST
Created attachment 422393 [details] [diff] [review]
Overall Directionality of formulas (third version)
Comment 10 Frédéric Wang (:fredw) 2010-01-19 13:21:34 PST
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) 2010-06-12 06:11:38 PDT
Created attachment 450847 [details] [diff] [review]
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) 2010-06-12 06:12:08 PDT
Created attachment 450848 [details] [diff] [review]
Overall Directionality of formulas (V4)
Comment 13 Frédéric Wang (:fredw) 2010-08-12 13:20:01 PDT
Created attachment 465353 [details] [diff] [review]
Add a directionality flag on MathML frames (V3)
Comment 14 Frédéric Wang (:fredw) 2010-08-12 13:20:41 PDT
Created attachment 465355 [details] [diff] [review]
Overall Directionality of formulas (V5)
Comment 15 Frédéric Wang (:fredw) 2011-08-03 13:27:34 PDT
(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] 2011-08-25 12:24:55 PDT
(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] 2011-08-25 15:02:41 PDT
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) 2011-09-15 12:48:26 PDT
Created attachment 560434 [details] [diff] [review]
Add a directionality flag on MathML frames (V4)
Comment 19 Frédéric Wang (:fredw) 2011-09-15 12:49:39 PDT
Created attachment 560435 [details] [diff] [review]
Overall Directionality of formulas (V6)
Comment 20 Frédéric Wang (:fredw) 2011-09-15 12:53:24 PDT
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] 2011-09-30 05:14:55 PDT
The patches here need to be updated because of the elimination of the PRBool data type.
Comment 22 Frédéric Wang (:fredw) 2011-10-13 00:52:55 PDT
Created attachment 566756 [details] [diff] [review]
Overall Directionality of formulas (V7)

fix conflicts mentioned in comment 21...
Comment 23 Frédéric Wang (:fredw) 2011-10-13 00:55:00 PDT
Created attachment 566758 [details] [diff] [review]
Add a directionality flag on MathML frames  (V5)
Comment 25 Karl Tomlinson (back Dec 13 :karlt) 2011-11-07 16:01:26 PST
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) 2011-11-08 12:45:07 PST
Created attachment 572961 [details] [diff] [review]
Add a directionality flag on MathML frames (V5)
Comment 27 Karl Tomlinson (back Dec 13 :karlt) 2011-11-28 20:44:36 PST
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) 2011-11-29 01:20:56 PST
(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 (back Dec 13 :karlt) 2011-11-29 01:25:09 PST
Oh, I should have noticed that, thanks.
Comment 30 Karl Tomlinson (back Dec 13 :karlt) 2011-12-05 18:42:58 PST
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) 2011-12-07 08:02:15 PST
> 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 (back Dec 13 :karlt) 2011-12-07 15:00:53 PST
(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) 2011-12-07 16:46:07 PST
Created attachment 579898 [details] [diff] [review]
Overall Directionality of formulas (V7)

What about this?
Comment 34 Frédéric Wang (:fredw) 2011-12-07 17:01:06 PST
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 (back Dec 13 :karlt) 2011-12-07 17:39:39 PST
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) 2011-12-08 11:13:00 PST
Created attachment 580110 [details] [diff] [review]
Overall Directionality of formulas (V8)
Comment 37 Frédéric Wang (:fredw) 2011-12-08 11:16:18 PST
(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) 2011-12-08 13:03:20 PST
Created attachment 580161 [details] [diff] [review]
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 (back Dec 13 :karlt) 2011-12-08 17:37:27 PST
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 (back Dec 13 :karlt) 2011-12-08 17:51:31 PST
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 (back Dec 13 :karlt) 2011-12-08 17:56:25 PST
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) 2011-12-09 00:53:23 PST
(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 2011-12-10 07:50:25 PST
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
Comment 44 Mozilla RelEng Bot 2011-12-10 08:00:56 PST
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
Comment 45 Mozilla RelEng Bot 2011-12-10 08:10:26 PST
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
Comment 46 Mozilla RelEng Bot 2011-12-10 08:20:26 PST
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
Comment 47 Mozilla RelEng Bot 2011-12-10 08:30:27 PST
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
Comment 48 Mozilla RelEng Bot 2011-12-10 08:40:24 PST
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
Comment 49 Mozilla RelEng Bot 2011-12-10 08:40:29 PST
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
Comment 50 Mozilla RelEng Bot 2011-12-10 08:50:27 PST
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
Comment 51 Mozilla RelEng Bot 2011-12-10 08:50:32 PST
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
Comment 52 Mozilla RelEng Bot 2011-12-10 09:00:44 PST
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
Comment 53 Mozilla RelEng Bot 2011-12-10 09:00:48 PST
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
Comment 54 Mozilla RelEng Bot 2011-12-10 09:10:27 PST
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
Comment 55 Mozilla RelEng Bot 2011-12-10 09:10:31 PST
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
Comment 56 Mozilla RelEng Bot 2011-12-10 09:20:24 PST
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
Comment 57 Mozilla RelEng Bot 2011-12-10 09:20:29 PST
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
Comment 58 Mozilla RelEng Bot 2011-12-10 09:30:27 PST
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
Comment 59 Mozilla RelEng Bot 2011-12-10 09:30:32 PST
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
Comment 60 Mozilla RelEng Bot 2011-12-10 09:40:25 PST
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
Comment 61 Mozilla RelEng Bot 2011-12-10 09:40:29 PST
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
Comment 62 Mozilla RelEng Bot 2011-12-10 09:50:35 PST
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
Comment 63 Mozilla RelEng Bot 2011-12-10 09:50:43 PST
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
Comment 64 Mozilla RelEng Bot 2011-12-10 10:00:55 PST
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
Comment 65 Mozilla RelEng Bot 2011-12-10 10:00:59 PST
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
Comment 66 Mozilla RelEng Bot 2011-12-10 10:10:27 PST
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
Comment 67 Mozilla RelEng Bot 2011-12-10 10:10:31 PST
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
Comment 68 Mozilla RelEng Bot 2011-12-10 10:20:25 PST
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
Comment 69 Mozilla RelEng Bot 2011-12-10 10:20:29 PST
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
Comment 70 Mozilla RelEng Bot 2011-12-10 10:30:24 PST
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
Comment 71 Mozilla RelEng Bot 2011-12-10 10:30:28 PST
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
Comment 72 Mozilla RelEng Bot 2011-12-10 10:40:25 PST
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
Comment 73 Mozilla RelEng Bot 2011-12-10 10:40:29 PST
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
Comment 74 Mozilla RelEng Bot 2011-12-10 10:50:27 PST
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
Comment 75 Mozilla RelEng Bot 2011-12-10 10:50:31 PST
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
Comment 76 Mozilla RelEng Bot 2011-12-10 11:00:48 PST
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
Comment 77 Mozilla RelEng Bot 2011-12-10 11:10:26 PST
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
Comment 78 Mozilla RelEng Bot 2011-12-10 11:20:25 PST
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
Comment 79 Mozilla RelEng Bot 2011-12-10 11:30:27 PST
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
Comment 80 Frédéric Wang (:fredw) 2011-12-10 11:32:27 PST
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) 2011-12-10 11:33:32 PST
Created attachment 580663 [details] [diff] [review]
reftests
Comment 82 Mozilla RelEng Bot 2011-12-10 11:40:24 PST
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
Comment 83 Mozilla RelEng Bot 2011-12-10 11:50:27 PST
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
Comment 84 Mozilla RelEng Bot 2011-12-10 12:00:47 PST
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
Comment 86 Frédéric Wang (:fredw) 2011-12-11 02:41:31 PST
Created attachment 580728 [details]
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) 2011-12-11 23:43:39 PST
Created attachment 580837 [details] [diff] [review]
Make lspace/rspace in mo behave as leading/trailing spaces
Comment 88 Frédéric Wang (:fredw) 2011-12-11 23:44:39 PST
Created attachment 580838 [details] [diff] [review]
Add MathML reftests for lspace/rspace in dir=rtl. r=karlt
Comment 89 Frédéric Wang (:fredw) 2011-12-11 23:47:20 PST
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) 2011-12-11 23:48:53 PST
Created attachment 580839 [details] [diff] [review]
Add MathML reftests for dir=rtl. r=karlt
Comment 91 Karl Tomlinson (back Dec 13 :karlt) 2011-12-14 18:45:39 PST
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 (back Dec 13 :karlt) 2011-12-14 19:42:45 PST
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) 2011-12-15 23:54:09 PST
(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 (back Dec 13 :karlt) 2011-12-16 19:08:01 PST
Yes, that looks good to me.  r=me with that change.
Comment 95 Frédéric Wang (:fredw) 2011-12-17 03:03:30 PST
Created attachment 582508 [details] [diff] [review]
Make lspace/rspace in mo behave as leading/trailing spaces (V2)
Comment 96 Frédéric Wang (:fredw) 2011-12-17 03:05:00 PST
Created attachment 582509 [details] [diff] [review]
Add MathML reftests for lspace/rspace in dir=rtl. (V2)
Comment 97 Frédéric Wang (:fredw) 2011-12-17 03:08:43 PST
Created attachment 582510 [details] [diff] [review]
Add a directionality flag on MathML frames (V7)
Comment 98 Frédéric Wang (:fredw) 2011-12-17 03:09:16 PST
Created attachment 582511 [details] [diff] [review]
Overall Directionality of formulas (V9)
Comment 99 Frédéric Wang (:fredw) 2011-12-17 03:18:19 PST
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 (back Dec 13 :karlt) 2011-12-17 17:30:38 PST
(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) 2011-12-18 00:28:18 PST
Created attachment 582649 [details] [diff] [review]
Add MathML reftests for dir=rtl. r=karlt
Comment 102 Frédéric Wang (:fredw) 2011-12-18 00:30:57 PST
Created attachment 582650 [details] [diff] [review]
Add MathML reftests for lspace/rspace in dir=rtl. (V3)

Note You need to log in before you can comment on or make changes to this bug.