Bug 120058 – Add an MathMLSelectElement class to implement <maction> and <semantics>

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

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

maction/semantics both select one child to be displayed and could share the implementation.

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

Created attachment 209170 [details]
Patch V1

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

Created attachment 211644 [details]
Patch V2

refreshing the patch

[Build Bot]

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

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

Created attachment 211645 [details]
Patch V3

try to fix xcode makefile for mac

[Build Bot]

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

[Build Bot]

Comment on attachment 211645 [details]
Patch V3

Attachment 211645 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1877365

[Build Bot]

Comment on attachment 211645 [details]
Patch V3

Attachment 211645 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1817405

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

Created attachment 211664 [details]
Patch V4

Trying to fix the build files on mac and windows again.

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

Created attachment 211694 [details]
Patch V5

Refresh patch again after the mmultiscript commit.

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

Created attachment 211770 [details]
Patch V6

Refresh again after the raw pointer => ref pointer changes.

[Darin Adler]

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.

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

(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."

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

Created attachment 212332 [details]
Patch V7

[EFL EWS Bot]

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

[Build Bot]

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

[EFL EWS Bot]

Comment on attachment 212332 [details]
Patch V7

Attachment 212332 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1917197

[Build Bot]

Comment on attachment 212332 [details]
Patch V7

Attachment 212332 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1836535

[kov's GTK+ EWS bot]

Comment on attachment 212332 [details]
Patch V7

Attachment 212332 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1836552

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

Created attachment 212337 [details]
Patch V7 - bis

refresh patch to fix compilation errors due to pointer => reference change on master.

[Darin Adler]

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.

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

(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?

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

Created attachment 212371 [details]
Patch V8

[Darin Adler]

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.

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

(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.

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

(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.

[Brent Fulgham]

What is the state of this patch? Is additional work needed before it can be landed?

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

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).

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

Created attachment 216535 [details]
Patch V9

Refreshing the patch. The SVG in semantics seems to be broken by bug 124128.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

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

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).

[Build Bot]

Comment on attachment 216535 [details]
Patch V9

Attachment 216535 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22599414

[Brent Fulgham]

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.

[Brent Fulgham]

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?

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

(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)

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

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>).

[chris fleizach]

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

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

(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.

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

Created attachment 218169 [details]
Patch v11

[WebKit Commit Bot]

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.

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

Created attachment 218170 [details]
Patch v11 - bis

[WebKit Commit Bot]

Comment on attachment 218170 [details]
Patch v11 - bis

Clearing flags on attachment: 218170

Committed r160005: <http://trac.webkit.org/changeset/160005>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

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

(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).

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

Mass change: add WebExposed keyword to help MDN documentation.