Bug 70173 – REGRESSION(r97533): fast/forms/select-script-onchange.html failed after

• Component: Forms
• Status: RESOLVED FIXED

[Tamas Czene]

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/forms/select-script-onchange-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/forms/select-script-onchange-actual.txt 
@@ -1,6 +1,8 @@
 Test for http://bugs.webkit.org/show_bug.cgi?id=23721 Changing dropdown's selectedIndex within onchange handler fires another onchange.
 
-SUCCESS
+
+FAILURE: onchange(1) called 1 times.
+FAILURE: onchange(2) called 2 times.
 
 This select changes on focus: should not fire onchange.  
 This select changes on change: should only fire onchange once.

[Csaba Osztrogonác]

It fails on SL and on Qt bot too.

[Ryosuke Niwa]

View in context: https://bugs.webkit.org/attachment.cgi?id=111075&action=review

> Source/WebCore/html/HTMLOptionElement.cpp:192
> -    HTMLSelectElement* select = ownerSelectElement();
> -    if (select)
> -        select->childrenChanged(changedByParser);
> +    if (HTMLSelectElement* select = ownerSelectElement())
> +        select->optionElementChildrenChanged();

Maybe this change?

[Darin Adler]

I’ll investigate now.

[Darin Adler]

Created attachment 111142 [details]
Patch

[WebKit Review Bot]

Comment on attachment 111142 [details]
Patch

Attachment 111142 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10075313

New failing tests:
fast/events/popup-when-select-change.html
fast/events/onchange-select-popup.html

[Darin Adler]

Created attachment 111145 [details]
Patch

[Darin Adler]

Comment on attachment 111145 [details]
Patch

OK, lets try a more targeted fix.

[Darin Adler]

Hmmpf, even this smaller change seems to break onchange-select-popup.html

[Darin Adler]

Created attachment 111146 [details]
Patch

[Darin Adler]

New patch; this one passes all the tests in fast/events.

[Adam Barth]

Comment on attachment 111146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111146&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:1388
> +            setSelectedIndex(listToOptionIndex(index), true, false, true);

IMHO, this is calling out for enum arguments.

[Ryosuke Niwa]

Comment on attachment 111146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111146&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:1009
> +            setSelectedIndex(listToOptionIndex(listIndex), true, false, true);

These boolean values are hurting my eyes :(

[Ryosuke Niwa]

(In reply to comment #11)
> (From update of attachment 111146 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111146&action=review
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:1388
> > +            setSelectedIndex(listToOptionIndex(index), true, false, true);
> 
> IMHO, this is calling out for enum arguments.

I think we can all agree to that.

[Darin Adler]

Yes, we all agree. Even my ChangeLog agrees! And the patch I am working on right now, too.

[WebKit Review Bot]

Comment on attachment 111146 [details]
Patch

Clearing flags on attachment: 111146

Committed r97565: <http://trac.webkit.org/changeset/97565>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.