Bug 534963 (mathml-dir)

[MathML3] Mechanisms for controlling the Directionality of layout

RESOLVED FIXED in mozilla12

Status

()

Core
MathML
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

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

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

Updated

8 years ago
(Assignee)

Comment 1

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

Comment 2

8 years ago
Created attachment 420052 [details] [diff] [review]
Overall Directionality of formulas (first version)

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

Comment 3

8 years ago
Created attachment 420053 [details]
Screenshot of maghreb test
(Assignee)

Comment 4

8 years ago
Created attachment 420054 [details]
Testcase Overall Directionality
(Assignee)

Comment 5

8 years ago
Created attachment 420055 [details]
Screenshot of testcase 420054
(Assignee)

Comment 6

8 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

8 years ago
Created attachment 420334 [details]
Testcase Overall Directionality

Updated testcase
Attachment #420054 - Attachment is obsolete: true
(Assignee)

Comment 8

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

Updated

8 years ago
Depends on: 208309
(Assignee)

Comment 9

8 years ago
Created attachment 422393 [details] [diff] [review]
Overall Directionality of formulas (third version)
Attachment #420335 - Attachment is obsolete: true
(Assignee)

Comment 10

8 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

7 years ago
Alias: mathml-dir
(Assignee)

Comment 11

7 years ago
Created attachment 450847 [details] [diff] [review]
Add a directionality flag on MathML frames (V2)

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

Comment 12

7 years ago
Created attachment 450848 [details] [diff] [review]
Overall Directionality of formulas (V4)
(Assignee)

Comment 13

7 years ago
Created attachment 465353 [details] [diff] [review]
Add a directionality flag on MathML frames (V3)
(Assignee)

Comment 14

7 years ago
Created attachment 465355 [details] [diff] [review]
Overall Directionality of formulas (V5)
Attachment #450847 - Attachment is obsolete: true
Attachment #450848 - Attachment is obsolete: true
(Assignee)

Comment 15

6 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

6 years ago
Created attachment 560434 [details] [diff] [review]
Add a directionality flag on MathML frames (V4)
Attachment #465353 - Attachment is obsolete: true
Attachment #560434 - Flags: review?(karlt)
(Assignee)

Comment 19

6 years ago
Created attachment 560435 [details] [diff] [review]
Overall Directionality of formulas (V6)
Attachment #560435 - Flags: review?(karlt)
(Assignee)

Updated

6 years ago
Attachment #465355 - Attachment is obsolete: true
(Assignee)

Comment 20

6 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

6 years ago
Attachment #420053 - Attachment is obsolete: true
(Assignee)

Updated

6 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

6 years ago
Created attachment 566756 [details] [diff] [review]
Overall Directionality of formulas (V7)

fix conflicts mentioned in comment 21...
(Assignee)

Comment 23

6 years ago
Created attachment 566758 [details] [diff] [review]
Add a directionality flag on MathML frames  (V5)
Blocks: 208309
No longer depends on: 208309
(Assignee)

Updated

6 years ago
Attachment #566756 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #566758 - Attachment is obsolete: true
(Assignee)

Comment 24

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

Comment 26

6 years ago
Created attachment 572961 [details] [diff] [review]
Add a directionality flag on MathML frames (V5)
Attachment #572961 - Flags: review?(karlt)
Attachment #572961 - Flags: review?(karlt) → review+
(Assignee)

Updated

6 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

6 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

6 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

6 years ago
Created attachment 579898 [details] [diff] [review]
Overall Directionality of formulas (V7)

What about this?
Attachment #560435 - Attachment is obsolete: true
(Assignee)

Comment 34

6 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

6 years ago
Created attachment 580110 [details] [diff] [review]
Overall Directionality of formulas (V8)
Attachment #579898 - Attachment is obsolete: true
Attachment #580110 - Flags: review?(karlt)
(Assignee)

Comment 37

6 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

6 years ago
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.
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

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

Comment 43

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
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

6 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

6 years ago
Created attachment 580663 [details] [diff] [review]
reftests

Comment 82

6 years ago
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

6 years ago
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

6 years ago
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 85

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

Comment 86

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

Comment 87

6 years ago
Created attachment 580837 [details] [diff] [review]
Make lspace/rspace in mo behave as leading/trailing spaces
Attachment #580837 - Flags: review?(karlt)
(Assignee)

Comment 88

6 years ago
Created attachment 580838 [details] [diff] [review]
Add MathML reftests for lspace/rspace in dir=rtl. r=karlt
Attachment #580838 - Flags: review?(karlt)
(Assignee)

Comment 89

6 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

6 years ago
Created attachment 580839 [details] [diff] [review]
Add MathML reftests for dir=rtl. r=karlt
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

6 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

6 years ago
Created attachment 582508 [details] [diff] [review]
Make lspace/rspace in mo behave as leading/trailing spaces (V2)
Attachment #580837 - Attachment is obsolete: true
Attachment #580837 - Flags: review?(karlt)
(Assignee)

Comment 96

6 years ago
Created attachment 582509 [details] [diff] [review]
Add MathML reftests for lspace/rspace in dir=rtl. (V2)
Attachment #580838 - Attachment is obsolete: true
(Assignee)

Comment 97

6 years ago
Created attachment 582510 [details] [diff] [review]
Add a directionality flag on MathML frames (V7)
Attachment #580161 - Attachment is obsolete: true
(Assignee)

Comment 98

6 years ago
Created attachment 582511 [details] [diff] [review]
Overall Directionality of formulas (V9)
(Assignee)

Updated

6 years ago
Attachment #580110 - Attachment is obsolete: true
(Assignee)

Comment 99

6 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.
Attachment #582508 - Flags: review+
(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

6 years ago
Created attachment 582649 [details] [diff] [review]
Add MathML reftests for dir=rtl. r=karlt
Attachment #580839 - Attachment is obsolete: true
(Assignee)

Comment 102

6 years ago
Created attachment 582650 [details] [diff] [review]
Add MathML reftests for lspace/rspace in dir=rtl. (V3)
Attachment #582509 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #580728 - Attachment is obsolete: true
(Assignee)

Comment 103

6 years ago
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
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Whiteboard: [checkin: see comment 103]
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
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [checkin: see comment 103]
Target Milestone: --- → mozilla12
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

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