How to reproduce: 1. Open data:text/html,<script>c=0;</script><select autofocus onchange="c++;document.getElementById('console').innerHTML='onchange: '+c"><option>abc<option>def</select><div id=console></div> 2. Type 'd'. The option 'def' is selected. Expected: 3. 'onchange: 1' message is shown. Actual: 3. Nothing happens. This is very similar to Bug 74384.
Put HTML+SCRIPT into jsfiddle.net. See URL field.
Created attachment 119551 [details] Patch 1
Comment on attachment 119551 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=119551&action=review The code change looks ok. > LayoutTests/fast/forms/select/menulist-type-ahead-find-expected.txt:1 > +WebKit Bug 74590 We should show what is tested. The bug number is not useful. > LayoutTests/fast/forms/select/menulist-type-ahead-find.html:16 > +function log(msg) > +{ > + var res = document.getElementById("log"); > + res.innerHTML = res.innerHTML + msg + "<br>"; > +} > + > +function expectEq(expected, got) > +{ > + if (got == expected) > + log("PASSED got=[" + got + "]"); > + else > + log("FAILED: expected[" + expected + "] got=[" + got + "]"); > +} You don't need to make these function if you use ../../js/resources/js-test-pre.js. or, they can be removed if we have <div id="res">FAIL</div> initially and recordIt() changes the content to 'PASS'. > LayoutTests/fast/forms/select/menulist-type-ahead-find.html:21 > +function recordIt(ctrl) > +{ > + document.getElementById("res").innerText = ctrl.value; > +} I think recording the control value is not necessary. .innerText = 'PASS' is enough. > LayoutTests/fast/forms/select/menulist-type-ahead-find.html:37 > + if (!window.layoutTestController) > + return; It would be nice if we show a manual test instruction. This test shows 'Type "c" To see "cherry"' for now. We had better explain the expected result too.
Created attachment 119552 [details] Patch 2
Comment on attachment 119552 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=119552&action=review > LayoutTests/fast/forms/select/menulist-type-ahead-find.html:30 > + expectEq("apple", sel.value); > + eventSender.keyDown(ch); > + expectEq(expected, sel.value); I think these expectEq() are unnecessary. The type-ahead feature is tested in other test files. > LayoutTests/fast/forms/select/menulist-type-ahead-find.html:31 > + expectEq("PASS", res.innerText); This is not needed. The test result contains 'PASS' in "res" element. > LayoutTests/fast/forms/select/menulist-type-ahead-find.html:45 > +<h1>WebKit Bug <a href="https://bugs.webkit.org/show_bug.cgi?id=74590">74590</a></h1> Please show the purpose of the test. The bug number is not useful.
Created attachment 119560 [details] Patch 3
Comment on attachment 119560 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=119560&action=review > LayoutTests/fast/forms/select/menulist-type-ahead-find.html:29 > + expectEq("PASS", res.innerText); I don't think this is needed. 'PASS' for this is already shown as a dump of <div id="res">
Created attachment 119564 [details] Patch 4
Comment on attachment 119564 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=119564&action=review > LayoutTests/fast/forms/select/menulist-type-ahead-find.html:13 > + const sel = document.getElementById("sel"); > + const res = document.getElementById("res"); > + sel.focus(); Variable "res" is not needed. You might want to remove "sel" too.
Created attachment 119566 [details] Patch 5
Comment on attachment 119566 [details] Patch 5 ok
Comment on attachment 119566 [details] Patch 5 Attachment 119566 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10903567 New failing tests: fast/events/onchange-select-popup.html
Created attachment 119570 [details] Patch 6 - Update failed test
Comment on attachment 119570 [details] Patch 6 - Update failed test ok
Comment on attachment 119570 [details] Patch 6 - Update failed test Clearing flags on attachment: 119570 Committed r103045: <http://trac.webkit.org/changeset/103045>
All reviewed patches have been landed. Closing bug.
It made fast/forms/select-script-onchange.html fail on Qt - https://bugs.webkit.org/show_bug.cgi?id=74700
Committed r103050: <http://trac.webkit.org/changeset/103050>
Reopen, because it was rolled out.
Rolling out at http://trac.webkit.org/changeset/103050 due to a test failure on Chromium Windows: - fast/forms/select-script-onchange.html Result: ---- Test for http://bugs.webkit.org/show_bug.cgi?id=23721 Changing dropdown's selectedIndex within onchange handler fires another onchange. 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. This select is changed by a timeout in the test script. It should not fire onchange. ----
Hi Adam, I would like to remove onchange event for Enter key for Windows because of this change makes type ahead fires onchange event and fix for BUG-74384 fires onchange for Up/Down key. So, hitting Enter key fires additional onchange event. It seems Enter key behavior is introduced by http://trac.webkit.org/changeset/16977 Is it OK to remove this Enter key behavior? Thanks. -yosi Here is code for firing onchange: HTMLSelectElement::menuListDefaultEventHandler #if SPACE_OR_RETURN_POP_MENU ... GTK #elif ARROW_KEYS_POP_MENU ... Mac #else int listIndex = optionToListIndex(selectedIndex()); if (keyCode == '\r') { // listIndex should already be selected, but this will fire the onchange handler. selectOption(listToOptionIndex(listIndex), DeselectOtherOptions | DispatchChangeEvent | UserDriven); handled = true; } #endif
Why is this a desirable change to make? (I'm not saying it's undesirable, just that no evidence is shown in favor of the change in this bug or the attached patches.) For instance, if the new behavior matches other browsers, that would be one reason it is a desirable change. This information should go in both the bug and your ChangeLog entry.
(In reply to comment #22) > Why is this a desirable change to make? (I'm not saying it's undesirable, just that no evidence is shown in favor of the change in this bug or the attached patches.) For instance, if the new behavior matches other browsers, that would be one reason it is a desirable change. This information should go in both the bug and your ChangeLog entry. We have some user-reported bugs. e.g. http://code.google.com/p/chromium/issues/detail?id=90447 Issue 90447: select onchange not working by cursor key selection Cursor key selection and type-ahead selection change HTMLSelectElement::value, but don't dispatch 'change' events. This behavior looks a bug for web developers, and IE and Opera fire 'change' events immediately in this case.
Created attachment 119973 [details] Patch 7
(In reply to comment #23) > (In reply to comment #22) > > Why is this a desirable change to make? (I'm not saying it's undesirable, just that no evidence is shown in favor of the change in this bug or the attached patches.) For instance, if the new behavior matches other browsers, that would be one reason it is a desirable change. This information should go in both the bug and your ChangeLog entry. > > We have some user-reported bugs. e.g. > http://code.google.com/p/chromium/issues/detail?id=90447 Issue 90447: select onchange not working by cursor key selection > > Cursor key selection and type-ahead selection change HTMLSelectElement::value, but don't dispatch 'change' events. This behavior looks a bug for web developers, and IE and Opera fire 'change' events immediately in this case. Thanks for the info. Knowing that IE, Opera, and Firefox (according to the Chromium bug) all have this behavior makes a strong case for WebKit having it, too!
Comment on attachment 119973 [details] Patch 7 View in context: https://bugs.webkit.org/attachment.cgi?id=119973&action=review > Source/WebCore/ChangeLog:12 > + and type ahead selection. So, onchange for Enter key is redundant. This behavior is > + compatible to IE. You should mention Firefox and Opera too, and version numbers if you know them. > LayoutTests/fast/events/onchange-select-popup-expected.txt:9 > blur event fired. > > +PASS: change event fired. This change isn't mentioned in your ChangeLog.
Created attachment 120123 [details] Patch 8
Comment on attachment 120123 [details] Patch 8 ok.
Comment on attachment 120123 [details] Patch 8 Clearing flags on attachment: 120123 Committed r103497: <http://trac.webkit.org/changeset/103497>