Bug 120059 - Add support for maction@toggle
: Add support for maction@toggle
Status: RESOLVED FIXED
: WebKit
MathML
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://www.w3.org/Math/draft-spec/cha...
: WebExposed
: 120058
: 85734 100626 125785
  Show dependency treegraph
 
Reported: 2013-08-20 02:31 PST by
Modified: 2014-03-10 12:26 PST (History)


Attachments
Patch V1 (9.29 KB, patch)
2013-08-20 02:38 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
no flags Review Patch | Details | Formatted Diff | Diff
Patch V2 (9.88 KB, patch)
2013-09-15 22:15 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
no flags Review Patch | Details | Formatted Diff | Diff
Patch V3 (9.98 KB, patch)
2013-09-16 06:26 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
bfulgham: review-
Review Patch | Details | Formatted Diff | Diff
Patch V4 (10.30 KB, patch)
2013-11-27 05:24 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
no flags Review Patch | Details | Formatted Diff | Diff
Patch V5 (10.29 KB, patch)
2013-12-03 12:07 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
darin: review-
darin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch V6 (11.53 KB, patch)
2013-12-10 01:31 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
darin: review+
darin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch Final Version (13.07 KB, patch)
2013-12-12 05:41 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
no flags Review Patch | Details | Formatted Diff | Diff
Patch (Final Version) (13.05 KB, patch)
2013-12-12 05:53 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
darin: review+
darin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (Final Version) (13.05 KB, patch)
2013-12-12 07:57 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
no flags Review Patch | Details | Formatted Diff | Diff
Patch (Final Version) (13.06 KB, patch)
2013-12-15 22:03 PST, Frédéric Wang (:fredw) | away 27/04 to 06/05
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2013-08-20 02:38:00 PST -------
Created an attachment (id=209175) [details]
Patch V1
------- Comment #2 From 2013-08-21 16:13:37 PST -------
(From update of attachment 209175 [details])
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
------- Comment #3 From 2013-08-22 00:20:04 PST -------
(I forgot to mention that, but of course the patch applies on top of the one from bug 120058)
------- Comment #4 From 2013-09-15 22:15:42 PST -------
Created an attachment (id=211738) [details]
Patch V2
------- Comment #5 From 2013-09-16 06:26:15 PST -------
Created an attachment (id=211773) [details]
Patch V3

Refresh again after the raw pointer => ref pointer changes.
------- Comment #6 From 2013-11-26 17:13:24 PST -------
(From update of attachment 211773 [details])
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;
}
------- Comment #7 From 2013-11-27 05:24:23 PST -------
Created an attachment (id=217943) [details]
Patch V4
------- Comment #8 From 2013-12-03 12:07:11 PST -------
Created an attachment (id=218321) [details]
Patch V5
------- Comment #9 From 2013-12-03 14:57:22 PST -------
(From update of attachment 218321 [details])
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.
------- Comment #10 From 2013-12-04 11:23:23 PST -------
(From update of attachment 218321 [details])
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.
------- Comment #11 From 2013-12-09 10:31:25 PST -------
(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.
------- Comment #12 From 2013-12-09 11:46:24 PST -------
(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.
------- Comment #13 From 2013-12-10 01:31:41 PST -------
Created an attachment (id=218844) [details]
Patch V6
------- Comment #14 From 2013-12-10 01:35:02 PST -------
(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...
------- Comment #15 From 2013-12-11 09:16:18 PST -------
(From update of attachment 218844 [details])
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
------- Comment #16 From 2013-12-11 09:25:55 PST -------
(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...
------- Comment #17 From 2013-12-11 14:51:48 PST -------
(From update of attachment 218844 [details])
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.
------- Comment #18 From 2013-12-12 05:41:14 PST -------
Created an attachment (id=219077) [details]
Patch Final Version
------- Comment #19 From 2013-12-12 05:43:26 PST -------
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.
------- Comment #20 From 2013-12-12 05:53:32 PST -------
Created an attachment (id=219078) [details]
Patch (Final Version)
------- Comment #21 From 2013-12-12 07:44:23 PST -------
(From update of attachment 219078 [details])
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.
------- Comment #22 From 2013-12-12 07:57:21 PST -------
Created an attachment (id=219085) [details]
Patch (Final Version)

oops, mixing up with Gecko's convention... sorry.
------- Comment #23 From 2013-12-15 21:32:06 PST -------
Can someone please commit the latest version?
------- Comment #24 From 2013-12-15 21:50:32 PST -------
(From update of attachment 219085 [details])
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
------- Comment #25 From 2013-12-15 22:03:00 PST -------
Created an attachment (id=219296) [details]
Patch (Final Version)
------- Comment #26 From 2013-12-15 22:50:26 PST -------
(From update of attachment 219296 [details])
Clearing flags on attachment: 219296

Committed r160631: <http://trac.webkit.org/changeset/160631>
------- Comment #27 From 2013-12-15 22:50:31 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #28 From 2013-12-16 10:15:25 PST -------
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.
------- Comment #29 From 2013-12-16 10:19:40 PST -------
Some follow-up changes in https://bugs.webkit.org/show_bug.cgi?id=125785
------- Comment #30 From 2014-03-10 12:26:09 PST -------
Mass change: add WebExposed keyword to help MDN documentation.