Last Comment Bug 764996 - Improve how MathML directionality is determined
: Improve how MathML directionality is determined
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
Depends on: 878396
Blocks: 819664 838506 838509
  Show dependency treegraph
 
Reported: 2012-06-14 13:43 PDT by Frédéric Wang (:fredw)
Modified: 2013-09-30 05:38 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Dir attribute mapping (4.09 KB, patch)
2012-12-13 03:35 PST, Cosmin Clapon
fred.wang: feedback+
Details | Diff | Review
Determine directionality (23.84 KB, patch)
2012-12-13 03:38 PST, Cosmin Clapon
fred.wang: feedback+
Details | Diff | Review
Reftest (17.41 KB, patch)
2012-12-13 03:39 PST, Cosmin Clapon
fred.wang: feedback+
Details | Diff | Review
Dir attribute mapping (5.08 KB, patch)
2012-12-13 10:04 PST, Cosmin Clapon
fred.wang: feedback+
Details | Diff | Review
Determine directionality (22.49 KB, patch)
2012-12-13 10:06 PST, Cosmin Clapon
fred.wang: feedback+
Details | Diff | Review
Reftest (2.67 KB, patch)
2012-12-13 10:07 PST, Cosmin Clapon
karlt: review+
fred.wang: feedback+
Details | Diff | Review
Dir attribute mapping (5.07 KB, patch)
2012-12-13 12:05 PST, Cosmin Clapon
karlt: review+
fred.wang: feedback+
Details | Diff | Review
Determine directionality (23.40 KB, patch)
2012-12-13 12:17 PST, Cosmin Clapon
fred.wang: feedback+
Details | Diff | Review
Determine directionality (23.33 KB, patch)
2012-12-14 03:14 PST, Cosmin Clapon
karlt: review-
karlt: feedback+
Details | Diff | Review
Reftest (2.68 KB, patch)
2013-05-23 06:27 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Dir attribute mapping (5.70 KB, patch)
2013-05-23 06:28 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Determine directionality (23.35 KB, patch)
2013-05-23 06:29 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Review
Determine directionality (23.35 KB, patch)
2013-05-23 13:31 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Review

Description Frédéric Wang (:fredw) 2012-06-14 13:43:51 PDT
MathML BiDi has been implemented in bug 534963. The directionality on a frame is determined by NS_MATHML_IS_RTL(mPresentationData.flags) (see attachment 582508 [details] [diff] [review] and attachment 582511 [details] [diff] [review]). We can simplify this a bit by using GetDirection() instead (which gives access to the current CSS "direction" property). Then the code added in attachment 582510 [details] [diff] [review] to set the directionality flag can be removed.

This will also enable people to use the CSS "direction" property to change the directionality of mathematical formulas.

Finally, we can remove the corresponding code from layout/mathml/mathml.css and directly implement the mapping of the dir attribute in nsMathMLElement::MapMathMLAttributesInto.
Comment 1 Pranav Ravichandran [:pranavrc] 2012-06-18 20:11:03 PDT
I'm interested in working on this, I'll assign it to myself if that's okay.
Comment 2 Frédéric Wang (:fredw) 2012-06-19 02:37:18 PDT
(In reply to Pranav Ravichandran [:pranavrc] from comment #1)
> I'm interested in working on this, I'll assign it to myself if that's okay.

Thank you for your interest on this bug. If you need more information, don't hesitate to ask. There is also a IRC #mathml channel.
Comment 3 Frédéric Wang (:fredw) 2012-12-05 22:55:09 PST
Cosmin Clapon would like to work on this bug.
Comment 4 Cosmin Clapon 2012-12-13 03:35:42 PST
Created attachment 691755 [details] [diff] [review]
Dir attribute mapping

Mapped the dir attribute to CSS direction
Comment 5 Cosmin Clapon 2012-12-13 03:38:14 PST
Created attachment 691757 [details] [diff] [review]
Determine directionality

Improved how directionality is determined (via CSS direction value) and removed the code that sets the directionality flag
Comment 6 Cosmin Clapon 2012-12-13 03:39:47 PST
Created attachment 691758 [details] [diff] [review]
Reftest

Compares the rendering with dir="rtl" and direction: rtl (CSS)
Comment 7 Frédéric Wang (:fredw) 2012-12-13 07:31:36 PST
Comment on attachment 691755 [details] [diff] [review]
Dir attribute mapping

Review of attachment 691755 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/mathml/content/src/nsMathMLElement.cpp
@@ +164,5 @@
>    nsIAtom* tag = Tag();
>    if (tag == nsGkAtoms::ms_ || tag == nsGkAtoms::mi_ ||
>        tag == nsGkAtoms::mn_ || tag == nsGkAtoms::mo_ ||
> +      tag == nsGkAtoms::mtext_ || tag == nsGkAtoms::mspace_ ||
> +      tag == nsGkAtoms::mrow_)

I'm wondering if we should just remove nsGkAtoms::mspace_. All the attributes in sTokenStyles, including dir, do not apply to this element.

Note that you are allowing on mrow all the attributes from "tokenMap"...

@@ +169,4 @@
>      return FindAttributeDependence(aAttribute, tokenMap);
>    if (tag == nsGkAtoms::mstyle_ ||
> +      tag == nsGkAtoms::math ||
> +      tag == nsGkAtoms::mrow_)

... and all the attributes from "mstyleMap". This is not correct, 
you only want to add a new "dir" attribute on mrow.

I guess you have to create two new variables:

Element::MappedAttributeEntry sDirStyles[]
static const MappedAttributeEntry* const mrowMap[]

sDirStyles will only have &nsGkAtoms::dir and will be used in "tokenMap", "styleMap" and "mrowMap".

In addition to sDirStyles, "mrowMap" should keep the sCommonPresStyles. Then in nsMathMLElement::IsAttributeMapped, instead of calling FindAttributeDependence(aAttribute, commonPresMap) when tag == nsGkAtoms::mrow_, you'll call FindAttributeDependence(aAttribute, mrowMap)
Comment 8 Frédéric Wang (:fredw) 2012-12-13 07:35:38 PST
Comment on attachment 691757 [details] [diff] [review]
Determine directionality

Review of attachment 691757 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/mathml/nsMathMLmrowFrame.cpp
@@ +33,5 @@
>    // let the base class get the default from our parent
>    nsMathMLContainerFrame::InheritAutomaticData(aParent);
>  
>    mPresentationData.flags |= NS_MATHML_STRETCH_ALL_CHILDREN_VERTICALLY;
> +  mPresentationData.mstyle = this;

Why did you set mPresentationData.mstyle here?
Comment 9 Cosmin Clapon 2012-12-13 07:39:54 PST
(In reply to Frédéric Wang (:fredw) from comment #8)
> Comment on attachment 691757 [details] [diff] [review]
> Determine directionality
> 
> Review of attachment 691757 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/mathml/nsMathMLmrowFrame.cpp
> @@ +33,5 @@
> >    // let the base class get the default from our parent
> >    nsMathMLContainerFrame::InheritAutomaticData(aParent);
> >  
> >    mPresentationData.flags |= NS_MATHML_STRETCH_ALL_CHILDREN_VERTICALLY;
> > +  mPresentationData.mstyle = this;
> 
> Why did you set mPresentationData.mstyle here?

Because, otherwise, the test
<mrow dir="rtl">
<mi>a</mi>
            <mi>b</mi>
            <mi>c</mi>
          </mrow>
Comment 10 Frédéric Wang (:fredw) 2012-12-13 07:40:58 PST
Comment on attachment 691758 [details] [diff] [review]
Reftest

Review of attachment 691758 [details] [diff] [review]:
-----------------------------------------------------------------

The dir attribute is already tested in other tests. I would prefer a small test pages with only what you have changed. So comparing the "dir" attribute and the CSS direction for <math>, <mstyle>, <mrow> and the token elements.
Comment 11 Frédéric Wang (:fredw) 2012-12-13 07:48:10 PST
(In reply to Cosmin Clapon from comment #9)
> > > +  mPresentationData.mstyle = this;
> > 
> Because, otherwise, the test
> <mrow dir="rtl">
> <mi>a</mi>
>             <mi>b</mi>
>             <mi>c</mi>
>           </mrow>

OK, I didn't realized that you used 

mPresentationData.mstyle->GetStyleContext()

to determine the directionality. You only need GetStyleContext() to get the style context on the current frame and not the one from an mstyle ancestor (That's the goal of this bug to use normal CSS inheritance for dir instead of the mPresentationData mechanism).
Comment 12 Frédéric Wang (:fredw) 2012-12-13 07:49:29 PST
(In reply to Frédéric Wang (:fredw) from comment #11)
> to determine the directionality. You only need GetStyleContext()

I mean you can just remove the "mPresentationData.mstyle->" prefix.
Comment 13 Cosmin Clapon 2012-12-13 10:04:32 PST
Created attachment 691876 [details] [diff] [review]
Dir attribute mapping

Mapped the dir attribute to CSS direction and fixed mrow attribute dependence
Comment 14 Cosmin Clapon 2012-12-13 10:06:01 PST
Created attachment 691878 [details] [diff] [review]
Determine directionality

Changed the way directionality is determined (via CSS direction)
Comment 15 Cosmin Clapon 2012-12-13 10:07:31 PST
Created attachment 691879 [details] [diff] [review]
Reftest

Comparing dir="rtl" with direction: rtl
Comment 16 Frédéric Wang (:fredw) 2012-12-13 11:21:56 PST
Comment on attachment 691878 [details] [diff] [review]
Determine directionality

Review of attachment 691878 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/mathml/nsIMathMLFrame.h
@@ -334,5 @@
>    (NS_MATHML_SPACE_LIKE == ((_flags) & NS_MATHML_SPACE_LIKE))
>  
> -#define NS_MATHML_IS_RTL(_flags) \
> -  (NS_MATHML_RTL == ((_flags) & NS_MATHML_RTL))
> -

I guess you can remove the definition of NS_MATHML_RTL too. Can you please search strings rtl, direction etc to see if you have not forgotten anything. Or compare with the patch from bug 534963.
Comment 17 Frédéric Wang (:fredw) 2012-12-13 11:23:58 PST
@Cosmin: the patches look good to me, thanks. Just try to avoid some whitespace changes. I haven't verified the coding style in details.
Comment 18 Frédéric Wang (:fredw) 2012-12-13 11:39:38 PST
There is one issue to consider. In some arabic countries, the text is written RTL and inner math too. In most arabic countries and in Hebrew too, the text is RTL but the inner math is LTR. See

https://developer.mozilla.org/ar/docs/Mozilla_MathML_Project/Start
https://developer.mozilla.org/he/docs/Mozilla_MathML_Project/Start

By default the math element in mathml.css is set to direction: ltr, so the default is LTR which I think is correct. Do your patches preserve this behavior? (I think so)
Comment 19 Cosmin Clapon 2012-12-13 12:05:17 PST
Created attachment 691943 [details] [diff] [review]
Dir attribute mapping

Mapped the dir attribute to CSS direction and fixed mrow attribute dependence [+removed some whitespaces]
Comment 20 Cosmin Clapon 2012-12-13 12:13:49 PST
(In reply to Frédéric Wang (:fredw) from comment #18)
> There is one issue to consider. In some arabic countries, the text is
> written RTL and inner math too. In most arabic countries and in Hebrew too,
> the text is RTL but the inner math is LTR. See
> 
> https://developer.mozilla.org/ar/docs/Mozilla_MathML_Project/Start
> https://developer.mozilla.org/he/docs/Mozilla_MathML_Project/Start
> 
> By default the math element in mathml.css is set to direction: ltr, so the
> default is LTR which I think is correct. Do your patches preserve this
> behavior? (I think so)

I checked the links and they look fine.
In the first link, both the text and the math are RTL and in the second link the text is RTL and the math is LTR.
Comment 21 Cosmin Clapon 2012-12-13 12:17:08 PST
Created attachment 691951 [details] [diff] [review]
Determine directionality

Changed the way directionality is determined (via CSS direction) [+removed some redundant code and whitespaces]
Comment 22 Frédéric Wang (:fredw) 2012-12-13 15:00:45 PST
Comment on attachment 691951 [details] [diff] [review]
Determine directionality

Review of attachment 691951 [details] [diff] [review]:
-----------------------------------------------------------------

I'm wondering if sometimes, instead of calling GetStyleContext(), you could just use mStyleContext or the custom style context of the nsMathMLChar's. I doubt it will really change the result, though. I'm OK with these patches, so no need to ask a feedback again.
Comment 23 Cosmin Clapon 2012-12-14 03:14:01 PST
Created attachment 692250 [details] [diff] [review]
Determine directionality

Changed the way directionality is determined (via CSS direction) [+removed some whitespaces, redundant code and replaced GetStyleContext() with mStyleContext]
Comment 24 Karl Tomlinson (ni?:karlt) 2013-01-03 20:35:25 PST
Comment on attachment 691879 [details] [diff] [review]
Reftest

Yes, the MathML dir attribute and the CSS direction property look like they should behave similarly.

One difference might be that CSS direction would affect the direction of table column layout, but the default dir=ltr on math may not be expected to affect the direction of html tables embedded in the mathml.  I guess html within mathml doesn't have defined behavior anyway so this is not important.
Comment 25 Karl Tomlinson (ni?:karlt) 2013-01-03 20:40:37 PST
(In reply to Frédéric Wang (:fredw) from comment #18)
> By default the math element in mathml.css is set to direction: ltr, so the
> default is LTR which I think is correct.

Yes, http://www.w3.org/TR/MathML3/chapter2.html#interf.toplevel.atts says
"Note that the dir attribute defaults to "ltr" on the math element (but inherits on all other elements which accept the dir attribute); this provides for backward compatibility with MathML 2.0 which had no notion of directionality."

I'll see if I can look over these patches next week, thanks.
Comment 26 Karl Tomlinson (ni?:karlt) 2013-01-07 00:53:54 PST
Comment on attachment 691943 [details] [diff] [review]
Dir attribute mapping

>+  if (tag == nsGkAtoms::mrow_)
>+    return FindAttributeDependence(aAttribute, mrowMap);
>   if (tag == nsGkAtoms::maction_ ||

Please add a blank line after the "return" statement.
Surrounding style is inconsistent, but usually we have a blank line after a
jump statement if there is no closing brace.

>+    if(value && value->Type() == nsAttrValue::eString &&
>+       direction->GetUnit() == eCSSUnit_Null) {

Style is "if (".

>+      static const char dirs[2][4] = { "ltr", "rtl" };

Does dirs[2][] work here?

>+      for(uint32_t i = 0; i < ArrayLength(dirs); ++i) {

Style is "for (".

>+        if(str.EqualsASCII(dirs[i])) {
>+          direction->SetIntValue(dirValues[i], eCSSUnit_Enumerated);
>+          break;
>+        }
>+
>+      }

No need for the blank line here.

Should we use "str.CompressWhitespace()" for this attribute?

r+ with those addressed.
Comment 27 Karl Tomlinson (ni?:karlt) 2013-01-07 13:34:40 PST
Comment on attachment 692250 [details] [diff] [review]
Determine directionality

>+          nscoord dx = (mStyleContext->GetStyleVisibility()
>+                                         ->mDirection ?

No need for "mStyleContext->" here as there is a GetStyleVisibility() method
on |this| (nsIFrame).

Put ->mDirection immediately after, on the same line as, GetStyleVisibility().

Similarly elsewhere.

>   nsMathMLContainerFrame::InheritAutomaticData(aParent);
> 
>-  if (mContent->Tag() != nsGkAtoms::mspace_) {
>-    // see if the directionality attribute is there
>-    nsMathMLFrame::FindAttrDirectionality(mContent, mPresentationData);
>-  }
> 
>   ProcessTextData();

Leave only one blank line here.

>   containerSize.descent = delta - axisHeight;
> 
>-  bool isRTL = NS_MATHML_IS_RTL(mPresentationData.flags);
> 
>+  bool isRTL = mStyleContext->GetStyleVisibility()
>+                            ->mDirection;
>   /////////////////

Leave the line where it was between the blank lines.

>   nsMathMLFrame::FindAttrDisplaystyle(mContent, mPresentationData);
> 
>-  // see if the directionality attribute is there
>-  nsMathMLFrame::FindAttrDirectionality(mContent, mPresentationData);
> 
>   return NS_OK;

Only one blank line here.

Looks good thanks, but I'd like to check style and alignment in the next
patch.  I think |GetStyleVisibility()->mDirection| should always be able to
stay together without exceeding 80 columns.  Gecko style is to have the
operator at the end of the first line if breaking at the operator.  Operators
with no spaces around them (-> and .) are usually only used for line break if
there are no other reasonable options.

Please include function names in future patches.  This can be done
automatically by setting up ~/.hgrc as described at
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 28 Josh Matthews [:jdm] 2013-04-04 11:32:36 PDT
Cosmin, do you have time to update your patch here to address Karl's review comments?
Comment 29 Frédéric Wang (:fredw) 2013-05-23 02:02:11 PDT
Since Cosmin didn't answer I'll finish the work on this.
Comment 30 Frédéric Wang (:fredw) 2013-05-23 02:26:50 PDT
(In reply to Karl Tomlinson (:karlt) from comment #26)
> Should we use "str.CompressWhitespace()" for this attribute?
> 

The MathML RelaxNG schema does not allow whitespaces:

  attribute dir {"ltr" | "rtl"}?

so I think we don't need to do so.
Comment 31 Frédéric Wang (:fredw) 2013-05-23 06:27:59 PDT
Created attachment 753250 [details] [diff] [review]
Reftest
Comment 32 Frédéric Wang (:fredw) 2013-05-23 06:28:32 PDT
Created attachment 753251 [details] [diff] [review]
Dir attribute mapping
Comment 33 Frédéric Wang (:fredw) 2013-05-23 06:29:14 PDT
Created attachment 753252 [details] [diff] [review]
Determine directionality
Comment 34 Frédéric Wang (:fredw) 2013-05-23 06:31:46 PDT
(In reply to Karl Tomlinson (:karlt) from comment #26)
> Does dirs[2][] work here?
> 
It's the first bound in multidimensional arrays that can be omitted, so I've done dirs[][4] instead.

I've sent the updated patches to the try server

https://tbpl.mozilla.org/?tree=Try&rev=a7486a7d0df9
Comment 35 Frédéric Wang (:fredw) 2013-05-23 06:34:27 PDT
> Leave only one blank line here.
> 
> Leave the line where it was between the blank lines.
> 
> Only one blank line here.
> 

Oops, I forgot to fix these whitespace mistakes. I'll update the patch later.
Comment 36 Frédéric Wang (:fredw) 2013-05-23 13:31:07 PDT
Created attachment 753458 [details] [diff] [review]
Determine directionality

OK, only the spacing mistake in nsMathMLmstyleFrame actually remained.

The try server results look good.
Comment 39 Florian Scholz [:fscholz] (MDN) 2013-09-30 05:38:57 PDT
A note has been added to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/24#MathML

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