Summary: | Add support for maction@toggle | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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, cfleizach, commit-queue, darin, dbarton, esprehn+autocc, kangil.han, mrobinson | ||||||||||||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||
URL: | http://www.w3.org/Math/draft-spec/chapter3.html#presm.maction | ||||||||||||||||||||||||
Bug Depends on: | 120058 | ||||||||||||||||||||||||
Bug Blocks: | 85734, 100626, 125785 | ||||||||||||||||||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2013-08-20 02:31:21 PDT
Created attachment 209175 [details]
Patch V1
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 (I forgot to mention that, but of course the patch applies on top of the one from bug 120058) Created attachment 211738 [details]
Patch V2
Created attachment 211773 [details]
Patch V3
Refresh again after the raw pointer => ref pointer changes.
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; } Created attachment 217943 [details]
Patch V4
Created attachment 218321 [details]
Patch V5
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 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. (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. (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. Created attachment 218844 [details]
Patch V6
(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 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 (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 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. Created attachment 219077 [details]
Patch Final Version
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.
Created attachment 219078 [details]
Patch (Final Version)
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. Created attachment 219085 [details]
Patch (Final Version)
oops, mixing up with Gecko's convention... sorry.
Can someone please commit the latest version? 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 Created attachment 219296 [details]
Patch (Final Version)
Comment on attachment 219296 [details] Patch (Final Version) Clearing flags on attachment: 219296 Committed r160631: <http://trac.webkit.org/changeset/160631> All reviewed patches have been landed. Closing bug. 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. Some follow-up changes in https://bugs.webkit.org/show_bug.cgi?id=125785 Mass change: add WebExposed keyword to help MDN documentation. |