Bug 23721 - Changing dropdown's selectedIndex within onchange handler fires another onchange
Summary: Changing dropdown's selectedIndex within onchange handler fires another onchange
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL: http://chatbotgame.com/?cmd=chat_s
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2009-02-03 17:27 PST by jasneet
Modified: 2009-06-25 13:43 PDT (History)
3 users (show)

See Also:


Attachments
testcase (999 bytes, text/html)
2009-02-03 17:27 PST, jasneet
no flags Details
patch & test (12.76 KB, patch)
2009-06-05 19:38 PDT, John Gregg
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2009-02-03 17:27:16 PST
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
Comment 1 jasneet 2009-02-03 17:27:33 PST
Created attachment 27303 [details]
testcase
Comment 2 Alexey Proskuryakov 2009-03-31 00:17:12 PDT
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.
Comment 3 John Gregg 2009-06-05 19:31:52 PDT
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.
Comment 4 John Gregg 2009-06-05 19:38:47 PDT
Created attachment 31025 [details]
patch & test
Comment 5 Sam Weinig 2009-06-23 22:39:40 PDT
Comment on attachment 31025 [details]
patch & test

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 44476)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,34 @@
> +2009-06-05  John Gregg  <johnnyg@google.com>
> +
> +        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  <sam@webkit.org>
>  
>          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 @@
> +2009-06-05  John Gregg  <johnnyg@google.com>
> +
> +        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  <benm@google.com>
>  
>          <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>
Comment 6 David Levin 2009-06-25 10:28:22 PDT
Assign to levin for landing.
Comment 7 David Levin 2009-06-25 13:43:16 PDT
Committed as http://trac.webkit.org/changeset/45185.