Bug 124572

Summary: Map the dir attribute to the CSS direction property
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cfleizach, commit-queue, darin, dbarton, mrobinson
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 123018    
Attachments:
Description Flags
Patch V1
none
Follow up V1
none
Follow up V2
none
Follow up V3 none

Description Frédéric Wang (:fredw) 2013-11-19 02:51:34 PST
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).
Comment 1 Frédéric Wang (:fredw) 2013-11-19 02:57:38 PST
Created attachment 217283 [details]
Patch V1
Comment 2 Darin Adler 2013-11-19 09:06:40 PST
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.
Comment 3 WebKit Commit Bot 2013-11-19 09:33:41 PST
Comment on attachment 217283 [details]
Patch V1

Clearing flags on attachment: 217283

Committed r159504: <http://trac.webkit.org/changeset/159504>
Comment 4 WebKit Commit Bot 2013-11-19 09:33:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Brent Fulgham 2013-11-19 09:41:37 PST
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.
Comment 6 Frédéric Wang (:fredw) 2013-11-19 09:55:49 PST
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.
Comment 7 Frédéric Wang (:fredw) 2013-11-20 02:43:45 PST
(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?
Comment 8 Frédéric Wang (:fredw) 2013-11-20 03:31:23 PST
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.
Comment 9 Darin Adler 2013-11-20 09:32:11 PST
(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
Comment 10 Frédéric Wang (:fredw) 2013-11-20 13:04:14 PST
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".
Comment 11 Darin Adler 2013-11-20 20:13:35 PST
(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.
Comment 12 Frédéric Wang (:fredw) 2013-11-21 03:14:33 PST
Created attachment 217541 [details]
Follow up V3
Comment 13 Frédéric Wang (:fredw) 2013-11-21 03:16:19 PST
(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.
Comment 14 Frédéric Wang (:fredw) 2013-11-21 03:17:10 PST
Reopening per comment 5.
Comment 15 Darin Adler 2013-11-21 13:47:03 PST
(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.
Comment 16 Brent Fulgham 2013-11-21 15:21:46 PST
(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.
Comment 17 Darin Adler 2013-11-21 20:14:14 PST
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.
Comment 18 WebKit Commit Bot 2013-11-21 21:01:04 PST
Comment on attachment 217541 [details]
Follow up V3

Clearing flags on attachment: 217541

Committed r159680: <http://trac.webkit.org/changeset/159680>
Comment 19 WebKit Commit Bot 2013-11-21 21:01:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Frédéric Wang (:fredw) 2014-03-10 12:26:20 PDT
Mass change: add WebExposed keyword to help MDN documentation.