Bug 124572 – Map the dir attribute to the CSS direction property

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Frédéric Wang (:fredw)]

This is the first step for bug 123018: map dir attribute on math, mstyle, mrow and token elements to style. I'll add minimal tests for that and address Davin's request in but 124121 comment 2. The actual implementation is not done, but that should work for token elements (needed for MathJax error message) and very simple math (e.g. mrow, scripts).

[Frédéric Wang (:fredw)]

Created attachment 217283 [details]
Patch V1

[Darin Adler]

Comment on attachment 217283 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=217283&action=review

This is OK, but I see room for improvement, especially on test coverage.

> Source/WebCore/mathml/MathMLElement.cpp:81
> +    if (name == mathbackgroundAttr || name == mathsizeAttr || name == mathcolorAttr || name == fontsizeAttr || name == backgroundAttr || name == colorAttr || name == fontstyleAttr || name == fontweightAttr || name == fontfamilyAttr || name == dirAttr)

I would be more comfortable with this if statement if the attributes were in some obvious order, say alphabetical. Also would be nice if there was a test that would fail if each of these was removed. I am betting that I could remove one of these 10 without any test failing.

> Source/WebCore/mathml/MathMLElement.cpp:110
> +        if (hasTagName(mathTag) || hasTagName(mstyleTag) || hasTagName(mrowTag) || hasTagName(mtextTag) || hasTagName(msTag) || hasTagName(moTag) || hasTagName(miTag) || hasTagName(mnTag))

I would be more comfortable with this if statement if the tag names were in some obvious order, say alphabetical. Also would be nice if there was a test that would fail if each of these was removed. I am betting that I could remove one of these 8 without any test failing.

Also, this might be a good candidate for a named function (can be inline so the code generated is the same), since it’s not obvious to me what these elements have in common. Think of the function name as a good way to comment what this set of tags represents.

> Source/WebCore/mathml/mathtags.in:11
> +mstyle interfaceName=MathMLElement

I’d like to see test coverage for this. We have tests that check if elements get the right kind of wrapper, and we should cover this tag in that kind of test.

[WebKit Commit Bot]

Comment on attachment 217283 [details]
Patch V1

Clearing flags on attachment: 217283

Committed r159504: <http://trac.webkit.org/changeset/159504>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

Frédéric: Would you mind posting a secondary patch to incorporate Darin's suggestions for improvements? We can reopen this bug to do so.

[Frédéric Wang (:fredw)]

OK, I will come back to this tomorrow.

Regarding the first set, there should already test coverage for these attributes and test for dir is added here.

Regarding the second set, new tests are added here for each element. However, perhaps I should add a test verifying that dir has no effect on other elements like e.g. msqrt.

[Frédéric Wang (:fredw)]

(In reply to comment #6)
> I’d like to see test coverage for this. We have tests that check if elements get the right kind of wrapper, and we should cover this tag in that kind of test.

Can you give me an example of such a test so that I can do that for mstyle?

[Frédéric Wang (:fredw)]

Created attachment 217415 [details]
Follow up V1

> if (name == mathbackgroundAttr || name == mathsizeAttr || name == mathcolorAttr || name == fontsizeAttr || name == backgroundAttr || name == colorAttr || name == fontstyleAttr || name == fontweightAttr || name == fontfamilyAttr || name == dirAttr)

These were tested by mathml/presentation/attributes-mathvariant, mathsize and background-color.
The dir attribute is tested by the test added in the first patch.

>  if (hasTagName(mathTag) || hasTagName(mstyleTag) || hasTagName(mrowTag) || hasTagName(mtextTag) || hasTagName(msTag) || hasTagName(moTag) || hasTagName(miTag) || hasTagName(mnTag))

These are tested by the new tests, especially direction-overall and direction-token. I've added a test to direction-overall to check that the dir attribute is not applied to <msqrt>.

> I’d like to see test coverage for this. We have tests that check if elements get the right kind of wrapper, and we should cover this tag in that kind of test.

I have not done that yet.

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #6)
> > I’d like to see test coverage for this. We have tests that check if elements get the right kind of wrapper, and we should cover this tag in that kind of test.
> 
> Can you give me an example of such a test so that I can do that for mstyle?

fast/dom/wrapper-classes.html

[Frédéric Wang (:fredw)]

Created attachment 217469 [details]
Follow up V2

Mmm, MathML does not have a DOM so I'm not really sure I can access the exact class from Javascript. I tried something but it seems to just returns "Element".

[Darin Adler]

(In reply to comment #10)
> Mmm, MathML does not have a DOM so I'm not really sure I can access the exact class from Javascript. I tried something but it seems to just returns "Element".

I see. I did not know that.

We’ll have to test this indirectly by checking for the different behavior caused by the internal DOM class, not by looking at wrappers at all.

[Frédéric Wang (:fredw)]

Created attachment 217541 [details]
Follow up V3

[Frédéric Wang (:fredw)]

(In reply to comment #11)
> I see. I did not know that.
> 
> We’ll have to test this indirectly by checking for the different behavior caused by the internal DOM class, not by looking at wrappers at all.

OK, I've removed that test. Although removing one of the presentation attribute would have broken the test for the token elements, there were nothing to check the presentation attributes on <mstyle> so I've added a test for that.

[Frédéric Wang (:fredw)]

Reopening per comment 5.

[Darin Adler]

(In reply to comment #5)
> Frédéric: Would you mind posting a secondary patch to incorporate Darin's suggestions for improvements? We can reopen this bug to do so.

Brent, why did you suggest reopening this bug? I thought the best practice for a case like this is to use a new bug report.

[Brent Fulgham]

(In reply to comment #15)
> (In reply to comment #5)
> > Frédéric: Would you mind posting a secondary patch to incorporate Darin's suggestions for improvements? We can reopen this bug to do so.
> 
> Brent, why did you suggest reopening this bug? I thought the best practice for a case like this is to use a new bug report.

I thought he just had a couple of small changes to incorporate some suggestions you had on his original patch.  I thought it would be easier to track this in the same bug, rather than have multiple bugs (and possibly Radars) that track a small set of changes focused on the same topic.

But in the future I'll have people leave the bug closed and open a new one.

[Darin Adler]

Comment on attachment 217541 [details]
Follow up V3

View in context: https://bugs.webkit.org/attachment.cgi?id=217541&action=review

> Source/WebCore/mathml/MathMLElement.cpp:128
> +bool MathMLElement::isMathMLToken() const
> +{
> +    return hasTagName(miTag) || hasTagName(mnTag) || hasTagName(moTag) || hasTagName(msTag) || hasTagName(mtextTag);
> +}

Are you sure this is actually getting inlined? In older compilers we had to put the function higher in the file than the call sites, and also mark it inline here, not just in the declaration.

[WebKit Commit Bot]

Comment on attachment 217541 [details]
Follow up V3

Clearing flags on attachment: 217541

Committed r159680: <http://trac.webkit.org/changeset/159680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

Mass change: add WebExposed keyword to help MDN documentation.