Bug 935876 part.1 Don't consume key event on <select> element if it's never handled r=
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 24 Jan 2014 15:26:52 +0900
changeset 164984 5b206fc5ecb67f4dc5b99f13a9278ab24487d26c
parent 164983 23061213bdd13cc89fd115d172c5340291617110
child 164985 2e93f0faa6ad80c1260913dc54c80bce688e15d3
push id38868
push usermasayuki@d-toybox.com
push dateFri, 24 Jan 2014 06:27:00 +0000
treeherdermozilla-inbound@5b206fc5ecb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs935876
milestone29.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 935876 part.1 Don't consume key event on <select> element if it's never handled r=
layout/forms/nsListControlFrame.cpp
layout/forms/nsListControlFrame.h
--- a/layout/forms/nsListControlFrame.cpp
+++ b/layout/forms/nsListControlFrame.cpp
@@ -2070,16 +2070,18 @@ nsListControlFrame::KeyDown(nsIDOMEvent*
 {
   MOZ_ASSERT(aKeyEvent, "aKeyEvent is null.");
 
   nsEventStates eventStates = mContent->AsElement()->State();
   if (eventStates.HasState(NS_EVENT_STATE_DISABLED)) {
     return NS_OK;
   }
 
+  AutoIncrementalSearchResetter incrementalSearchResetter;
+
   // Don't check defaultPrevented value because other browsers don't prevent
   // the key navigation of list control even if preventDefault() is called.
 
   const WidgetKeyboardEvent* keyEvent =
     aKeyEvent->GetInternalNSEvent()->AsKeyboardEvent();
   MOZ_ASSERT(keyEvent,
     "DOM event must have WidgetKeyboardEvent for its internal event");
 
@@ -2119,51 +2121,55 @@ nsListControlFrame::KeyDown(nsIDOMEvent*
       break;
     case NS_VK_DOWN:
     case NS_VK_RIGHT:
       AdjustIndexForDisabledOpt(mEndSelectionIndex, newIndex,
                                 static_cast<int32_t>(numOptions),
                                 1, 1);
       break;
     case NS_VK_RETURN:
-      if (mComboboxFrame) {
-        nsWeakFrame weakFrame(this);
+      if (IsInDropDownMode()) {
+        // If the select element is a dropdown style, Enter key should be
+        // consumed everytime since Enter key may be pressed accidentally after
+        // the dropdown is closed by Enter key press.
+        aKeyEvent->PreventDefault();
+
         if (mComboboxFrame->IsDroppedDown()) {
-          // At closing dropdown, users may not expect there is additional
-          // behavior for this key event.  Therefore, let's consume the event.
-          aKeyEvent->PreventDefault();
+          nsWeakFrame weakFrame(this);
           ComboboxFinish(mEndSelectionIndex);
           if (!weakFrame.IsAlive()) {
             return NS_OK;
           }
         }
+        // XXX This is strange. On other browsers, "change" event is fired
+        //     immediately after the selected item is changed rather than
+        //     Enter key is pressed.
         FireOnChange();
-        if (!weakFrame.IsAlive()) {
-          // If the keydown event causes destroying this, fired keypress on
-          // another element may cause another action which may not be
-          // expected by the user.
-          aKeyEvent->PreventDefault();
-        }
         return NS_OK;
       }
+
+      // If this is single select listbox, Enter key doesn't cause anything.
+      if (!GetMultiple()) {
+        return NS_OK;
+      }
+
       newIndex = mEndSelectionIndex;
       break;
     case NS_VK_ESCAPE: {
-      nsWeakFrame weakFrame(this);
-      // XXX When the Escape keydown causes closing dropdown, it shouldn't
-      //     cause any additonal actions. We should call preventDefault() here.
-      AboutToRollup();
-      if (!weakFrame.IsAlive()) {
-        // If the keydown event causes destroying this, fired keypress on
-        // another element may cause another action which may not be
-        // expected by the user.
-        aKeyEvent->PreventDefault();
+      // If the select element is a listbox style, Escape key causes nothing.
+      if (!IsInDropDownMode()) {
         return NS_OK;
       }
-      break;
+
+      AboutToRollup();
+      // If the select element is a dropdown style, Enter key should be
+      // consumed everytime since Escape key may be pressed accidentally after
+      // the dropdown is closed by Escepe key.
+      aKeyEvent->PreventDefault();
+      return NS_OK;
     }
     case NS_VK_PAGE_UP: {
       int32_t itemsPerPage =
         std::max(1, static_cast<int32_t>(mNumDisplayRows - 1));
       AdjustIndexForDisabledOpt(mEndSelectionIndex, newIndex,
                                 static_cast<int32_t>(numOptions),
                                 -itemsPerPage, -1);
       break;
@@ -2189,40 +2195,40 @@ nsListControlFrame::KeyDown(nsIDOMEvent*
 
 #if defined(XP_WIN) || defined(XP_OS2)
     case NS_VK_F4:
       DropDownToggleKey(aKeyEvent);
       return NS_OK;
 #endif
 
     default: // printable key will be handled by keypress event.
+      incrementalSearchResetter.Cancel();
       return NS_OK;
   }
 
   aKeyEvent->PreventDefault();
 
-  // Cancel incremental search if it's being performed.
-  GetIncrementalString().Truncate();
-
   // Actually process the new index and let the selection code
   // do the scrolling for us
   PostHandleKeyEvent(newIndex, 0, keyEvent->IsShift(), isControlOrMeta);
   return NS_OK;
 }
 
 nsresult
 nsListControlFrame::KeyPress(nsIDOMEvent* aKeyEvent)
 {
   MOZ_ASSERT(aKeyEvent, "aKeyEvent is null.");
 
   nsEventStates eventStates = mContent->AsElement()->State();
   if (eventStates.HasState(NS_EVENT_STATE_DISABLED)) {
     return NS_OK;
   }
 
+  AutoIncrementalSearchResetter incrementalSearchResetter;
+
   const WidgetKeyboardEvent* keyEvent =
     aKeyEvent->GetInternalNSEvent()->AsKeyboardEvent();
   MOZ_ASSERT(keyEvent,
     "DOM event must have WidgetKeyboardEvent for its internal event");
 
   // Select option with this as the first character
   // XXX Not I18N compliant
 
@@ -2245,27 +2251,35 @@ nsListControlFrame::KeyPress(nsIDOMEvent
   bool isControlOrMeta = (keyEvent->IsControl() || keyEvent->IsMeta());
   if (isControlOrMeta && keyEvent->charCode != ' ') {
     return NS_OK;
   }
 
   // NOTE: If keyCode of keypress event is not 0, charCode is always 0.
   //       Therefore, all non-printable keys are not handled after this block.
   if (!keyEvent->charCode) {
-    // Backspace key will delete the last char in the string
-    // XXX Backspace key causes "go back the history" on Windows.  Shouldn't we
-    //     prevent its default action if incremental search is used since
-    //     getting focus?  When I tested this, it worked accidentally.
-    if (keyEvent->keyCode == NS_VK_BACK && !GetIncrementalString().IsEmpty()) {
-      GetIncrementalString().Truncate(GetIncrementalString().Length() - 1);
+    // Backspace key will delete the last char in the string.  Otherwise,
+    // non-printable keypress should reset incremental search.
+    if (keyEvent->keyCode == NS_VK_BACK) {
+      incrementalSearchResetter.Cancel();
+      if (!GetIncrementalString().IsEmpty()) {
+        GetIncrementalString().Truncate(GetIncrementalString().Length() - 1);
+      }
       aKeyEvent->PreventDefault();
+    } else {
+      // XXX When a select element has focus, even if the key causes nothing,
+      //     it might be better to call preventDefault() here because nobody
+      //     should expect one of other elements including chrome handles the
+      //     key event.
     }
     return NS_OK;
   }
 
+  incrementalSearchResetter.Cancel();
+
   // We ate the key if we got this far.
   aKeyEvent->PreventDefault();
 
   // XXX Why don't we check/modify timestamp first?
 
   // Incremental Search: if time elapsed is below
   // INCREMENTAL_SEARCH_KEYPRESS_TIME, append this keystroke to the search
   // string we will use to find options and start searching at the current
--- a/layout/forms/nsListControlFrame.h
+++ b/layout/forms/nsListControlFrame.h
@@ -440,12 +440,33 @@ protected:
 #ifdef DO_REFLOW_COUNTER
   int32_t mReflowId;
 #endif
 
 private:
   // for incremental typing navigation
   static nsAString& GetIncrementalString ();
   static DOMTimeStamp gLastKeyTime;
+
+  class MOZ_STACK_CLASS AutoIncrementalSearchResetter
+  {
+  public:
+    AutoIncrementalSearchResetter() :
+      mCancelled(false)
+    {
+    }
+    ~AutoIncrementalSearchResetter()
+    {
+      if (!mCancelled) {
+        nsListControlFrame::GetIncrementalString().Truncate();
+      }
+    }
+    void Cancel()
+    {
+      mCancelled = true;
+    }
+  private:
+    bool mCancelled;
+  };
 };
 
 #endif /* nsListControlFrame_h___ */