2009-03-17 Darin Adler <darin@apple.com>
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Mar 2009 19:42:51 +0000 (19:42 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Mar 2009 19:42:51 +0000 (19:42 +0000)
        Earlier version reviewed by Adele Peterson.

        Bug 24304: REGRESSION (r39864): Hitting the space bar to select an <input type=radio>
        or push an <input type=button> or <button> causes the page to scroll down.

        Would be best to add a regression test for Windows eventually; tested that this has
        no effect on the Mac OS X platform.

        * html/HTMLInputElement.cpp:
        (WebCore::HTMLInputElement::defaultEventHandler): Added FIXMEs and tweaked formatting.
        Use the code that calls the base class's defaultEventHandler early only in the cases
        where it's needed: keydown and keypress events in text fields. In other cases, do the
        more typical thing and call the default handler only at the end of the function.
        This function already had code to make sure the keypress event for space never gets
        through, but it was running too late since the scrolling code was moved into the
        base class default event handler.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@41781 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/html/HTMLInputElement.cpp

index c345fae..ce3b6e0 100644 (file)
@@ -1,3 +1,22 @@
+2009-03-17  Darin Adler  <darin@apple.com>
+
+        Earlier version reviewed by Adele Peterson.
+
+        Bug 24304: REGRESSION (r39864): Hitting the space bar to select an <input type=radio>
+        or push an <input type=button> or <button> causes the page to scroll down.
+
+        Would be best to add a regression test for Windows eventually; tested that this has
+        no effect on the Mac OS X platform.
+
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::defaultEventHandler): Added FIXMEs and tweaked formatting.
+        Use the code that calls the base class's defaultEventHandler early only in the cases
+        where it's needed: keydown and keypress events in text fields. In other cases, do the
+        more typical thing and call the default handler only at the end of the function.
+        This function already had code to make sure the keypress event for space never gets
+        through, but it was running too late since the scrolling code was moved into the
+        base class default event handler.
+
 2009-03-17  Simon Fraser  <simon.fraser@apple.com>
 
         Reviewed by Darin Adler
index deb1e10..1535687 100644 (file)
@@ -1082,6 +1082,9 @@ void HTMLInputElement::postDispatchEventHandler(Event *evt, void* data)
 
 void HTMLInputElement::defaultEventHandler(Event* evt)
 {
+    // FIXME: It would be better to refactor this for the different types of input element.
+    // Having them all in one giant function makes this hard to read, and almost all the handling is type-specific.
+
     bool clickDefaultFormButton = false;
 
     if (isTextField() && evt->type() == eventNames().textInputEvent && evt->isTextEvent() && static_cast<TextEvent*>(evt)->data() == "\n")
@@ -1103,20 +1106,29 @@ void HTMLInputElement::defaultEventHandler(Event* evt)
         }
     }
 
-    if (isTextField() && evt->type() == eventNames().keydownEvent && evt->isKeyboardEvent() && focused() && document()->frame()
-        && document()->frame()->doTextFieldCommandFromEvent(this, static_cast<KeyboardEvent*>(evt))) {
+    if (isTextField()
+            && evt->type() == eventNames().keydownEvent
+            && evt->isKeyboardEvent()
+            && focused()
+            && document()->frame()
+            && document()->frame()->doTextFieldCommandFromEvent(this, static_cast<KeyboardEvent*>(evt))) {
         evt->setDefaultHandled();
         return;
     }
 
-    if (inputType() == RADIO && evt->isMouseEvent()
-        && evt->type() == eventNames().clickEvent && static_cast<MouseEvent*>(evt)->button() == LeftButton) {
+    if (inputType() == RADIO
+            && evt->isMouseEvent()
+            && evt->type() == eventNames().clickEvent
+            && static_cast<MouseEvent*>(evt)->button() == LeftButton) {
         evt->setDefaultHandled();
         return;
     }
     
-    // Let the key handling done in Node take precedence over the event handling here for editable text fields
-    if (!clickDefaultFormButton) {
+    // Call the base event handler before any of our own event handling for almost all events in text fields.
+    // Makes editing keyboard handling take precedence over the keydown and keypress handling in this function.
+    bool callBaseClassEarly = isTextField() && !clickDefaultFormButton
+        && (evt->type() == eventNames().keydownEvent || evt->type() == eventNames().keypressEvent);
+    if (callBaseClassEarly) {
         HTMLFormControlElementWithState::defaultEventHandler(evt);
         if (evt->defaultHandled())
             return;
@@ -1215,7 +1227,8 @@ void HTMLInputElement::defaultEventHandler(Event* evt)
                 case SUBMIT:
                 case RADIO:
                     setActive(true, true);
-                    // No setDefaultHandled() - IE dispatches a keypress in this case.
+                    // No setDefaultHandled(), because IE dispatches a keypress in this case
+                    // and the caller will only dispatch a keypress if we don't call setDefaultHandled.
                     return;
                 default:
                     break;
@@ -1331,7 +1344,7 @@ void HTMLInputElement::defaultEventHandler(Event* evt)
             MouseEvent* mEvt = static_cast<MouseEvent*>(evt);
             if (!slider->mouseEventIsInThumb(mEvt)) {
                 IntPoint eventOffset(mEvt->offsetX(), mEvt->offsetY());
-                if (mEvt->target() != this) // Does this ever happen now? Was added for <video> controls
+                if (mEvt->target() != this) // Does this ever happen now? Was originally added for <video> controls.
                     eventOffset = roundedIntPoint(renderer()->absoluteToLocal(FloatPoint(mEvt->pageX(), mEvt->pageY()), false, true));
                 slider->setValueForPosition(slider->positionForOffset(eventOffset));
             }
@@ -1339,6 +1352,9 @@ void HTMLInputElement::defaultEventHandler(Event* evt)
         if (evt->isMouseEvent() || evt->isDragEvent() || evt->isWheelEvent())
             slider->forwardEvent(evt);
     }
+
+    if (!callBaseClassEarly && !evt->defaultHandled())
+        HTMLFormControlElementWithState::defaultEventHandler(evt);
 }
 
 bool HTMLInputElement::isURLAttribute(Attribute *attr) const