903715 - Using keyboard to select value in <select> with enter key ends up submitting form when it didn't use to

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Byron, I finally hunted this down and it's a change in the browser, not bugzilla; the cache bits were just red herrings.

BUILD: Any build since bug 501496 was checked in.

STEPS TO REPRODUCE:

1)  Load this bug.
2)  Click on the "Product" dropdown
3)  Type "F" so that "Finance" is selected
4)  Hit enter

EXPECTED RESULTS: Select dropdown closes, "Finance" is still selected there, focus is transferred to the assignee field which has its contents selected.

ACTUAL RESULTS: As expected results, but then the form submits and I'm asked to pick a correct component.

At a guess, the enter press ends up somehow being also processed on the text input, which triggers a form submission...

Be careful to test this with the Product <select>.  If you do it with the Component one, you'll end up actually switching components a lot.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and I narrowed the regression down to http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=12c11406ed75&tochange=949a5e125a8d using tinderbox builds....

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Hmm, we don't call preventDefault()...
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#2139

Comment 3

[Mats Palmgren (inactive)]

Judging by the code for NS_VK_ESCAPE, do we need a preventDefault()
before returning on line 2135 too?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to Mats Palmgren (:mats) from comment #3)
> Judging by the code for NS_VK_ESCAPE, do we need a preventDefault()
> before returning on line 2135 too?

Yeah, I think so.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

I still don't understand well this bug.

Not calling preventDefault() is not a new behavior. Why does the older build prevent form submission? I'm looking for the Enter key handler for form submission...

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Ah, I understand. The focus is moved to <input> by handling of closing dropdown at *keydown*. So, the following keypress event is now fired on <input> which causes the form submission.

Comment 7

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

How do other browsers behave?

Comment 8

[Boris Zbarsky [:bzbarsky]]

WebKit, Blink, Presto do not submit the form in this case when I close the dropdown via enter.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: testcase

It's odd... This test case causes firing submit event on all browsers...

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to Vacation until Aug 19.  Do not ask for review. from comment #8)
> WebKit, Blink, Presto do not submit the form in this case when I close the
> dropdown via enter.

No, IE 11 and Google Chrome (for Win) submit the form! Presto doesn't so, though.

I think this is a bug of bugzilla if it's not intentional behavior.

bugzilla calls preventDefault of change event. However, it's not cancellable. I have no idea for smart fix of this bug even on bugzilla side.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: testcase

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Okay, I confirmed that as bz mentioned, Google Chrome for Mac and Safari don't submit the form because they prevent keypress event on Mac.

So, Blink behaves differently on Windows and Mac.

There are two issues:

1. Should we prevent to fire keypress event when Enter key pressed when dropdown is opened?
2. Anyway, bugzilla needs to change the code if it's not intentional behavior.

For compatibility with IE, I prefer to keep current behavior. And I hate to change our behavior only on Mac.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

I filed bug 903925 for #2 of comment 12 (this bug should be fixed by bugzilla side, anyway).

This bug should be used for working on #1 of comment 12.

Smaug, how do you think what's the best behavior of Gecko?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

I think deeper for this.

Now, I think that we should call preventDefault() at Enter keydown handler of <select> because:

* It's more similar to the old *our* behavior, so, its risk is less than keeping current behavior for us.
* Closing a dropdown with Enter key is really targeted to the <select> element, not for the new <input> element. So, working <input> element is really odd behavior for all users.
* Closing a dropdown with Enter key is a reaction for the key operation. So, most users don't expect additional behavior to both browser and website.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Consume Enter key event if dropdown is closed by it

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=c069028ecda3

Comment 16

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/13-8/18 JST) from comment #14)
> I think deeper for this.
> 
> Now, I think that we should call preventDefault() at Enter keydown handler
> of <select> because:
I agree.
(Even better might be multipleActionsPrevented, if it works in this case, since
that doesn't change event's state reflected to JS side.)


> 
> * It's more similar to the old *our* behavior, so, its risk is less than
> keeping current behavior for us.
> * Closing a dropdown with Enter key is really targeted to the <select>
> element, not for the new <input> element. So, working <input> element is
> really odd behavior for all users.
> * Closing a dropdown with Enter key is a reaction for the key operation. So,
> most users don't expect additional behavior to both browser and website.
agree

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 788870 [details] [diff] [review]
Consume Enter key event if dropdown is closed by it

Okay, let's go.

Comment 18

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

Comment on attachment 788870 [details] [diff] [review]
Consume Enter key event if dropdown is closed by it

>+          // At closing dropdown, users must not expect there is additional
>+          // behavior for this key event.  Therefore, let's consume the event.

perhaps s/must not/may not/
Same also elsewhere

>+function runTests()
>+{
>+  var form = document.getElementById("form");
>+  form.addEventListener("keypress", function (aEvent) {
>+    ok(false, "keypress event shouldn't be fired when the precede keydown event caused closing dropdown of select element");
s/precede/preceding/ and 'the dropdown of the select' I think. 


>+  }, true);
>+  form.addEventListener("submit", function (aEvent) {
>+    ok(false, "submit shouldn't be performed by enter key press on the select element");
the enter key

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Thank you!
https://hg.mozilla.org/integration/mozilla-inbound/rev/64324e19ca10

Comment 20

[Ed Morley [:emorley]]

Backed out for failures in test_bug903715.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26435740&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26436444&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b542735c1ce3

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Ah... I need to disable the new test on Android. But I cannot do it at least tomorrow. If I got a chance to touch it on my vacation, I'd do it.

Comment 22

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

Masayuki, if you don't have time to do that, please ask feedback from me or something so that I could
disable it (later this week).

Comment 23

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

(I need the feedback or need-info to remind myself.)

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Thanks, but I landed new patch which disables the new test on Android.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aebcc13238f5

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/aebcc13238f5

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/13-8/18 JST) from comment #24)
> Thanks, but I landed new patch which disables the new test on Android.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/aebcc13238f5

Strictly speaking, that needed review from a build peer.

Loosely speaking, the test should have been disabled in the android.json file, and should have had a bug filed and annotated.

Comment 27

[Alex Keybl [:akeybl]]

This will have unknown web impact, and risk will only get larger the later we uplift. Let's get this into Aurora 25.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Get rid of illegal change in Makefile.in

(In reply to :Ms2ger from comment #26)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/13-8/18
> JST) from comment #24)
> > Thanks, but I landed new patch which disables the new test on Android.
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/aebcc13238f5
> 
> Strictly speaking, that needed review from a build peer.
> 
> Loosely speaking, the test should have been disabled in the android.json
> file, and should have had a bug filed and annotated.

Thank you for the information. We should not uplift the patch until fixing the illegal change.

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 792080 [details] [diff] [review]
Get rid of illegal change in Makefile.in

Review of attachment 792080 [details] [diff] [review]:
-----------------------------------------------------------------

Derp. I meant to type a comment. I'm actually not familiar with those .json files. Over to someone on A*Team for r+ on those bits.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Thank you, Gregory and Andrew!

https://hg.mozilla.org/integration/mozilla-inbound/rev/f37ea733c94c

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f37ea733c94c

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Can I take the patches to Aurora only with tracking-firefox25+? Or do I need to request approval for each patch?

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 788870 [details] [diff] [review]
Consume Enter key event if dropdown is closed by it

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 501496
User impact if declined: user might submit form when closing <select> dropdown with Enter key and the web page moves focus to <input> element automatically then.
Testing completed (on m-c, etc.): on m-c, this has been landed, and has automated test.
Risk to taking this patch (and alternatives if risky): This is not so risky because the patched behavior is similar to our old behavior.
String or IDL/UUID changes made by this patch: none

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 792080 [details] [diff] [review]
Get rid of illegal change in Makefile.in

[Approval Request Comment]
Bug caused by (feature/regressing bug #): fixes a bug of the previous patch.
User impact if declined: none
Testing completed (on m-c, etc.): already landed on m-c. 
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Requesting approval for each patch due to no reply from akeybl. And the check-in rules don't mention about the relation between tracking flag and check-in.
https://wiki.mozilla.org/Tree_Rules#mozilla-aurora

Comment 36

[Mats Palmgren (inactive)]

Masayuki-san, patches needs to have explicit approval for branches (although if
you have multiple patches that should land together you can request approval on
just one and say "the approval request is for all the patches" to be clear).

The wiki page says "Patches must have the approval-mozilla-aurora+ flag in
Bugzilla. To request approval, set the approval-mozilla-aurora? flag on the
patch you wish to check in."  (and ditto for -beta).

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to Mats Palmgren (:mats) from comment #36)

Thank you for the information! I understood.

So, tracking flag is just used for managing the bug? IIRC, each patch didn't need approval if the bug blocks the release (at least a couple of years ago).

Comment 38

[Mats Palmgren (inactive)]

> So, tracking flag is just used for managing the bug?

Yes.  The B2G project may still have blocking-* flags that implies approval to
land, but I'm not sure.  I just ask for "approval‑mozilla‑b2g18" anyway, it's simpler.

> IIRC, each patch didn't need approval if the bug blocks the release (at least a couple of years ago)

I think you're right, but that was before "rapid release" probably.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a9878e0ae7b