Closed
Bug 764996
Opened 12 years ago
Closed 11 years ago
Improve how MathML directionality is determined
Categories
(Core :: MathML, defect)
Core
MathML
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.
Comment 1•12 years ago
|
||
I'm interested in working on this, I'll assign it to myself if that's okay.
Updated•12 years ago
|
Assignee: nobody → prp.1111
Assignee | ||
Comment 2•12 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•12 years ago
|
Assignee: prp.1111 → nobody
Assignee | ||
Comment 3•12 years ago
|
||
Cosmin Clapon would like to work on this bug.
Assignee: nobody → cosmin.clapon
Comment 4•11 years ago
|
||
Mapped the dir attribute to CSS direction
Attachment #691755 -
Flags: review?(karlt)
Comment 5•11 years ago
|
||
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•11 years ago
|
||
Compares the rendering with dir="rtl" and direction: rtl (CSS)
Attachment #691758 -
Flags: review?(karlt)
Updated•11 years ago
|
Attachment #691755 -
Flags: feedback?(fred.wang)
Updated•11 years ago
|
Attachment #691757 -
Flags: feedback?(fred.wang)
Updated•11 years ago
|
Attachment #691758 -
Flags: feedback?(fred.wang)
Assignee | ||
Comment 7•11 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•11 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•11 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•11 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•11 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•11 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•11 years ago
|
Attachment #691755 -
Attachment is obsolete: true
Attachment #691755 -
Flags: review?(karlt)
Updated•11 years ago
|
Attachment #691757 -
Attachment is obsolete: true
Attachment #691757 -
Flags: review?(karlt)
Updated•11 years ago
|
Attachment #691758 -
Attachment is obsolete: true
Attachment #691758 -
Flags: review?(karlt)
Comment 13•11 years ago
|
||
Mapped the dir attribute to CSS direction and fixed mrow attribute dependence
Attachment #691876 -
Flags: feedback?(fred.wang)
Comment 14•11 years ago
|
||
Changed the way directionality is determined (via CSS direction)
Attachment #691878 -
Flags: feedback?(fred.wang)
Comment 15•11 years ago
|
||
Comparing dir="rtl" with direction: rtl
Attachment #691879 -
Flags: feedback?(fred.wang)
Assignee | ||
Comment 16•11 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•11 years ago
|
Attachment #691876 -
Flags: feedback?(fred.wang) → feedback+
Assignee | ||
Updated•11 years ago
|
Attachment #691879 -
Flags: feedback?(fred.wang) → feedback+
Assignee | ||
Comment 17•11 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•11 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•11 years ago
|
||
Mapped the dir attribute to CSS direction and fixed mrow attribute dependence [+removed some whitespaces]
Attachment #691876 -
Attachment is obsolete: true
Comment 20•11 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•11 years ago
|
||
Changed the way directionality is determined (via CSS direction) [+removed some redundant code and whitespaces]
Attachment #691878 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #691943 -
Flags: feedback?(fred.wang)
Updated•11 years ago
|
Attachment #691951 -
Flags: feedback?(fred.wang)
Assignee | ||
Updated•11 years ago
|
Attachment #691943 -
Flags: feedback?(fred.wang) → feedback+
Assignee | ||
Comment 22•11 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•11 years ago
|
||
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•11 years ago
|
Attachment #691879 -
Flags: review?(karlt)
Updated•11 years ago
|
Attachment #691943 -
Flags: review?(karlt)
Updated•11 years ago
|
Attachment #692250 -
Flags: review?(karlt)
Blocks: 819664
Comment 24•11 years ago
|
||
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+
Comment 25•11 years ago
|
||
(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•11 years ago
|
||
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 27•11 years ago
|
||
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+
Comment 28•11 years ago
|
||
Cosmin, do you have time to update your patch here to address Karl's review comments?
Flags: needinfo?(cosmin.clapon)
Updated•11 years ago
|
Assignee: cosmin.clapon → nobody
Flags: needinfo?(cosmin.clapon)
Assignee | ||
Comment 29•11 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•11 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•11 years ago
|
||
Attachment #691879 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
Attachment #691943 -
Attachment is obsolete: true
Assignee | ||
Comment 33•11 years ago
|
||
Attachment #692250 -
Attachment is obsolete: true
Attachment #753252 -
Flags: review?(karlt)
Assignee | ||
Comment 34•11 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•11 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•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #753458 -
Flags: review?(karlt) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 37•11 years ago
|
||
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
Comment 38•11 years ago
|
||
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 39•11 years ago
|
||
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.
Description
•