Reviewed by Adam.
authoradele <adele@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 19 Mar 2007 18:21:52 +0000 (18:21 +0000)
committeradele <adele@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 19 Mar 2007 18:21:52 +0000 (18:21 +0000)
        Adding comments and a little cleanup from my last checkin.

        * html/HTMLSelectElement.cpp:
        (WebCore::HTMLSelectElement::setSelectedIndex): Removed commented out assert.  Added comment about how we use onChange.
        (WebCore::HTMLSelectElement::selectAll): Added comment about how we use saveLastSelection and onChange.
        (WebCore::HTMLSelectElement::dispatchFocusEvent): ditto.
        (WebCore::HTMLSelectElement::dispatchBlurEvent): ditto.
        (WebCore::HTMLSelectElement::menuListDefaultEventHandler): ditto.
        (WebCore::HTMLSelectElement::listBoxDefaultEventHandler): ditto.
        (WebCore::HTMLSelectElement::menuListOnChange): Added assert that usesMenuList() is true.
        (WebCore::HTMLSelectElement::listBoxOnChange): Added assert that usesMenuList() is false.
        (WebCore::HTMLSelectElement::saveLastSelection): Added early return for menu lists.

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

WebCore/ChangeLog
WebCore/html/HTMLSelectElement.cpp

index 2bed51ab15dadf8be2c4587566fcd48856ab22b5..82cfc534b9baa88497abd294d3e75d8f555953c5 100644 (file)
@@ -1,3 +1,20 @@
+2007-03-19  Adele Peterson  <adele@apple.com>
+
+        Reviewed by Adam.
+
+        Adding comments and a little cleanup from my last checkin.
+
+        * html/HTMLSelectElement.cpp:
+        (WebCore::HTMLSelectElement::setSelectedIndex): Removed commented out assert.  Added comment about how we use onChange.
+        (WebCore::HTMLSelectElement::selectAll): Added comment about how we use saveLastSelection and onChange.
+        (WebCore::HTMLSelectElement::dispatchFocusEvent): ditto.
+        (WebCore::HTMLSelectElement::dispatchBlurEvent): ditto.
+        (WebCore::HTMLSelectElement::menuListDefaultEventHandler): ditto.
+        (WebCore::HTMLSelectElement::listBoxDefaultEventHandler): ditto.
+        (WebCore::HTMLSelectElement::menuListOnChange): Added assert that usesMenuList() is true.
+        (WebCore::HTMLSelectElement::listBoxOnChange): Added assert that usesMenuList() is false.
+        (WebCore::HTMLSelectElement::saveLastSelection): Added early return for menu lists.
+
 2007-03-19  Mitz Pettel  <mitz@webkit.org>
 
         Reviewed by Tim Hatcher.
index 9955095109b1ee1938b56d04784eb55388103505..5dffc05c91f7513a41a76315701ab5423bd26c68 100644 (file)
@@ -188,7 +188,7 @@ void HTMLSelectElement::setSelectedIndex(int optionIndex, bool deselect, bool fi
 
     scrollToSelection();
 
-    //ASSERT(m_lastOnChangeIndex == -1 || m_lastOnChangeIndex == optionIndex);
+    // This only gets called with fireOnChange for menu lists. 
     if (fireOnChange && usesMenuList())
         menuListOnChange();
 }
@@ -404,6 +404,7 @@ void HTMLSelectElement::selectAll()
     if (!renderer() || !multiple())
         return;
     
+    // Save the selection so it can be compared to the new selectAll selection when we call onChange
     saveLastSelection();
     
     m_activeSelectionState = true;
@@ -573,6 +574,7 @@ void HTMLSelectElement::dispatchFocusEvent()
 {
 #if !ARROW_KEYS_POP_MENU
     if (usesMenuList())
+        // Save the selection so it can be compared to the new selection when we call onChange during dispatchBlurEvent.
         saveLastSelection();
 #endif
     HTMLGenericFormElement::dispatchFocusEvent();
@@ -581,6 +583,8 @@ void HTMLSelectElement::dispatchFocusEvent()
 void HTMLSelectElement::dispatchBlurEvent()
 {
 #if !ARROW_KEYS_POP_MENU
+    // We only need to fire onChange here for menu lists, because we fire onChange for list boxes whenever the selection change is actually made.
+    // This matches other browsers' behavior.
     if (usesMenuList())
         menuListOnChange();
 #endif
@@ -632,6 +636,8 @@ void HTMLSelectElement::menuListDefaultEventHandler(Event* evt)
         }
         if ((keyIdentifier == "Down" || keyIdentifier == "Up" || keyIdentifier == "U+000020") && renderer() && usesMenuList()) {
             focus();
+            // Save the selection so it can be compared to the new selection when we call onChange during setSelectedIndex,
+            // which gets called from RenderMenuList::valueChanged, which gets called after the user makes a selection from the menu.
             saveLastSelection();
             menuList->showPopup();
             handled = true;
@@ -657,6 +663,7 @@ void HTMLSelectElement::menuListDefaultEventHandler(Event* evt)
                 setSelectedIndex(listToOptionIndex(listIndex));
             handled = true;
         } else if (keyIdentifier == "Enter") {
+            // Save the selection so it can be compared to the new selection when we call onChange during setSetSelectedIndex.
             saveLastSelection();
             setSelectedIndex(listToOptionIndex(listIndex), true, true);
         }
@@ -670,6 +677,8 @@ void HTMLSelectElement::menuListDefaultEventHandler(Event* evt)
         if (menuList->popupIsVisible())
             menuList->hidePopup();
         else {
+            // Save the selection so it can be compared to the new selection when we call onChange during setSelectedIndex,
+            // which gets called from RenderMenuList::valueChanged, which gets called after the user makes a selection from the menu.
             saveLastSelection();
             menuList->showPopup();
         }
@@ -688,6 +697,7 @@ void HTMLSelectElement::listBoxDefaultEventHandler(Event* evt)
         MouseEvent* mEvt = static_cast<MouseEvent*>(evt);
         int listIndex = static_cast<RenderListBox*>(renderer())->listIndexAtOffset(mEvt->offsetX(), mEvt->offsetY());
         if (listIndex >= 0) {
+            // Save the selection so it can be compared to the new selection when we call onChange during mouseup, or after autoscroll finishes.
             saveLastSelection();
 
             m_activeSelectionState = true;
@@ -774,6 +784,7 @@ void HTMLSelectElement::listBoxDefaultEventHandler(Event* evt)
         }
         
         if (keyIdentifier == "Down" || keyIdentifier == "Up") {
+            // Save the selection so it can be compared to the new selection when we call onChange immediately after making the new selection.
             saveLastSelection();
 
             ASSERT(endIndex >= 0 && (unsigned)endIndex < listItems().size()); 
@@ -842,12 +853,15 @@ void HTMLSelectElement::updateListBoxSelection(bool deselectOtherOptions)
 
 void HTMLSelectElement::menuListOnChange()
 {
+    ASSERT(usesMenuList());
     if (m_lastOnChangeIndex != selectedIndex())
         onChange();
 }
 
 void HTMLSelectElement::listBoxOnChange()
 {
+    ASSERT(!usesMenuList());
+
     const Vector<HTMLElement*>& items = listItems();
     
     // If the cached selection list is empty, or the size has changed, then fire onChange, and return early.
@@ -876,15 +890,16 @@ void HTMLSelectElement::saveLastSelection()
 
     if (usesMenuList()) {
         m_lastOnChangeIndex = selectedIndex();
-    } else {
-        m_lastOnChangeSelection.clear();
-        for (unsigned i = 0; i < items.size(); i++) {
-            if (items[i]->hasLocalName(optionTag)) {
-                HTMLOptionElement* option = static_cast<HTMLOptionElement*>(items[i]);
-                m_lastOnChangeSelection.append(option->selected());
-            } else
-                m_lastOnChangeSelection.append(false);
-        }
+        return;
+    }
+    
+    m_lastOnChangeSelection.clear();
+    for (unsigned i = 0; i < items.size(); i++) {
+        if (items[i]->hasLocalName(optionTag)) {
+            HTMLOptionElement* option = static_cast<HTMLOptionElement*>(items[i]);
+            m_lastOnChangeSelection.append(option->selected());
+        } else
+            m_lastOnChangeSelection.append(false);
     }
 }