Bug 124572 - Map the dir attribute to the CSS direction property
Map the dir attribute to the CSS direction property
 Status: RESOLVED FIXED None WebKit Unclassified MathML (show other bugs) 528+ (Nightly build) All All P2 Normal Frédéric Wang (:fredw) WebExposed 123018 Show dependency tree / graph

 Reported: 2013-11-19 02:51 PST by Frédéric Wang (:fredw) 2014-03-10 12:26 PDT (History) 6 users (show) bfulgham cfleizach commit-queue darin dbarton mrobinson

Attachments
Patch V1 (12.70 KB, patch)
2013-11-19 02:57 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Follow up V1 (4.04 KB, patch)
2013-11-20 03:31 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Follow up V2 (7.22 KB, patch)
2013-11-20 13:04 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Follow up V3 (9.24 KB, patch)
2013-11-21 03:14 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

 Note You need to log in before you can comment on or make changes to this bug.
 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). Frédéric Wang (:fredw) 2013-11-19 02:57:38 PST Created attachment 217283 [details] Patch V1 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. WebKit Commit Bot 2013-11-19 09:33:41 PST Comment on attachment 217283 [details] Patch V1 Clearing flags on attachment: 217283 Committed r159504:  WebKit Commit Bot 2013-11-19 09:33:43 PST All reviewed patches have been landed. Closing bug. 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. 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. 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? 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 . > 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 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 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". 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. Frédéric Wang (:fredw) 2013-11-21 03:14:33 PST Created attachment 217541 [details] Follow up V3 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 so I've added a test for that. Frédéric Wang (:fredw) 2013-11-21 03:17:10 PST Reopening per comment 5. 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. 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. 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. 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:  WebKit Commit Bot 2013-11-21 21:01:08 PST All reviewed patches have been landed. Closing bug. Frédéric Wang (:fredw) 2014-03-10 12:26:20 PDT Mass change: add WebExposed keyword to help MDN documentation.