Closed Bug 903715 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- unaffected
firefox25 + fixed
firefox26 + fixed

People

(Reporter: bzbarsky, Assigned: masayuki)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.
Judging by the code for NS_VK_ESCAPE, do we need a preventDefault()
before returning on line 2135 too?
(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.
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...
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.
How do other browsers behave?
WebKit, Blink, Presto do not submit the form in this case when I close the dropdown via enter.
Attached file testcase (obsolete) —
It's odd... This test case causes firing submit event on all browsers...
(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.
OS: Mac OS X → All
Hardware: x86 → All
Attached file testcase
Attachment #788744 - Attachment is obsolete: true
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.
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?
Flags: needinfo?(bugs)
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.
(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
Flags: needinfo?(bugs)
Comment on attachment 788870 [details] [diff] [review]
Consume Enter key event if dropdown is closed by it

Okay, let's go.
Attachment #788870 - Flags: review?(bugs)
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
Attachment #788870 - Flags: review?(bugs) → review+
Thank you!
https://hg.mozilla.org/integration/mozilla-inbound/rev/64324e19ca10
Status: NEW → ASSIGNED
Version: unspecified → Trunk
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.
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).
(I need the feedback or need-info to remind myself.)
Thanks, but I landed new patch which disables the new test on Android.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aebcc13238f5
https://hg.mozilla.org/mozilla-central/rev/aebcc13238f5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(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.
Flags: needinfo?(masayuki)
This will have unknown web impact, and risk will only get larger the later we uplift. Let's get this into Aurora 25.
(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.
Attachment #792080 - Flags: review?(gps)
Flags: needinfo?(masayuki)
Attachment #792080 - Flags: review?(gps) → review+
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.
Attachment #792080 - Flags: review?(ahalberstadt)
Attachment #792080 - Flags: review?(ahalberstadt) → review+
Can I take the patches to Aurora only with tracking-firefox25+? Or do I need to request approval for each patch?
Flags: needinfo?(akeybl)
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
Attachment #788870 - Flags: approval-mozilla-aurora?
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
Attachment #792080 - Flags: approval-mozilla-aurora?
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
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).
Flags: needinfo?(akeybl)
(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).
> 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.
Attachment #788870 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #792080 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.