Bug 120059 - Add support for maction@toggle
: Add support for maction@toggle
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: MathML
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Frédéric Wang (:fredw)
http://www.w3.org/Math/draft-spec/cha...
: WebExposed
Depends on: 120058
Blocks: maction 100626 125785
  Show dependency treegraph
 
Reported: 2013-08-20 02:31 PDT by Frédéric Wang (:fredw)
Modified: 2014-03-10 12:26 PDT (History)
8 users (show)

See Also:


Attachments
Patch V1 (9.29 KB, patch)
2013-08-20 02:38 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch V2 (9.88 KB, patch)
2013-09-15 22:15 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch V3 (9.98 KB, patch)
2013-09-16 06:26 PDT, Frédéric Wang (:fredw)
bfulgham: review-
Details | Formatted Diff | Diff
Patch V4 (10.30 KB, patch)
2013-11-27 05:24 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch V5 (10.29 KB, patch)
2013-12-03 12:07 PST, Frédéric Wang (:fredw)
darin: review-
darin: commit‑queue-
Details | Formatted Diff | Diff
Patch V6 (11.53 KB, patch)
2013-12-10 01:31 PST, Frédéric Wang (:fredw)
darin: review+
darin: commit‑queue-
Details | Formatted Diff | Diff
Patch Final Version (13.07 KB, patch)
2013-12-12 05:41 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (Final Version) (13.05 KB, patch)
2013-12-12 05:53 PST, Frédéric Wang (:fredw)
darin: review+
darin: commit‑queue-
Details | Formatted Diff | Diff
Patch (Final Version) (13.05 KB, patch)
2013-12-12 07:57 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (Final Version) (13.06 KB, patch)
2013-12-15 22:03 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.
Description Frédéric Wang (:fredw) 2013-08-20 02:31:21 PDT
Some tests are available here:
https://developer.mozilla.org/fr/docs/Mozilla/MathML_Project/Various
Comment 1 Frédéric Wang (:fredw) 2013-08-20 02:38:00 PDT
Created attachment 209175 [details]
Patch V1
Comment 2 chris fleizach 2013-08-21 16:13:37 PDT
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
Comment 3 Frédéric Wang (:fredw) 2013-08-22 00:20:04 PDT
(I forgot to mention that, but of course the patch applies on top of the one from bug 120058)
Comment 4 Frédéric Wang (:fredw) 2013-09-15 22:15:42 PDT
Created attachment 211738 [details]
Patch V2
Comment 5 Frédéric Wang (:fredw) 2013-09-16 06:26:15 PDT
Created attachment 211773 [details]
Patch V3

Refresh again after the raw pointer => ref pointer changes.
Comment 6 Brent Fulgham 2013-11-26 17:13:24 PST
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;
}
Comment 7 Frédéric Wang (:fredw) 2013-11-27 05:24:23 PST
Created attachment 217943 [details]
Patch V4
Comment 8 Frédéric Wang (:fredw) 2013-12-03 12:07:11 PST
Created attachment 218321 [details]
Patch V5
Comment 9 Darin Adler 2013-12-03 14:57:22 PST
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.
Comment 10 Darin Adler 2013-12-04 11:23:23 PST
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.
Comment 11 Frédéric Wang (:fredw) 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 Darin Adler 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 Frédéric Wang (:fredw) 2013-12-10 01:31:41 PST
Created attachment 218844 [details]
Patch V6
Comment 14 Frédéric Wang (:fredw) 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 chris fleizach 2013-12-11 09:16:18 PST
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
Comment 16 Frédéric Wang (:fredw) 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 Darin Adler 2013-12-11 14:51:48 PST
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.
Comment 18 Frédéric Wang (:fredw) 2013-12-12 05:41:14 PST
Created attachment 219077 [details]
Patch Final Version
Comment 19 WebKit Commit Bot 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 Frédéric Wang (:fredw) 2013-12-12 05:53:32 PST
Created attachment 219078 [details]
Patch (Final Version)
Comment 21 Darin Adler 2013-12-12 07:44:23 PST
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.
Comment 22 Frédéric Wang (:fredw) 2013-12-12 07:57:21 PST
Created attachment 219085 [details]
Patch (Final Version)

oops, mixing up with Gecko's convention... sorry.
Comment 23 Frédéric Wang (:fredw) 2013-12-15 21:32:06 PST
Can someone please commit the latest version?
Comment 24 chris fleizach 2013-12-15 21:50:32 PST
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
Comment 25 Frédéric Wang (:fredw) 2013-12-15 22:03:00 PST
Created attachment 219296 [details]
Patch (Final Version)
Comment 26 WebKit Commit Bot 2013-12-15 22:50:26 PST
Comment on attachment 219296 [details]
Patch (Final Version)

Clearing flags on attachment: 219296

Committed r160631: <http://trac.webkit.org/changeset/160631>
Comment 27 WebKit Commit Bot 2013-12-15 22:50:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Darin Adler 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 Darin Adler 2013-12-16 10:19:40 PST
Some follow-up changes in https://bugs.webkit.org/show_bug.cgi?id=125785
Comment 30 Frédéric Wang (:fredw) 2014-03-10 12:26:09 PDT
Mass change: add WebExposed keyword to help MDN documentation.