[webkit-reviews] review granted: [Bug 23721] Changing dropdown's selectedIndex within onchange handler fires another onchange : [Attachment 31025] patch & test
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 23 22:39:40 PDT 2009
Sam Weinig <sam at webkit.org> has granted John Gregg <johnnyg at google.com>'s
request for review:
Bug 23721: Changing dropdown's selectedIndex within onchange handler fires
another onchange
https://bugs.webkit.org/show_bug.cgi?id=23721
Attachment 31025: patch & test
https://bugs.webkit.org/attachment.cgi?id=31025&action=review
------- Additional Comments from Sam Weinig <sam at webkit.org>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 44476)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,34 @@
> +2009-06-05 John Gregg <johnnyg at 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 at 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 at 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 at 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>
More information about the webkit-reviews
mailing list