Bug 74590 – [Forms] Selection change by type-ahead doesn't fire 'change' event

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

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

[Kent Tamura]

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

[Kent Tamura]

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

[Kent Tamura]

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

[Kent Tamura]

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

[Kent Tamura]

Comment on attachment 119566 [details]
Patch 5

ok

[WebKit Review Bot]

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

[Kent Tamura]

Comment on attachment 119570 [details]
Patch 6 - Update failed test

ok

[WebKit Review Bot]

Comment on attachment 119570 [details]
Patch 6 - Update failed test

Clearing flags on attachment: 119570

Committed r103045: <http://trac.webkit.org/changeset/103045>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It made fast/forms/select-script-onchange.html fail on Qt - https://bugs.webkit.org/show_bug.cgi?id=74700

[Hajime Morrita]

Committed r103050: <http://trac.webkit.org/changeset/103050>

[Csaba Osztrogonác]

Reopen, because it was rolled out.

[Hajime Morrita]

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

[Adam Roben (:aroben)]

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.

[Kent Tamura]

(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

[Adam Roben (:aroben)]

(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!

[Adam Roben (:aroben)]

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

[Kent Tamura]

Comment on attachment 120123 [details]
Patch 8

ok.

[WebKit Review Bot]

Comment on attachment 120123 [details]
Patch 8

Clearing flags on attachment: 120123

Committed r103497: <http://trac.webkit.org/changeset/103497>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.