Bug 23721 – Changing dropdown's selectedIndex within onchange handler fires another onchange

• Component: Forms
• Status: RESOLVED FIXED

[jasneet]

I Steps:
Go to http://chatbotgame.com/?cmd=chat_s

II Issue:
What happens on this site, is if you answer cancel in the confirmation dialog, and then blur the dropdown (say by scrolling the page), it will submit the form.

<select name="filter" onchange="if (this.selectedIndex == 0 || confirm('Turn safe spy off?  If you 
are younger than 18, click Cancel.') ) document.safeSpyForm.submit(); else this.selectedIndex = 
0;">
       <option value="on" selected="selected">I'm 13+</option>      
       <option value="off" >I'm 18+</option>      
    </select> 

The problem is in Chromium + Safari, if you change the selectedIndex from within the onchange handler (notably while it still has focus), then once you blur if the index has changed it fires another onchange event.

III Other Browsers:
IE7: ok
FF3: ok

IV Nightly tested: 40471

Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=4251

[jasneet]

Created attachment 27303 [details]
testcase

[Alexey Proskuryakov]

The bug title is somewhat misleading - onchange if fired when deselecting the SELECT, not when changing it programmatically. Further investigation is needed to find out what exactly goes wrong here.

[John Gregg]

Did some investigating: the problem will occur whenever selectedIndex is changed by script while the select element is in focus.  when the focus leaves, the onchange event fires.  this doesn't happen on IE or Firefox.  it's not limited to within an onchange: script in onfocus, onkeypress, or a timer will cause the same inconsistency.

test and patch on the way.

[John Gregg]

Created attachment 31025 [details]
patch & test

[Sam Weinig]

Comment on attachment 31025 [details]
patch & test

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 44476)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,34 @@
> [phone]  John Gregg  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fix for bug 23721: onchange events fire when a SELECT element has
> +        focus and the selectedIndex is updated by script in some way--either
> +        during another onchange, onkeypress, onfocus, or timer--and then
> +        focus is lost. 

It is good to add the URL for the bug and the title in the changelogs.

> +
> +        Fixed by making a separate method for user-driven selectedIndex 
> +        changes, leaving scripts to use one which doesn't cause onchange to
> +        be queued.
> +        
> +        Test: fast/forms/select-script-onchange.html
> +
> +        * dom/SelectElement.cpp: check if the pending change is user driven
> +        before calling onchange
> +        (WebCore::SelectElement::menuListOnChange):
> +        (WebCore::SelectElement::setSelectedIndex):
> +        * dom/SelectElement.h: store whether the pending change is user driven
> +        (WebCore::SelectElementData::userDrivenChange):
> +        (WebCore::SelectElementData::setUserDrivenChange):
> +        * html/HTMLSelectElement.cpp: split into two methods -- script version
> +        [non-user-driven] corresponds to IDL defined property name
> +        (WebCore::HTMLSelectElement::setSelectedIndex):
> +        (WebCore::HTMLSelectElement::setSelectedIndexByUser):
> +        * html/HTMLSelectElement.h: 
> +        * rendering/RenderMenuList.cpp: use ByUser method when coming through
> +        the renderer
> +        (WebCore::RenderMenuList::valueChanged):
> +
>  2009-06-05  Sam Weinig  <[e-mail]>
>  
>          Reviewed by Anders Carlsson.
> Index: WebCore/dom/SelectElement.cpp
> ===================================================================
> --- WebCore/dom/SelectElement.cpp	(revision 44474)
> +++ WebCore/dom/SelectElement.cpp	(working copy)
> @@ -198,8 +198,9 @@ void SelectElement::menuListOnChange(Sel
>      ASSERT(data.usesMenuList());
>  
>      int selected = selectedIndex(data, element);
> -    if (data.lastOnChangeIndex() != selected) {
> +    if (data.lastOnChangeIndex() != selected && data.userDrivenChange()) {
>          data.setLastOnChangeIndex(selected);
> +        data.setUserDrivenChange(false);
>          element->dispatchFormControlChangeEvent();
>      }
>  }
> @@ -309,7 +310,7 @@ int SelectElement::selectedIndex(const S
>      return -1;
>  }
>  
> -void SelectElement::setSelectedIndex(SelectElementData& data, Element* element, int optionIndex, bool deselect, bool fireOnChange)
> +void SelectElement::setSelectedIndex(SelectElementData& data, Element* element, int optionIndex, bool deselect, bool fireOnChangeNow, bool userDrivenChange)
>  {
>      const Vector<Element*>& items = data.listItems(element);
>      int listIndex = optionToListIndex(data, element, optionIndex);
> @@ -335,9 +336,12 @@ void SelectElement::setSelectedIndex(Sel
>  
>      scrollToSelection(data, element);
>  
> -    // This only gets called with fireOnChange for menu lists. 
> -    if (fireOnChange && data.usesMenuList())
> -        menuListOnChange(data, element);
> +    // This only gets called with fireOnChangeNow for menu lists. 
> +    if (data.usesMenuList()) {
> +        data.setUserDrivenChange(userDrivenChange);
> +        if (fireOnChangeNow)
> +            menuListOnChange(data, element);
> +    }
>  
>      if (Frame* frame = element->document()->frame())
>          frame->page()->chrome()->client()->formStateDidChange(element);
> Index: WebCore/dom/SelectElement.h
> ===================================================================
> --- WebCore/dom/SelectElement.h	(revision 44474)
> +++ WebCore/dom/SelectElement.h	(working copy)
> @@ -57,7 +57,8 @@ public:
>      virtual int optionToListIndex(int optionIndex) const = 0;
>  
>      virtual int selectedIndex() const = 0;
> -    virtual void setSelectedIndex(int index, bool deselect = true, bool fireOnChange = false) = 0;
> +    virtual void setSelectedIndex(int index, bool deselect = true) = 0;
> +    virtual void setSelectedIndexByUser(int index, bool deselect = true, bool fireOnChangeNow = false) = 0;
>  
>  protected:
>      virtual ~SelectElement() { }
> @@ -78,7 +79,7 @@ protected:
>      static void setRecalcListItems(SelectElementData&, Element*);
>      static void recalcListItems(SelectElementData&, const Element*, bool updateSelectedStates = true);
>      static int selectedIndex(const SelectElementData&, const Element*);
> -    static void setSelectedIndex(SelectElementData&, Element*, int optionIndex, bool deselect = true, bool fireOnChange = false);
> +    static void setSelectedIndex(SelectElementData&, Element*, int optionIndex, bool deselect = true, bool fireOnChangeNow = false, bool userDrivenChange = true);
>      static int optionToListIndex(const SelectElementData&, const Element*, int optionIndex);
>      static int listToOptionIndex(const SelectElementData&, const Element*, int listIndex);
>      static void dispatchFocusEvent(SelectElementData&, Element*);
> @@ -117,6 +118,9 @@ public:
>      int lastOnChangeIndex() const { return m_lastOnChangeIndex; }
>      void setLastOnChangeIndex(int value) { m_lastOnChangeIndex = value; }
>  
> +    bool userDrivenChange() const { return m_userDrivenChange; }
> +    void setUserDrivenChange(bool value) { m_userDrivenChange = value; }
> +
>      Vector<bool>& lastOnChangeSelection() { return m_lastOnChangeSelection; }
>  
>      bool activeSelectionState() const { return m_activeSelectionState; }
> @@ -154,6 +158,7 @@ private:
>  
>      int m_lastOnChangeIndex;
>      Vector<bool> m_lastOnChangeSelection;
> +    bool m_userDrivenChange;
>  
>      bool m_activeSelectionState;
>      int m_activeSelectionAnchorIndex;
> Index: WebCore/html/HTMLSelectElement.cpp
> ===================================================================
> --- WebCore/html/HTMLSelectElement.cpp	(revision 44474)
> +++ WebCore/html/HTMLSelectElement.cpp	(working copy)
> @@ -80,9 +80,14 @@ void HTMLSelectElement::deselectItems(HT
>      SelectElement::deselectItems(m_data, this, excludeElement);
>  }
>  
> -void HTMLSelectElement::setSelectedIndex(int optionIndex, bool deselect, bool fireOnChange)
> +void HTMLSelectElement::setSelectedIndex(int optionIndex, bool deselect)
>  {
> -    SelectElement::setSelectedIndex(m_data, this, optionIndex, deselect, fireOnChange);
> +    SelectElement::setSelectedIndex(m_data, this, optionIndex, deselect, false, false);
> +}
> +
> +void HTMLSelectElement::setSelectedIndexByUser(int optionIndex, bool deselect, bool fireOnChangeNow)
> +{
> +    SelectElement::setSelectedIndex(m_data, this, optionIndex, deselect, fireOnChangeNow, true);
>  }
>  
>  int HTMLSelectElement::activeSelectionStartListIndex() const
> Index: WebCore/html/HTMLSelectElement.h
> ===================================================================
> --- WebCore/html/HTMLSelectElement.h	(revision 44474)
> +++ WebCore/html/HTMLSelectElement.h	(working copy)
> @@ -39,7 +39,8 @@ public:
>      HTMLSelectElement(const QualifiedName&, Document*, HTMLFormElement* = 0);
>  
>      virtual int selectedIndex() const;
> -    virtual void setSelectedIndex(int index, bool deselect = true, bool fireOnChange = false);
> +    virtual void setSelectedIndex(int index, bool deselect = true);
> +    virtual void setSelectedIndexByUser(int index, bool deselect = true, bool fireOnChangeNow = false);
>  
>      unsigned length() const;
>  
> Index: WebCore/rendering/RenderMenuList.cpp
> ===================================================================
> --- WebCore/rendering/RenderMenuList.cpp	(revision 44474)
> +++ WebCore/rendering/RenderMenuList.cpp	(working copy)
> @@ -297,7 +297,7 @@ void RenderMenuList::hidePopup()
>  void RenderMenuList::valueChanged(unsigned listIndex, bool fireOnChange)
>  {
>      SelectElement* select = toSelectElement(static_cast<Element*>(node()));
> -    select->setSelectedIndex(select->listToOptionIndex(listIndex), true, fireOnChange);
> +    select->setSelectedIndexByUser(select->listToOptionIndex(listIndex), true, fireOnChange);
>  }
>  
>  String RenderMenuList::itemText(unsigned listIndex) const
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 44476)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  John Gregg  <[e-mail]>
> +
> +        Check for unnecessary calls to onchange in response to script
> +        actions.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=23721
> +
> +        * fast/forms/select-script-onchange-expected.txt: Added.
> +        * fast/forms/select-script-onchange.html: Added.
> +
>  2009-06-01  Ben Murdoch  <[e-mail]>
>  
>          <https://bugs.webkit.org/show_bug.cgi?id=25710> HTML5 Database stops executing transactions if the URL hash changes while a transaction is open and an XHR is in progress.
> Index: LayoutTests/fast/forms/select-script-onchange-expected.txt
> ===================================================================
> --- LayoutTests/fast/forms/select-script-onchange-expected.txt	(revision 0)
> +++ LayoutTests/fast/forms/select-script-onchange-expected.txt	(revision 0)
> @@ -0,0 +1,7 @@
> +Test for http://bugs.webkit.org/show_bug.cgi?id=23721 Changing dropdown's selectedIndex within onchange handler fires another onchange.
> +
> +SUCCESS
> +
> +This select changes on focus: should not fire onchange.  
> +This select changes on change: should only fire onchange once.  
> +This select is changed by a timeout in the test script. It should not fire onchange.  
> Index: LayoutTests/fast/forms/select-script-onchange.html
> ===================================================================
> --- LayoutTests/fast/forms/select-script-onchange.html	(revision 0)
> +++ LayoutTests/fast/forms/select-script-onchange.html	(revision 0)
> @@ -0,0 +1,86 @@
> +<html>
> +    <head>
> +        <title></title>
> +        <script type="text/javascript">
> +            var changeCount = new Array(4);
> +            changeCount[1] = changeCount[2] = changeCount[3] = 0;
> +
> +            function test()
> +            {
> +                if (!window.eventSender)
> +                    return;
> +
> +                layoutTestController.dumpAsText();
> +
> +                var popup = document.getElementById("switcher1");
> +                popup.focus();
> +
> +                popup = document.getElementById("switcher2");
> +                popup.focus();
> +
> +                eventSender.keyDown("t", null);
> +                eventSender.keyDown("\r", null);
> +
> +                var popup = document.getElementById("switcher3");
> +                popup.focus();
> +
> +                check();
> +           }
> +
> +           function check() {
> +                setTimeout("document.getElementById('switcher3').selectedIndex = 1;", 0);
> +
> +                var popup = document.getElementById("switcher2");
> +                popup.focus();
> +
> +                var result = document.getElementById("result");
> +                result.innerHTML = "";
> +                if (changeCount[1] != 0) {
> +                    result.innerHTML += "<br/>FAILURE: onchange(1) called " + changeCount[1] + " times.";
> +                } 
> +                if (changeCount[2] != 1) {
> +                    result.innerHTML += "<br/>FAILURE: onchange(2) called " + changeCount[2] + " times.";
> +                } 
> +                if (changeCount[3] != 0) {
> +                    result.innerHTML += "<br/>FAILURE: onchange(3) called " + changeCount[3] + " times.";
> +                } 
> +                if (result.innerHTML == "") result.innerHTML = "SUCCESS";
> +
> +            }
> +
> +            function changed(select)
> +            {
> +                changeCount[select]++;
> +            }
> +        </script>
> +    </head>
> +    <body onload="test()">
> +        <p>
> +            Test for <i><a href="http://bugs.webkit.org/show_bug.cgi?id=23721">http://bugs.webkit.org/show_bug.cgi?id=23721</a>
> +            Changing dropdown's selectedIndex within onchange handler fires another onchange</i>.
> +        </p>
> +        <p id="result">
> +          To test interactively: focus on the first select (don't change it).<br/>
> +          Change the second select to "two"<br/>
> +          Focus on the third, then click <a href="#" onclick="check(); return false;">here</a>.  
> +        </p>
> +        This select changes on focus: should not fire onchange.
> +        <select name="switcher1" id="switcher1" onfocus="this.selectedIndex = 1;" onchange="changed(1)">
> +            <option value="one">One</option>
> +            <option value="two">Two</option>
> +        </select>
> +        <hr/>
> +        This select changes on change: should only fire onchange once.
> +        <select name="switcher2" id="switcher2" onchange="changed(2); if (this.selectedIndex == 1) this.selectedIndex = 2;">
> +            <option value="one">One</option>
> +            <option value="two">Two</option>
> +            <option value="three">Three</option>
> +        </select>
> +        <hr/>
> +        This select is changed by a timeout in the test script.  It should not fire onchange.
> +        <select name="switcher3" id="switcher3" onchange="changed(3)">
> +            <option value="one">One</option>
> +            <option value="two">Two</option>
> +        </select>        
> +    </body>
> +</html>

[David Levin]

Assign to levin for landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/45185.

[Alexey Proskuryakov]

*** Bug 18748 has been marked as a duplicate of this bug. ***