Improve how MathML directionality is determined

RESOLVED FIXED in mozilla24

Status

()

Core
MathML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 10 obsolete attachments)

2.68 KB, patch
Details | Diff | Splinter Review
5.70 KB, patch
Details | Diff | Splinter Review
23.35 KB, patch
karlt
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
I'm interested in working on this, I'll assign it to myself if that's okay.
Assignee: nobody → prp.1111
(Assignee)

Comment 2

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

Updated

5 years ago
Assignee: prp.1111 → nobody
(Assignee)

Comment 3

5 years ago
Cosmin Clapon would like to work on this bug.
Assignee: nobody → cosmin.clapon

Comment 4

5 years ago
Created attachment 691755 [details] [diff] [review]
Dir attribute mapping

Mapped the dir attribute to CSS direction
Attachment #691755 - Flags: review?(karlt)

Comment 5

5 years ago
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
Attachment #691757 - Flags: review?(karlt)

Comment 6

5 years ago
Created attachment 691758 [details] [diff] [review]
Reftest

Compares the rendering with dir="rtl" and direction: rtl (CSS)
Attachment #691758 - Flags: review?(karlt)

Updated

5 years ago
Attachment #691755 - Flags: feedback?(fred.wang)

Updated

5 years ago
Attachment #691757 - Flags: feedback?(fred.wang)

Updated

5 years ago
Attachment #691758 - Flags: feedback?(fred.wang)
(Assignee)

Comment 7

5 years ago
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)
Attachment #691755 - Flags: feedback?(fred.wang) → feedback+
(Assignee)

Comment 8

5 years ago
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?
Attachment #691757 - Flags: feedback?(fred.wang) → feedback+

Comment 9

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

Comment 10

5 years ago
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.
Attachment #691758 - Flags: feedback?(fred.wang) → feedback+
(Assignee)

Comment 11

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

Comment 12

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

Updated

5 years ago
Attachment #691755 - Attachment is obsolete: true
Attachment #691755 - Flags: review?(karlt)

Updated

5 years ago
Attachment #691757 - Attachment is obsolete: true
Attachment #691757 - Flags: review?(karlt)

Updated

5 years ago
Attachment #691758 - Attachment is obsolete: true
Attachment #691758 - Flags: review?(karlt)

Comment 13

5 years ago
Created attachment 691876 [details] [diff] [review]
Dir attribute mapping

Mapped the dir attribute to CSS direction and fixed mrow attribute dependence
Attachment #691876 - Flags: feedback?(fred.wang)

Comment 14

5 years ago
Created attachment 691878 [details] [diff] [review]
Determine directionality

Changed the way directionality is determined (via CSS direction)
Attachment #691878 - Flags: feedback?(fred.wang)

Comment 15

5 years ago
Created attachment 691879 [details] [diff] [review]
Reftest

Comparing dir="rtl" with direction: rtl
Attachment #691879 - Flags: feedback?(fred.wang)
(Assignee)

Comment 16

5 years ago
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.
Attachment #691878 - Flags: feedback?(fred.wang) → feedback+
(Assignee)

Updated

5 years ago
Attachment #691876 - Flags: feedback?(fred.wang) → feedback+
(Assignee)

Updated

5 years ago
Attachment #691879 - Flags: feedback?(fred.wang) → feedback+
(Assignee)

Comment 17

5 years ago
@Cosmin: the patches look good to me, thanks. Just try to avoid some whitespace changes. I haven't verified the coding style in details.
(Assignee)

Comment 18

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

5 years ago
Created attachment 691943 [details] [diff] [review]
Dir attribute mapping

Mapped the dir attribute to CSS direction and fixed mrow attribute dependence [+removed some whitespaces]
Attachment #691876 - Attachment is obsolete: true

Comment 20

5 years ago
(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

5 years ago
Created attachment 691951 [details] [diff] [review]
Determine directionality

Changed the way directionality is determined (via CSS direction) [+removed some redundant code and whitespaces]
Attachment #691878 - Attachment is obsolete: true

Updated

5 years ago
Attachment #691943 - Flags: feedback?(fred.wang)

Updated

5 years ago
Attachment #691951 - Flags: feedback?(fred.wang)
(Assignee)

Updated

5 years ago
Attachment #691943 - Flags: feedback?(fred.wang) → feedback+
(Assignee)

Comment 22

5 years ago
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.
Attachment #691951 - Flags: feedback?(fred.wang) → feedback+

Comment 23

5 years ago
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]
Attachment #691951 - Attachment is obsolete: true

Updated

5 years ago
Attachment #691879 - Flags: review?(karlt)

Updated

5 years ago
Attachment #691943 - Flags: review?(karlt)

Updated

5 years ago
Attachment #692250 - Flags: review?(karlt)
Blocks: 819664
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.
Attachment #691879 - Flags: review?(karlt) → review+
(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 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.
Attachment #691943 - Flags: review?(karlt) → review+
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
Attachment #692250 - Flags: review?(karlt)
Attachment #692250 - Flags: review-
Attachment #692250 - Flags: feedback+
(Assignee)

Updated

4 years ago
Blocks: 838509
(Assignee)

Updated

4 years ago
Blocks: 838506
Cosmin, do you have time to update your patch here to address Karl's review comments?
Flags: needinfo?(cosmin.clapon)

Updated

4 years ago
Assignee: cosmin.clapon → nobody
Flags: needinfo?(cosmin.clapon)
(Assignee)

Comment 29

4 years ago
Since Cosmin didn't answer I'll finish the work on this.
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Keywords: helpwanted
Whiteboard: [mentor=fredw][lang=c++]
(Assignee)

Comment 30

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

Comment 31

4 years ago
Created attachment 753250 [details] [diff] [review]
Reftest
Attachment #691879 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
Created attachment 753251 [details] [diff] [review]
Dir attribute mapping
Attachment #691943 - Attachment is obsolete: true
(Assignee)

Comment 33

4 years ago
Created attachment 753252 [details] [diff] [review]
Determine directionality
Attachment #692250 - Attachment is obsolete: true
Attachment #753252 - Flags: review?(karlt)
(Assignee)

Comment 34

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

Comment 35

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

Comment 36

4 years ago
Created attachment 753458 [details] [diff] [review]
Determine directionality

OK, only the spacing mistake in nsMathMLmstyleFrame actually remained.

The try server results look good.
Attachment #753252 - Attachment is obsolete: true
Attachment #753252 - Flags: review?(karlt)
Attachment #753458 - Flags: review?(karlt)
Attachment #753458 - Flags: review?(karlt) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/40be35413245
https://hg.mozilla.org/integration/mozilla-inbound/rev/bff779366272
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d77a2d12af
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/40be35413245
https://hg.mozilla.org/mozilla-central/rev/bff779366272
https://hg.mozilla.org/mozilla-central/rev/b9d77a2d12af
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Assignee)

Updated

4 years ago
Depends on: 878396
A note has been added to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/24#MathML
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.