Bug 120059 – Add support for maction@toggle

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

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

Some tests are available here:
https://developer.mozilla.org/fr/docs/Mozilla/MathML_Project/Various

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

Created attachment 209175 [details]
Patch V1

[chris fleizach]

Comment on attachment 209175 [details]
Patch V1

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

> LayoutTests/mathml/presentation/maction-toggle.html:5
> +    <meta charset="utf-8"/>

this looks like it could be removed

> LayoutTests/mathml/presentation/maction-toggle.html:9
> +      var event = new MouseEvent('click', {bubbles: bubble, cancelable: true});

need some more indentation here

> LayoutTests/mathml/presentation/maction-toggle.html:17
> +      click('m1'); click('m1'); click('m1');

indentation needed

you might add a comment here explaining the different clicking happening

> Source/WebCore/mathml/MathMLSelectElement.cpp:42
> +class MactionEventListener : public EventListener {

I think the name should be MActionEventListener,

sort of like GObjectEventListener

> Source/WebCore/mathml/MathMLSelectElement.cpp:163
> +    int selection = (hasAttribute(MathMLNames::selectionAttr) ? getAttribute(MathMLNames::selectionAttr).toInt(&ok) : 1);

looks like this int could be unsigned

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

(I forgot to mention that, but of course the patch applies on top of the one from bug 120058)

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

Created attachment 211738 [details]
Patch V2

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

Created attachment 211773 [details]
Patch V3

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

[Brent Fulgham]

Comment on attachment 211773 [details]
Patch V3

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

A few minor changes, based in part of comments Darin made for Bug 120058.

> Source/WebCore/mathml/MathMLSelectElement.cpp:162
> +    unsigned int selection = (hasAttribute(MathMLNames::selectionAttr) ? getAttribute(MathMLNames::selectionAttr).toInt(&ok) : 1);

This should be "fastGetAttribute(MathMLNames::selectionAttr) (see Darin's comments in 120058).

> Source/WebCore/mathml/MathMLSelectElement.cpp:172
> +    }

Please revise this logic as Darin asked in Bug 120058:

int i = 1;
for (; i <= selection; i++) {
    Element* nextChild = child->nextElementSibling();
    if (!nextChild)
        break;
    child = nextChild;
}

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

Created attachment 217943 [details]
Patch V4

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

Created attachment 218321 [details]
Patch V5

[Darin Adler]

Comment on attachment 218321 [details]
Patch V5

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

> Source/WebCore/mathml/MathMLSelectElement.cpp:40
> +class MActionEventListener : public EventListener {

I don’t think we need an event listener for this. This should just be done in the default event handler for the MathMLSelectElement.

> Source/WebCore/mathml/MathMLSelectElement.cpp:42
> +    static PassRefPtr<MActionEventListener> create(MathMLSelectElement* maction) { return adoptRef(new MActionEventListener(maction)); }

Should return a PassRef rather than PassRefPtr. Should take MathMLSelectElement& instead of MathMLSelectElement*.

> Source/WebCore/mathml/MathMLSelectElement.cpp:43
> +    static const MActionEventListener* cast(const EventListener* listener)

Do we really need this function? The one call site below would be clearer without it.

> Source/WebCore/mathml/MathMLSelectElement.cpp:47
> +            : 0;

nullptr

> Source/WebCore/mathml/MathMLSelectElement.cpp:49
> +    virtual bool operator==(const EventListener& other);

Should be marked OVERRIDE.

> Source/WebCore/mathml/MathMLSelectElement.cpp:52
> +    MActionEventListener(MathMLSelectElement* maction)

Should mark this explicit. Should take a reference instead of a pointer.

> Source/WebCore/mathml/MathMLSelectElement.cpp:58
> +    virtual void handleEvent(ScriptExecutionContext*, Event*);

Needs OVERRIDE.

> Source/WebCore/mathml/MathMLSelectElement.cpp:59
> +    MathMLSelectElement* m_maction;

Should be a reference instead of pointer.

> Source/WebCore/mathml/MathMLSelectElement.cpp:68
> +        RefPtr<EventListener> listener = MActionEventListener::create(this);
> +        addEventListener("click", listener.release(), false);

Would read better as a one-liner. Also should use the clickEvent event name rather than “click”. Also not clear this should be done with addEventListener. Why can’t this just be in the default event handler for MathMLSelectElement?

> Source/WebCore/mathml/MathMLSelectElement.cpp:75
> +    if (event->defaultPrevented())
> +        return;

Surprised this is needed and not handled by the caller.

> Source/WebCore/mathml/MathMLSelectElement.cpp:190
> +    // We get the current value of the selection attribute.
> +    int selection = fastGetAttribute(MathMLNames::selectionAttr).toInt();
> +    if (selection < 1)
> +        selection = 1;
> +
> +    // We find the successor of the current selection.
> +    selection++;
> +    Element* child = firstElementChild();
> +    if (child) {
> +        for (int i = 1; i < selection; i++) {
> +            child = child->nextElementSibling();
> +            if (!child)
> +                break;
> +        }
> +    }
> +
> +    // If we reach the end of the child list, we go back to the first child.
> +    if (!child)
> +        selection = 1;
> +
> +    // We update the attribute value of the selection attribute.
> +    // This will also call MathMLSelectElement::attributeChanged to update the selected child.
> +    setAttribute(MathMLNames::selectionAttr, String::number(selection));

Can write this much more concisely:

    int selection = std::max(1, fastGetAttribute(MathMLNames::selectionAttr).toInt()) + 1;
    if (selection > childElementCount())
        selection = 1;
    setAttribute(MathMLNames::selectionAttr, String::number(selection));

The extra cost of counting all the children seems quite affordable; does not make the worst case any worse.

[Darin Adler]

Comment on attachment 218321 [details]
Patch V5

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

>> Source/WebCore/mathml/MathMLSelectElement.cpp:190
>> +    setAttribute(MathMLNames::selectionAttr, String::number(selection));
> 
> Can write this much more concisely:
> 
>     int selection = std::max(1, fastGetAttribute(MathMLNames::selectionAttr).toInt()) + 1;
>     if (selection > childElementCount())
>         selection = 1;
>     setAttribute(MathMLNames::selectionAttr, String::number(selection));
> 
> The extra cost of counting all the children seems quite affordable; does not make the worst case any worse.

Hmm, I just noticed that my code won’t do the right thing if passed super-huge numbers. I’d love to have test cases covering that. We should probably use unsigned instead of int here. Need test coverage for edge cases, non-numbers, maximum numbers, higher than maximum numbers, negative numbers, zero, numbers with leading spaces.

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

(In reply to comment #9)
> I don’t think we need an event listener for this. This should just be done in the default event handler for the MathMLSelectElement.
> 
> Do we really need this function? The one call site below would be clearer without it.
> 
> Also not clear this should be done with addEventListener. Why can’t this just be in the default event handler for MathMLSelectElement?

I don't know well the WebKit code. I saw that other elements register events that way, but I'm happy to use the "standard" way if you tell me what it is.

[Darin Adler]

(In reply to comment #11)
> I saw that other elements register events that way

Could you give me a list the examples you found? If they are trying to implement default event handling by using event listeners I believe they are wrong so I would like to fix them.

(In reply to comment #11)
> I'm happy to use the "standard" way if you tell me what it is.

Some examples that might help you write a good default event handler are:

    HTMLButtonElement::defaultEventHandler
    HTMLSummaryElement::defaultEventHandler

The general pattern is that the defaultEventHandler function checks the type of the event. If it knows how to handle the event, it does so, then calls setDefaultHandled on the event and returns. After that, in the case where the event was not handled it calls the base class’s defaultEventHandler function.

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

Created attachment 218844 [details]
Patch V6

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

(In reply to comment #12)
> Could you give me a list the examples you found? If they are trying to implement default event handling by using event listeners I believe they are wrong so I would like to fix them.

Not sure I remember exactly. I probably just searched of EventListener or similar in the source code and found these elements using EventListener.h. So I thought that was how it is supposed to be handled...

> The general pattern is that the defaultEventHandler function checks the type of the event. If it knows how to handle the event, it does so, then calls setDefaultHandled on the event and returns. After that, in the case where the event was not handled it calls the base class’s defaultEventHandler function.

Thanks. I didn't do setDefaultHandled, since in that case the event is not bubble to the parent when bubble = true (see the tests). Not sure if that is the right thing...

[chris fleizach]

Comment on attachment 218844 [details]
Patch V6

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

> Source/WebCore/mathml/MathMLSelectElement.cpp:123
> +        if (getAttribute(MathMLNames::actiontypeAttr) == "toggle") {

can this be fastGetAttribute()

> Source/WebCore/mathml/MathMLSelectElement.cpp:124
> +            toggle();

I think you also want to set
event->setDefaultHandled();
here

> Source/WebCore/mathml/MathMLSelectElement.h:38
> +    void toggle();

doesn't look like toggle() needs to be public

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

(In reply to comment #15)
> > Source/WebCore/mathml/MathMLSelectElement.cpp:124
> > +            toggle();
> 
> I think you also want to set
> event->setDefaultHandled();
> here

I tried that but then the test for bubble = true no longer works. Not sure why...

[Darin Adler]

Comment on attachment 218844 [details]
Patch V6

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

r=me; OK to land as is, but please fix as many of the comments as possible before landing. Other improvements can be done later.

> Source/WebCore/mathml/MathMLSelectElement.cpp:122
> +    if (event->type() == eventNames().clickEvent && event->isMouseEvent()) {

There’s no reason to check isMouseEvent here. Please remove that check.

>> Source/WebCore/mathml/MathMLSelectElement.cpp:123
>> +        if (getAttribute(MathMLNames::actiontypeAttr) == "toggle") {
> 
> can this be fastGetAttribute()

Yes, this should be fastGetAttribute.

>> Source/WebCore/mathml/MathMLSelectElement.cpp:124
>> +            toggle();
> 
> I think you also want to set
> event->setDefaultHandled();
> here

Calling setDefaultHandled should *not* break bubbling. I am quite surprised that it does. We should look into it more.

> Source/WebCore/mathml/MathMLSelectElement.cpp:150
> +    int selection = 1;
> +    if (m_selectedChild) {
> +        Element* child = firstElementChild();
> +        while (child && child != m_selectedChild->nextElementSibling()) {
> +            child = child->nextElementSibling();
> +            selection++;
> +        }
> +        // If we reach the end of the child list, we go back to the first child.
> +        if (!child)
> +            selection = 1;
> +    }

This way of writing the code makes a simple operation look too confusing. We need to write this in terms of helper functions so we don’t need to write out the while loop. I suggest adding a childElementIndex helper function and using that.

> Source/WebCore/mathml/MathMLSelectElement.cpp:154
> +    setAttribute(MathMLNames::selectionAttr, String::number(selection));

Should be AtomicString::number rather than String::number. Same for any other function using String::number but then passing it to setAttribute.

> Source/WebCore/mathml/MathMLSelectElement.h:30
> +#include "Event.h"

Not correct to include a header just so we can compile an Event*. And also unneeded. Base class header was already able to compile an Event* so no include is needed.

>> Source/WebCore/mathml/MathMLSelectElement.h:38
>> +    void toggle();
> 
> doesn't look like toggle() needs to be public

I agree. It should be private.

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

Created attachment 219077 [details]
Patch Final Version

[WebKit Commit Bot]

Attachment 219077 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/mathml/presentation/maction-toggle-expected.html', u'LayoutTests/mathml/presentation/maction-toggle.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/mathml/MathMLSelectElement.cpp', u'Source/WebCore/mathml/MathMLSelectElement.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/mathml/MathMLSelectElement.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

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

Created attachment 219078 [details]
Patch (Final Version)

[Darin Adler]

Comment on attachment 219078 [details]
Patch (Final Version)

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

> Source/WebCore/mathml/MathMLSelectElement.cpp:81
> +int
> +MathMLSelectElement::getSelectedChildAndIndex(Element*& selectedChild)

Formatting here is wrong. We don’t put the return value on a different line from the rest of the function declaration.

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

Created attachment 219085 [details]
Patch (Final Version)

oops, mixing up with Gecko's convention... sorry.

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

Can someone please commit the latest version?

[chris fleizach]

Comment on attachment 219085 [details]
Patch (Final Version)

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

> Source/WebCore/mathml/MathMLSelectElement.cpp:154
> +    Element* child;

I think child needs to be initialized to nullptr here

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

Created attachment 219296 [details]
Patch (Final Version)

[WebKit Commit Bot]

Comment on attachment 219296 [details]
Patch (Final Version)

Clearing flags on attachment: 219296

Committed r160631: <http://trac.webkit.org/changeset/160631>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

The bubbling test case in this patch is wrong. Telling an event to bubble does mean that event handlers will see it, but it does not mean that the same event can trigger default event handling on two different elements.

[Darin Adler]

Some follow-up changes in https://bugs.webkit.org/show_bug.cgi?id=125785

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

Mass change: add WebExposed keyword to help MDN documentation.