Closed Bug 764996 Opened 12 years ago Closed 11 years ago

Improve how MathML directionality is determined

Categories

(Core :: MathML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: fredw, Assigned: fredw)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 10 obsolete files)

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
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
(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: prp.1111 → nobody
Cosmin Clapon would like to work on this bug.
Assignee: nobody → cosmin.clapon
Attached patch Dir attribute mapping (obsolete) — Splinter Review
Mapped the dir attribute to CSS direction
Attachment #691755 - Flags: review?(karlt)
Attached patch Determine directionality (obsolete) — Splinter Review
Improved how directionality is determined (via CSS direction value) and removed the code that sets the directionality flag
Attachment #691757 - Flags: review?(karlt)
Attached patch Reftest (obsolete) — Splinter Review
Compares the rendering with dir="rtl" and direction: rtl (CSS)
Attachment #691758 - Flags: review?(karlt)
Attachment #691755 - Flags: feedback?(fred.wang)
Attachment #691757 - Flags: feedback?(fred.wang)
Attachment #691758 - Flags: feedback?(fred.wang)
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+
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+
(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 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+
(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).
(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.
Attachment #691755 - Attachment is obsolete: true
Attachment #691755 - Flags: review?(karlt)
Attachment #691757 - Attachment is obsolete: true
Attachment #691757 - Flags: review?(karlt)
Attachment #691758 - Attachment is obsolete: true
Attachment #691758 - Flags: review?(karlt)
Attached patch Dir attribute mapping (obsolete) — Splinter Review
Mapped the dir attribute to CSS direction and fixed mrow attribute dependence
Attachment #691876 - Flags: feedback?(fred.wang)
Attached patch Determine directionality (obsolete) — Splinter Review
Changed the way directionality is determined (via CSS direction)
Attachment #691878 - Flags: feedback?(fred.wang)
Attached patch Reftest (obsolete) — Splinter Review
Comparing dir="rtl" with direction: rtl
Attachment #691879 - Flags: feedback?(fred.wang)
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+
Attachment #691876 - Flags: feedback?(fred.wang) → feedback+
Attachment #691879 - Flags: feedback?(fred.wang) → feedback+
@Cosmin: the patches look good to me, thanks. Just try to avoid some whitespace changes. I haven't verified the coding style in details.
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)
Attached patch Dir attribute mapping (obsolete) — Splinter Review
Mapped the dir attribute to CSS direction and fixed mrow attribute dependence [+removed some whitespaces]
Attachment #691876 - Attachment is obsolete: true
(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.
Attached patch Determine directionality (obsolete) — Splinter Review
Changed the way directionality is determined (via CSS direction) [+removed some redundant code and whitespaces]
Attachment #691878 - Attachment is obsolete: true
Attachment #691943 - Flags: feedback?(fred.wang)
Attachment #691951 - Flags: feedback?(fred.wang)
Attachment #691943 - Flags: feedback?(fred.wang) → feedback+
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+
Attached patch Determine directionality (obsolete) — Splinter Review
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
Attachment #691879 - Flags: review?(karlt)
Attachment #691943 - Flags: review?(karlt)
Attachment #692250 - Flags: review?(karlt)
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+
Blocks: 838509
Blocks: 838506
Cosmin, do you have time to update your patch here to address Karl's review comments?
Flags: needinfo?(cosmin.clapon)
Assignee: cosmin.clapon → nobody
Flags: needinfo?(cosmin.clapon)
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++]
(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.
Attached patch ReftestSplinter Review
Attachment #691879 - Attachment is obsolete: true
Attachment #691943 - Attachment is obsolete: true
Attached patch Determine directionality (obsolete) — Splinter Review
Attachment #692250 - Attachment is obsolete: true
Attachment #753252 - Flags: review?(karlt)
(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
> 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.
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+
Keywords: checkin-needed
Depends on: 878396
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: