Bug 534963 (mathml-dir)

[MathML3] Mechanisms for controlling the Directionality of layout

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(6 attachments, 25 obsolete attachments)

13.29 KB, application/xhtml+xml
Details
29.72 KB, patch
karlt
: review+
Details | Diff | Splinter Review
13.93 KB, patch
Details | Diff | Splinter Review
34.00 KB, patch
Details | Diff | Splinter Review
23.68 KB, patch
Details | Diff | Splinter Review
3.90 KB, patch
Details | Diff | Splinter Review
Assignee

Description

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

Comment 1

10 years ago
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? ...
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Assignee

Comment 3

10 years ago
Posted image Screenshot of maghreb test (obsolete) —
Assignee

Comment 4

10 years ago
Posted file Testcase Overall Directionality (obsolete) —
Assignee

Comment 5

10 years ago
Posted image Screenshot of testcase 420054 (obsolete) —
Assignee

Comment 6

10 years ago
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.
Assignee

Comment 7

10 years ago
Updated testcase
Attachment #420054 - Attachment is obsolete: true
Assignee

Comment 8

10 years ago
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.
Attachment #420052 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Depends on: 208309
Assignee

Comment 9

10 years ago
Attachment #420335 - Attachment is obsolete: true
Assignee

Comment 10

10 years ago
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).
Assignee

Updated

9 years ago
Alias: mathml-dir
Assignee

Comment 11

9 years ago
Just make patches compatible with attachment 441538 [details] [diff] [review].
Attachment #419831 - Attachment is obsolete: true
Attachment #422393 - Attachment is obsolete: true
Assignee

Comment 14

9 years ago
Attachment #450847 - Attachment is obsolete: true
Attachment #450848 - Attachment is obsolete: true
Assignee

Comment 15

8 years ago
(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>
Assignee

Comment 18

8 years ago
Attachment #465353 - Attachment is obsolete: true
Attachment #560434 - Flags: review?(karlt)
Assignee

Comment 19

8 years ago
Attachment #560435 - Flags: review?(karlt)
Assignee

Updated

8 years ago
Attachment #465355 - Attachment is obsolete: true
Assignee

Comment 20

8 years ago
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.
Assignee

Updated

8 years ago
Attachment #420053 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #420055 - Attachment is obsolete: true
The patches here need to be updated because of the elimination of the PRBool data type.
Assignee

Comment 22

8 years ago
fix conflicts mentioned in comment 21...
Blocks: 208309
No longer depends on: 208309
Assignee

Updated

8 years ago
Attachment #566756 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #566758 - Attachment is obsolete: true
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.
Attachment #560434 - Flags: review?(karlt) → review-
Assignee

Comment 26

8 years ago
Attachment #572961 - Flags: review?(karlt)
Attachment #572961 - Flags: review?(karlt) → review+
Assignee

Updated

8 years ago
Attachment #560434 - Attachment is obsolete: true
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.
Assignee

Comment 28

8 years ago
(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.)
Attachment #560435 - Flags: review?(karlt) → review-
Assignee

Comment 31

8 years ago
> 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?
Assignee

Comment 33

8 years ago
What about this?
Attachment #560435 - Attachment is obsolete: true
Assignee

Comment 34

8 years ago
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.
Attachment #579898 - Flags: feedback+
Assignee

Comment 36

8 years ago
Attachment #579898 - Attachment is obsolete: true
Attachment #580110 - Flags: review?(karlt)
Assignee

Comment 37

8 years ago
(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
Assignee

Comment 38

8 years ago
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.
Attachment #572961 - Attachment is obsolete: true
Attachment #580161 - Flags: review?(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;
  }
Attachment #580110 - Flags: review?(karlt) → review+
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?
Attachment #580161 - Flags: review?(karlt) → review+
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.
Assignee

Comment 42

8 years ago
(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
Assignee

Comment 80

8 years ago
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.
Assignee

Comment 81

8 years ago
Posted patch reftests (obsolete) — Splinter Review
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
Assignee

Comment 86

8 years ago
Posted file testcase bug mfrac (obsolete) —
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.
Assignee

Comment 89

8 years ago
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.
Assignee

Comment 90

8 years ago
Attachment #580663 - Attachment is obsolete: true
Attachment #580839 - Flags: review?(karlt)
Attachment #580838 - Flags: review?(karlt) → review+
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.
Attachment #580839 - Flags: review?(karlt) → review+
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?
Assignee

Comment 93

8 years ago
(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.
Assignee

Comment 95

8 years ago
Attachment #580837 - Attachment is obsolete: true
Attachment #580837 - Flags: review?(karlt)
Assignee

Comment 96

8 years ago
Attachment #580838 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #580110 - Attachment is obsolete: true
Assignee

Comment 99

8 years ago
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.
Assignee

Comment 101

8 years ago
Attachment #580839 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #580728 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Whiteboard: [checkin: see comment 103]
Assignee

Updated

8 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.