1265968 - jquery change event not working when element change in dropdown [45.0.2]

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Anand Ratn]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.112 Safari/537.36

Steps to reproduce:

1) create a dropdown list
<select id="myid">
  <option value="volvo">Volvo</option>
  <option value="saab">Saab</option>
  <option value="opel">Opel</option>
  <option value="audi">Audi</option>
</select>

2: in javascript I added change event,
  $(document).ready(function(){
    $(function() {
      $("#myid").change(updateValue);
    });
    function updateValue() {
      var output = $('#myid option:selected').text();
      console.log(output);
    }
  });
3) when I try to select element with mouse then it works fine. If i use up arrow key or down arrow key to change the selection then "change event" is not getting triggerd. Ideally element is getting changed in dropdown. Event will get triggered only when "Enter" key is pressed after selection. It works fine with other browser.  


Actual results:

change event will not trigger even when the element is changed in dropdown list


Expected results:

change event should trigger even when the element is changed in dropdown list

Comment 1

[Loic]

Could you provide a testcase, please? (attached to the bug report or at http://codepen.io/ e.g.)

Comment 2

[Anand Ratn]

https://jsfiddle.net/0yq8xj2s/

Comment 3

[Anand Ratn]

(In reply to Loic from comment #1)
> Could you provide a testcase, please? (attached to the bug report or at
> http://codepen.io/ e.g.)

https://jsfiddle.net/0yq8xj2s/

Comment 4

[Andrew Overholt [:overholt]]

Olli, can you please take a look?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Is this a regression?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I guess no. 

The spec allows quite some freedom here:
"If the multiple attribute is absent, and the element is not disabled, then the user agent should allow the user to pick an option element in its list of options that is itself not disabled. Upon this option element being picked (either through a click, or through unfocusing the element after changing its value, or through a menu command, or through any other mechanism), and before the relevant user interaction event is queued (e.g. before the click event), the user agent must set the selectedness of the picked option element to true, set its dirtiness to true, and then send select update notifications."
The spec text doesn't quite make sense since if we're selecting based on click event, the click event sure has been queued already. But the basic idea is there...
https://html.spec.whatwg.org/multipage/forms.html#the-select-element

But I think following other browsers make sense.



Jessica, since you're looking at input and change events in that other bug, perhaps you'd be willing to look at this too.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a test

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Note, there is an e10s regression currently. https://bugzilla.mozilla.org/show_bug.cgi?id=1266575

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

And filed a spec bug to clarify this a bit.
https://github.com/whatwg/html/issues/1092

Anyhow, even though our current behavior is acceptable per the spec, changing to follow what other browsers do would be fine too. So fire change event when using keyboard to change the value.


(Firing input event on <select> was added to the spec at some point and we could have a separate bug for that.)

Comment 10

[Jessica Jong [:jessica] (inactive)]

Sure, I can take a look at this.

Actually, we were aware of this, there is a comment [1] saying:
  // XXX This is strange. On other browsers, "change" event is fired
  //     immediately after the selected item is changed rather than
  //     Enter key is pressed.

I can change this to follow what other browsers do.


[1] http://hg.mozilla.org/mozilla-central/file/b00232b9f65b/layout/forms/nsListControlFrame.cpp#l2166

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #9)
> And filed a spec bug to clarify this a bit.
> https://github.com/whatwg/html/issues/1092
> 
> Anyhow, even though our current behavior is acceptable per the spec,
> changing to follow what other browsers do would be fine too. So fire change
> event when using keyboard to change the value.
> 
> 
> (Firing input event on <select> was added to the spec at some point and we
> could have a separate bug for that.)

We do have a bug for that: Bug 1024350

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

This patch does the trick.

If I use the mouse to drop down the list, then use the keyboard up/down and then Enter" key to select an item, "change" event is fired (which makes sense), but I can't find the place where the event is triggered...

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

nsListControlFrame::KeyDown seems to explicitly handle return key.
In form controls some of the event handling is in DOM side, Pre/PostHandleEvent, and then stuff possibly more depending on the UI is in layout. Though, in certain cases this difference is largely because of some random historical reasons. In case of <select> where the popup is shown. E10s complicates this some more, since the popup lives in parent process and the actual element in child process.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

oh, you're changing nsListControlFrame::KeyDown. I should have looked at the patch first :)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8744210 [details] [diff] [review]
patch, v1.

Some tests would be great. wpt tests may not be able to open <select> but mochitests should be able to if click event is dispatched using the methods EventUtils.js has.

Comment 16

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8744210 [details] [diff] [review]
> patch, v1.
> 
> Some tests would be great. wpt tests may not be able to open <select> but
> mochitests should be able to if click event is dispatched using the methods
> EventUtils.js has.

Thanks, I'll add mochitests to verify this.

> If I use the mouse to drop down the list, then use the keyboard up/down and then Enter" key to 
> select an item, "change" event is fired (which makes sense), but I can't find the place where the 
> event is triggered...

Answering to myself, the event was triggered in SelectContentHelper [1] when drop down is dismissed.

[1] http://hg.mozilla.org/mozilla-central/file/4b16c1609af5/toolkit/modules/SelectContentHelper.jsm#l82

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

added mochitest.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

sorry, fixed indentation.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8746453 [details] [diff] [review]
patch, v2.

So looks like this doesn't fix 'change' handling in e10s.
If you try the test in this bug in a build which has this patch and enable e10s, opening the dropdown and using keyboard doesn't seem to dispatch 'change'.
To fix also e10s case, one need to probably modify SelectContentHelper.jsm and related files.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

e10s is different because there the UI for the dropdown lives in the parent process, yet <select> lives in child process.

Comment 21

[Jessica Jong [:jessica] (inactive)]

I tested this patch on nightly (which has e10s enabled by default) and I think it works as expected:

- focus on the dropdown list, without opening the dropdown, use keyboard up/down key to select an item _does_ fire change event.
- open the dropdown list using mouse, then using the keyboard to move through items _does not_ fire change event, but selecting one using "return" key or mouse, _does_ fire change event.

Chrome browser has the same behavior.
Do you mean we should fire change events while moving through the items when dropdown list is open?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

but the behavior is different in e10s vs. non-e10s. You can try that by opening a new non-e10s window from the File menu.
But you're right, the patch does give the same behavior on e10s what Chrome has. Could we get that behavior on non-e10s too then?

(and sorry, I thought we want to get 'change' when the popup is open based on the bug report, but if other browsers don't do that, no need for that.)

Comment 23

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #22)
> but the behavior is different in e10s vs. non-e10s. You can try that by
> opening a new non-e10s window from the File menu.
> But you're right, the patch does give the same behavior on e10s what Chrome
> has. Could we get that behavior on non-e10s too then?

You're right, it behaves differently on e10s and non-e10s. So, with the proposed patch, e10s works as expected, I'll see what to do on non-e10s.

> 
> (and sorry, I thought we want to get 'change' when the popup is open based
> on the bug report, but if other browsers don't do that, no need for that.)

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v4.

Comment 26

[Jessica Jong [:jessica] (inactive)]

The added mochitest fails on Android 4.3:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0d7d4a7099d&selectedJob=20393605

Still figuring out...

Comment 27

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v5.

This patch aligns the change event on e10s and non-e10s:
- While dropdown list is open, no change event is fired until users selects it using mouse click or Enter key.
- If dropdown list is closed, selecting items using up/down key fires change event.

I couldn't reproduce the failure locally on Android, so I have disabled it for now.

Comment 28

[Jessica Jong [:jessica] (inactive)]

I did notice something different between e10s and non-e10s: when dropdown list is open, moving through the items using up/down key, in e10s, the combobox display text will not be updated, but in non-e10s, the combobox display text is updated. So if user dismiss the dropdown list using Escape key or by clicking somewhere else, the final displayed text on e10s and non-e10s are different.

On chrome browser, when dropdown list is open, the display text is updated when moving through items using up/down key, but it is not updated when moving through items using mouse.

I think we should have the same behavior on e10s and non-e10s, but this of out scope, we can do it in another bug.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Indeed. Could you file a new bug about that e10s issue (non-e10s is right there).

Comment 30

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #29)
> Indeed. Could you file a new bug about that e10s issue (non-e10s is right
> there).

Sure, filed bug 1271532.

Comment 31

[Jessica Jong [:jessica] (inactive)]

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5071cc6579e7

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38e0933499cb

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38e0933499cb