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

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Events
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Anand Ratn, Assigned: jessica)

Tracking

45 Branch
mozilla49
Other
Windows 7
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

a year ago
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
(Reporter)

Updated

a year ago
OS: Unspecified → Windows 7
Hardware: Unspecified → Other
Component: Untriaged → DOM: Events
Product: Firefox → Core

Comment 1

a year ago
Could you provide a testcase, please? (attached to the bug report or at http://codepen.io/ e.g.)
Flags: needinfo?(anand.ratn07)
(Reporter)

Comment 2

a year ago
https://jsfiddle.net/0yq8xj2s/
Flags: needinfo?(anand.ratn07)
(Reporter)

Comment 3

a year ago
(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/
Olli, can you please take a look?
Flags: needinfo?(bugs)
Whiteboard: btpp-followup-2016-04-28
Is this a regression?
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.
Flags: needinfo?(bugs) → needinfo?(jjong)
Created attachment 8744065 [details]
a test
Note, there is an e10s regression currently. https://bugzilla.mozilla.org/show_bug.cgi?id=1266575
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.)
(Assignee)

Comment 10

a year ago
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
Assignee: nobody → jjong
Flags: needinfo?(jjong)
(Assignee)

Comment 11

a year ago
(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
Whiteboard: btpp-followup-2016-04-28 → btpp-active
(Assignee)

Comment 12

a year ago
Created attachment 8744210 [details] [diff] [review]
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...
Attachment #8744210 - Flags: feedback?(bugs)
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.
oh, you're changing nsListControlFrame::KeyDown. I should have looked at the patch first :)
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.
Attachment #8744210 - Flags: feedback?(bugs) → feedback+
(Assignee)

Comment 16

a year ago
(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
(Assignee)

Comment 17

a year ago
Created attachment 8746452 [details] [diff] [review]
patch, v2.

added mochitest.
Attachment #8744210 - Attachment is obsolete: true
Attachment #8746452 - Flags: review?(bugs)
(Assignee)

Comment 18

a year ago
Created attachment 8746453 [details] [diff] [review]
patch, v2.

sorry, fixed indentation.
Attachment #8746452 - Attachment is obsolete: true
Attachment #8746452 - Flags: review?(bugs)
Attachment #8746453 - Flags: review?(bugs)
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.
Attachment #8746453 - Flags: review?(bugs) → review-
e10s is different because there the UI for the dropdown lives in the parent process, yet <select> lives in child process.
(Assignee)

Comment 21

a year ago
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?
Flags: needinfo?(bugs)
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.)
Flags: needinfo?(bugs)
(Assignee)

Comment 23

a year ago
(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.)
(Assignee)

Comment 24

a year ago
Created attachment 8748977 [details] [diff] [review]
patch, v3.
Attachment #8746453 - Attachment is obsolete: true
(Assignee)

Comment 25

a year ago
Created attachment 8749114 [details] [diff] [review]
patch, v4.
Attachment #8748977 - Attachment is obsolete: true
(Assignee)

Comment 26

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

Still figuring out...
(Assignee)

Comment 27

a year ago
Created attachment 8749545 [details] [diff] [review]
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.
Attachment #8749114 - Attachment is obsolete: true
Attachment #8749545 - Flags: review?(bugs)
(Assignee)

Comment 28

a year ago
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.
Indeed. Could you file a new bug about that e10s issue (non-e10s is right there).
(Assignee)

Comment 30

a year ago
(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.

Updated

a year ago
Attachment #8749545 - Flags: review?(bugs) → review+
(Assignee)

Comment 31

a year ago
try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5071cc6579e7
Keywords: checkin-needed

Comment 32

a year ago
bugherderlanding
https://hg.mozilla.org/integration/mozilla-inbound/rev/38e0933499cb
Keywords: checkin-needed

Comment 33

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/38e0933499cb
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.