| Summary: | Add an MathMLSelectElement class to implement <maction> and <semantics> | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||||||||||||||||
| Component: | MathML | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, cfleizach, commit-queue, dbarton, eflews.bot, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, macpherson, menard, mrobinson, rakuco, rego+ews, rniwa, xan.lopez | ||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 124128 | ||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 115583, 120059 | ||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||
|
Description
Frédéric Wang (:fredw)
2013-08-20 02:26:58 PDT
Created attachment 209170 [details]
Patch V1
Created attachment 211644 [details]
Patch V2
refreshing the patch
Comment on attachment 211644 [details] Patch V2 Attachment 211644 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1804413 Created attachment 211645 [details]
Patch V3
try to fix xcode makefile for mac
Comment on attachment 211645 [details] Patch V3 Attachment 211645 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1878340 Comment on attachment 211645 [details] Patch V3 Attachment 211645 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1877365 Comment on attachment 211645 [details] Patch V3 Attachment 211645 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1817405 Created attachment 211664 [details]
Patch V4
Trying to fix the build files on mac and windows again.
Created attachment 211694 [details]
Patch V5
Refresh patch again after the mmultiscript commit.
Created attachment 211770 [details]
Patch V6
Refresh again after the raw pointer => ref pointer changes.
Comment on attachment 211770 [details] Patch V6 View in context: https://bugs.webkit.org/attachment.cgi?id=211770&action=review Patch looks generally good, but there are multiple mistakes that should be fixed. > Source/WebCore/mathml/MathMLSelectElement.cpp:85 > + String actiontype = getAttribute(MathMLNames::actiontypeAttr); Type should be const AtomicString& rather than String. Should call fastGetAttribute instead of getAttribute. > Source/WebCore/mathml/MathMLSelectElement.cpp:86 > + if (actiontype != "tooltip" && actiontype != "statusline") { Is this intentionally case sensitive? I’d like to see test coverage for this; many attribute values are non-case-sensitive. > Source/WebCore/mathml/MathMLSelectElement.cpp:98 > + bool ok = true; > + int selection = (hasAttribute(MathMLNames::selectionAttr) ? getAttribute(MathMLNames::selectionAttr).toInt(&ok) : 1); > + if (ok && selection >= 1) { > + Element* child = newSelectedChild; > + for (int i = 1; child; child = child->nextElementSibling()) { > + newSelectedChild = child; > + if (i == selection) > + break; > + i++; > + } > + } There need to be more test cases. I don’t see any test cases that cover a selection attribute that has, say, leading whitespace or any of the many other cases. It’s possible this function should be skipping whitespace or using toStrictInt rather than toInt but it’s test cases that would make that clear. I also don’t see any test cases that demonstrate that a value for selection larger than the number of children means that the last child is selected, even though that's what the code does. This logic is too complicated. There are many cases that are handled identically but have different code paths, for example, lack of an attribute, attribute that is empty string, attribute that is not parseable as an int are all treated the same in unnecessarily different ways. And the loop is needlessly strange the way it mixes the integer part with the pointer part. Here’s how I would write it: int selection = fastGetAttribute(MathMLNames::selectionAttr).toInt(); for (int i = 1; i < selection; ++i) { Element* nextChild = newSelectedChild->nextElementSibling(); if (!nextChild) break; newSelectedChild = nextChild; } > Source/WebCore/mathml/MathMLSelectElement.cpp:105 > + if (m_SelectedChild != newSelectedChild) { I think that early return would be a better way to write this. Return here if they are equal so the rest of the code is not nested. > Source/WebCore/mathml/MathMLSelectElement.h:39 > +protected: > + MathMLSelectElement(const QualifiedName& tagName, Document&); Why protected instead of private? > Source/WebCore/mathml/MathMLSelectElement.h:42 > + virtual RenderObject* createRenderer(RenderArena*, RenderStyle*); Needs to be marked OVERRIDE. > Source/WebCore/mathml/MathMLSelectElement.h:44 > + bool childShouldCreateRenderer(const Node*) const; Needs to be marked virtual and OVERRIDE. > Source/WebCore/mathml/MathMLSelectElement.h:48 > + void finishParsingChildren() OVERRIDE; > + void childrenChanged(const ChildChange&) OVERRIDE; > + void attributeChanged(const QualifiedName&, const AtomicString&, AttributeModificationReason = ModifiedDirectly) OVERRIDE; Should be marked virtual as well as OVERRIDE. > Source/WebCore/mathml/MathMLSelectElement.h:51 > + Element* m_SelectedChild; The "S" should not be capitalized. It should be m_selectedChild. (In reply to comment #11) > > Source/WebCore/mathml/MathMLSelectElement.cpp:86 > > + if (actiontype != "tooltip" && actiontype != "statusline") { > > Is this intentionally case sensitive? I’d like to see test coverage for this; many attribute values are non-case-sensitive. > There need to be more test cases. I don’t see any test cases that cover a selection attribute that has, say, leading whitespace or any of the many other cases. It’s possible this function should be skipping whitespace or using toStrictInt rather than toInt but it’s test cases that would make that clear. I also don’t see any test cases that demonstrate that a value for selection larger than the number of children means that the last child is selected, even though that's what the code does. The list of possible values for actiontype is open-ended, so this is not constrained by the RelaxNG schema. However, most MathML attributes are case insensitive (except color names, for compatibility with HTML) so I think that's what we want here. The selection attribute is defined to be xsd:positiveInteger http://www.w3.org/TR/xmlschema-2/#positiveInteger so strictly speaking I'm accepting invalid values, here. However, the MathML spec says that: "For most numerical attributes, only those in a subset of the expressible values are sensible; values outside this subset are not errors, unless otherwise specified, but rather are rounded up or down (at the discretion of the renderer) to the closest value within the allowed subset. The set of allowed values may depend on the renderer, and is not specified by MathML." so I've tried to make <= 0 values fallback to 1, > Nchildren fallback to Nchildren and non-integer values being rounded (I think that's what toInt does). Regarding whitespaces, the spec does not prohibit them clearly (although the RelaxNG schema probably does): "Since some applications are inconsistent about normalization of whitespace, for maximum interoperability it is advisable to use only a single whitespace character for separating parts of a value. Moreover, leading and trailing whitespace in attribute values should be avoided." Created attachment 212332 [details]
Patch V7
Comment on attachment 212332 [details] Patch V7 Attachment 212332 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1917196 Comment on attachment 212332 [details] Patch V7 Attachment 212332 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1864440 Comment on attachment 212332 [details] Patch V7 Attachment 212332 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1917197 Comment on attachment 212332 [details] Patch V7 Attachment 212332 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1836535 Comment on attachment 212332 [details] Patch V7 Attachment 212332 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1836552 Created attachment 212337 [details]
Patch V7 - bis
refresh patch to fix compilation errors due to pointer => reference change on master.
Comment on attachment 212337 [details] Patch V7 - bis View in context: https://bugs.webkit.org/attachment.cgi?id=212337&action=review > Source/WebCore/mathml/MathMLSelectElement.cpp:40 > + : MathMLInlineContainerElement(tagName, document), m_selectedChild(0) Should be nullptr instead of 0. We need to update our coding style guideline document to say this. > Source/WebCore/mathml/MathMLSelectElement.cpp:56 > + return (child->isElementNode() && toElement(child) == m_selectedChild); No need for the typecast. The compiler understands the relationship between the types. This should just read: return child == m_selectedChild; > Source/WebCore/mathml/MathMLSelectElement.cpp:86 > + if (actiontype != "tooltip" && actiontype != "statusline") { Do these need to be case sensitive? Does the test case check that they should be case sensitive? I guess I asked this before. > Source/WebCore/mathml/MathMLSelectElement.cpp:89 > + if (newSelectedChild) { Why not put this "if (newSelectedChild)" check around even the call to hasLocalName and save a lot of work? I see no reason to do all that stuff if there are no children. > Source/WebCore/mathml/MathMLSelectElement.cpp:107 > + if (m_selectedChild && m_selectedChild->renderer()) > + Style::detachRenderTree(*m_selectedChild); This seems surprisingly manual and low level. Explicitly deleting the renderer like this is surprising. I’d normally expect this to be done during the later style recalculation process rather than explicitly and immediately. > Source/WebCore/mathml/MathMLSelectElement.h:34 > +class MathMLSelectElement : public MathMLInlineContainerElement { Should mark this class FINAL. (In reply to comment #20) > (From update of attachment 212337 [details]) > > Source/WebCore/mathml/MathMLSelectElement.cpp:86 > > + if (actiontype != "tooltip" && actiontype != "statusline") { > > Do these need to be case sensitive? Does the test case check that they should be case sensitive? I guess I asked this before. Yes, I added a test to check that they are case sensitive. > > Source/WebCore/mathml/MathMLSelectElement.cpp:107 > > + if (m_selectedChild && m_selectedChild->renderer()) > > + Style::detachRenderTree(*m_selectedChild); > > This seems surprisingly manual and low level. Explicitly deleting the renderer like this is surprising. I’d normally expect this to be done during the later style recalculation process rather than explicitly and immediately. I think when I initially wrote the first patch, not detaching the render tree made the former selected child still visible. I took inspiration from the implementation of the SVG <switch> element but that one does not seem to work very well either (see bug 74749) so that didn't help. How can I ask the style recalculation process remove this render tree? Should I add some kind of "mark dirty" to the renderer or something? Created attachment 212371 [details]
Patch V8
Comment on attachment 212371 [details] Patch V8 View in context: https://bugs.webkit.org/attachment.cgi?id=212371&action=review > Source/WebCore/mathml/MathMLSelectElement.cpp:56 > + return (child->isElementNode() && child == m_selectedChild); There is no need for the child->isElementNode() check. This line should read: return child == m_selectedChild; No parentheses and no isElementNode check. > Source/WebCore/mathml/MathMLSelectElement.cpp:107 > + if (m_selectedChild && m_selectedChild->renderer()) > + Style::detachRenderTree(*m_selectedChild); We will need to do the further research. Setting an object's style to "display: none" results in the render object being destroyed, and I see no reason why this change couldn't also do the same thing. Just requires some research to figure out exactly how that works. (In reply to comment #23) > > Source/WebCore/mathml/MathMLSelectElement.cpp:107 > > + if (m_selectedChild && m_selectedChild->renderer()) > > + Style::detachRenderTree(*m_selectedChild); > > We will need to do the further research. Setting an object's style to "display: none" results in the render object being destroyed, and I see no reason why this change couldn't also do the same thing. Just requires some research to figure out exactly how that works. Thank you for the review Darin. You made a good point about "display: none". I'll try to figure out what's happening in that case and if I can do a similar thing for MathML too. I expect to come back to that bug again early next week. (In reply to comment #23) > > Source/WebCore/mathml/MathMLSelectElement.cpp:107 > > + if (m_selectedChild && m_selectedChild->renderer()) > > + Style::detachRenderTree(*m_selectedChild); > > We will need to do the further research. Setting an object's style to "display: none" results in the render object being destroyed, and I see no reason why this change couldn't also do the same thing. Just requires some research to figure out exactly how that works. I just did a quick search now (just reading the code, not debugging). IIUC, the renderer will be detached in StyleResolveTree.cpp when it detects that the "display" style has changed. For maction/semantics, there is no style change so I'm not sure that applies in that case and that seems a bit overkill to set e.g. a flag on the element to tell the style resolver to detach the child. There seem to be a few other places in the code where Style::detachRenderTree is called directly, like to render <input> element or XML errors. What is the state of this patch? Is additional work needed before it can be landed? Darin had concerns about detaching the render tree at a low level. However, see my comment 25. Otherwise, this can be taken as it (probably refreshing the patch will be necessary, though). Created attachment 216535 [details] Patch V9 Refreshing the patch. The SVG in semantics seems to be broken by bug 124128. Comment on attachment 216535 [details] Patch V9 Attachment 216535 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22589147 New failing tests: mathml/presentation/semantics.html Created attachment 216538 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216535 [details] Patch V9 Attachment 216535 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22369190 New failing tests: js/basic-set.html mathml/presentation/semantics.html Created attachment 216540 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216535 [details] Patch V9 Attachment 216535 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22699272 New failing tests: js/basic-set.html mathml/presentation/semantics.html Created attachment 216541 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
As expected the test semantics-1 failed because of bug 124128. However, for some reason it passes on other platforms (including gtk on which I see bug 124128). That would explain why we didn't see the regression with mathml/presentation/mspace-units.html before (which is disabled on Windows and Mac IIRC). Comment on attachment 216535 [details] Patch V9 Attachment 216535 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/22599414 Comment on attachment 216535 [details] Patch V9 View in context: https://bugs.webkit.org/attachment.cgi?id=216535&action=review I think this is ready to land, modulo a couple of minor tweaks (see below). > LayoutTests/mathml/presentation/semantics.html:18 > + <!-- This is not valid MathML but has become a standard hack to include SVG in MathML because of Gecko's initial implementation. --> Please mark this test as invalid in TestExpectations, and reference bug 124128 for the failure. > Source/WebCore/ChangeLog:11 > + This adds a new MathMLSelectElement class to prepare the implementation of the <maction> and <semantics> elements, for which only one "selected" child is visible. We now simply display the first child of the <semantics> element instead of hiding the annotations and this allows to handle the use case of SVG-in-MathML as generated by Instiki ; Gecko's selection algorithm will be implemented later (bug 100626). We now also rely on the @actiontype and @selection attributes to select the visible <maction> child ; It remains to deal with the user interaction (bug 85734). For some reason this appears as a single long line in the ChangeLog. This should be broken up into standard lines. > Source/WebCore/mathml/MathMLSelectElement.cpp:85 > + // FIXME: implement user interaction for the "tooltip", "statusline" and "toggle" action types. See bug 85734. Please include the Bug number to resolve the FIXME. Comment on attachment 216535 [details] Patch V9 View in context: https://bugs.webkit.org/attachment.cgi?id=216535&action=review > Source/WebCore/mathml/MathMLSelectElement.cpp:107 > + Style::detachRenderTree(*m_selectedChild); The only other place that seems to do this is in HTMLInputElement; however, I don't see anything specifically wrong here. Maybe we should double-check with hyatt? (In reply to comment #38) > (From update of attachment 216535 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216535&action=review > > > Source/WebCore/mathml/MathMLSelectElement.cpp:107 > > + Style::detachRenderTree(*m_selectedChild); > > The only other place that seems to do this is in HTMLInputElement; however, I don't see anything specifically wrong here. Maybe we should double-check with hyatt? IIRC, there was also a place where it is used for error message (XML error I think?). And as I said above, we also need to decide how to do that for the SVG switch element (bug 74749) Created attachment 217935 [details]
Patch V10
Updated patch. The only conflict was that childShouldCreateRenderer now uses a reference rather than a point. I've fixed that and also called MathMLElement::childShouldCreateRenderer to prevent non-MathML nodes for now (this should probably be revisited later for <semantics>).
Comment on attachment 217935 [details] Patch V10 View in context: https://bugs.webkit.org/attachment.cgi?id=217935&action=review > LayoutTests/mathml/presentation/maction-dynamic.html:10 > + var maction = document.getElementsByTagNameNS(mathmlNS, "maction"); these lines should be indented in this block > LayoutTests/mathml/presentation/maction.html:99 > + <maction actiontype="toggle" selection="+0003"> can you add a negative integer test just for completeness > LayoutTests/mathml/presentation/semantics.html:9 > + <math> can you add some more cases to cover the rules defined at https://developer.mozilla.org/en-US/docs/Web/MathML/Element/semantics, starting at the line "The rules of determining the visible child in a <semantics> element are the following"' > Source/WebCore/mathml/MathMLSelectElement.cpp:30 > +#include "MathMLSelectElement.h" this can be directly below the config.h outside the ENABLE(MATHML) > Source/WebCore/mathml/MathMLSelectElement.cpp:40 > + : MathMLInlineContainerElement(tagName, document), m_selectedChild(nullptr) m_selectedChild should be on a new line i think > Source/WebCore/mathml/MathMLSelectElement.cpp:87 > + if (actiontype != "tooltip" && actiontype != "statusline") { you should add a comment that we explicitly want to be case-sensitive, since future contributors may not realize that distinction is required i think this should be written as action type == "toggle", or if toggle is for some reason the default value for all things that are not tooltip or status line, then it should be written if (action type == "tooltip") //FIXME: implement else if () // FIXME: implement else // toggle so that its more clear that this section relates to toggle (In reply to comment #41) > > LayoutTests/mathml/presentation/semantics.html:9 > > + <math> > > can you add some more cases to cover the rules defined at > > https://developer.mozilla.org/en-US/docs/Web/MathML/Element/semantics, starting at the line > "The rules of determining the visible child in a <semantics> element are the following"' This is expected to be implemented in bug 100626 (and there are tests there). At the moment the regression bug 124128 prevents from implementing <semantics> correctly, so that will be handled later anyway. > i think this should be written as action type == "toggle", or if toggle is for some reason the default value for all things that are not tooltip or status line, then it should be written > > if (action type == "tooltip") > //FIXME: implement > else if () > // FIXME: implement > else > // toggle > > so that its more clear that this section relates to toggle As indicated in the comment, this "selection" attribute is used for "toggle" and "unknown" actiontype. However, "unknown" actiontype do follow the same behavior as toggle with respect to user click. I'll the else if blocks and address the other comments. Created attachment 218169 [details]
Patch v11
Attachment 218169 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/TestExpectations', u'LayoutTests/mathml/presentation/maction-dynamic-expected.html', u'LayoutTests/mathml/presentation/maction-dynamic.html', u'LayoutTests/mathml/presentation/maction-expected.html', u'LayoutTests/mathml/presentation/maction.html', u'LayoutTests/mathml/presentation/semantics-expected.html', u'LayoutTests/mathml/presentation/semantics.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/mathml.css', u'Source/WebCore/mathml/MathMLAllInOne.cpp', u'Source/WebCore/mathml/MathMLElement.h', u'Source/WebCore/mathml/MathMLInlineContainerElement.cpp', u'Source/WebCore/mathml/MathMLSelectElement.cpp', u'Source/WebCore/mathml/MathMLSelectElement.h', u'Source/WebCore/mathml/mathattrs.in', u'Source/WebCore/mathml/mathtags.in']" exit_code: 1
Source/WebCore/mathml/MathMLSelectElement.cpp:40: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/mathml/MathMLSelectElement.cpp:91: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 2 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 218170 [details]
Patch v11 - bis
Comment on attachment 218170 [details] Patch v11 - bis Clearing flags on attachment: 218170 Committed r160005: <http://trac.webkit.org/changeset/160005> All reviewed patches have been landed. Closing bug. (In reply to comment #41) > can you add some more cases to cover the rules defined at > > https://developer.mozilla.org/en-US/docs/Web/MathML/Element/semantics, starting at the line > "The rules of determining the visible child in a <semantics> element are the following"' I have updated the patch on bug 100626. BTW, the MDN page should probably be updated to clarify that the rules are Gecko's one (and will be used in WebKit too). Mass change: add WebExposed keyword to help MDN documentation. |