Closed Bug 935876 Opened 11 years ago Closed 10 years ago

<select> element shouldn't consume key events which don't cause any default action

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Version:
25 Branch
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla29
Tracking Flags:
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: lpaduraru, Assigned: masayuki)

References

(Depends on 1 open bug)

Details

(4 keywords, Whiteboard: [bugday-20131111])

Attachments

(5 files, 3 obsolete files)

testcase
470 bytes, text/html
Details
testcase
1.00 KB, text/html
Details
part.1 Don't consume key event on <select> element if it's never handled
7.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.2 Add new tests
17.97 KB, patch
smaug
: review-
Details | Diff | Splinter Review
part.2 Add new tests
18.16 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131025151332

Steps to reproduce:

Not working in Mozilla Firefox 25.0:

<html>
<head>
<title>Test</title>
</head>
<body>
<form name="forma" method="get" action="test1.php">
Test <select onkeypress="forma.submit()" size="2"><option>Da</option><option>Nu</option></select>
</form>
</body>
</html>
Works for me on 25: the form submits when I press a key while the select is focused.  Please try to reproduce in safe mode with all add-ons disabled: https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode
Flags: needinfo?(lpaduraru)
1. It does not function either in safe-mode.
2. The problem is that it does not function when I press <Enter>, but it works when I press <Tab>.
3. The whole code functions properly when I press <Enter> in other Firefox versions or IE.
4. The code functions properly in Firefox 25.0 if only I modify size="2" in size="1".
Attached file testcase — 

    
    

    
  
Reproduced comment #2 with 25.0 and 2013-11-10-03-02-05-mozilla-central-firefox-28.0a1.en-US.linux-x86_64

wfm: 2013-07-24-03-02-04-mozilla-central-firefox-25.0a1.ru.linux-x86_64    2983ca6d4d1a
bug: 2013-07-25-17-15-58-mozilla-central-firefox-25.0a1.en-US.linux-x86_64 76fb8f815ac7

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2983ca6d4d1a&tochange=76fb8f815ac7
Blocks: 501496
Status: UNCONFIRMED → NEW
Component: Untriaged → Event Handling
Ever confirmed: true
Flags: needinfo?(lpaduraru)
Keywords: reproducible
OS: Windows XP → All
Product: Firefox → Core
Whiteboard: [bugday-20131111]

    
    

    
  
See bug 903715. We assume that nobody expect "double action" for a key operation. Therefore, if keydown event is consumed by the <select>'s default action, keypress event is never fired anymore.

If web apps need to override the behavior, they should handle keydown event.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID

    
    

    
  
Is bug 903715 relevant? The testcase of this bug contains no dropdown (because of "size=2").

    
  
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

    
    

    
  

      
      
      
      

        Attached file
          
        testcase (obsolete)
        — 
      

    


  
At least Chrome and IE11 didn't prevent default action by default if the dropdown is closed or no dropdown is present from the start. I support reopening.

    
    

    
  
Let's sort out what's the problem of this bug.

If you think that keypress event should be fired even if a default action occurs, I don't agree with this. See bug 903925. Such behavior causes "double action". I don't think that it's better for users.

If you think that keypress event should be fired if any default action doesn't occur, it might be better.

I understood this bug is as the former. It this the latter?

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> Let's sort out what's the problem of this bug.
> 
> If you think that keypress event should be fired even if a default action
> occurs, I don't agree with this. See bug 903925. Such behavior causes
> "double action". I don't think that it's better for users.

I'm saying the "default action" for <select> elements should not submit the form. Chrome for Windows, IE11, and older versions of Firefox didn't prevent the default action, but also didn't submit. (Try the testcase if you don't believe.)
The "double action" will not occur because the "default action" will not submit the form itself.

    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  
Added an <input> element for comparison.

        Attachment #830166 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #9)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> > Let's sort out what's the problem of this bug.
> > 
> > If you think that keypress event should be fired even if a default action
> > occurs, I don't agree with this. See bug 903925. Such behavior causes
> > "double action". I don't think that it's better for users.
> 
> I'm saying the "default action" for <select> elements should not submit the
> form. Chrome for Windows, IE11, and older versions of Firefox didn't prevent
> the default action, but also didn't submit. (Try the testcase if you don't
> believe.)

So, do you agree with only the latter case should be fixed?

I should explain the background of current behavior.

The reason why bug 501496 moves a part of default action handler from keypress to keydown is for more serious compatibility with the other browsers. On the other browsers, even if preventDefault() of a keydown event is called, it doesn't cause preventing default action of <select> element. Therefore, our old code ignored defaultPrevented value intentionally.
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp?rev=a03c584f9874#2078

If we still used keypress event handler for handling default action, a call of preventDefault() of keydown would cause only our users cannot operate <select> element on such page because of no keypress event. (E.g., user may not be able to close dropdown on such page!)

By this change, we met bug 903715. The cause of the bug is, not preventing default of keydown event during dropdown open causes dispatching keypress event on new focused <input> element and it causes submitting the form. This bug also has Chrome for Windows and IE but not so Chrome for Mac and Safari. So, the other browsers behave inconsistent between platforms. We chose the better behavior, Mac version.

This is the reason why I don't agree with fixing the former case of my previous comment.

I believe that we should fix only the latter case of it.

    
    

    
  
Anyway, it's not clear what is a reported problem from comment 0. It looks like all keys should kick the keypress event handler. Then, the request is INVALID as I decided because it's by design mentioned in comment 11.

    
    

    
  
Hm, I must have been screwed up.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → INVALID

    
    

    
  
File test.html
--------------
<html>
<head>
<title>Test</title>
</head>
<script type="text/javascript">
function enter(myfield,e)
{
var keycode;
if (window.event)
  keycode=window.event.keyCode;
else
  if (e)
    keycode=e.which;
  else
    return true;
if (keycode==13)
  return false;
else
  return true;
}
function submitenter(myfield,e)
{
var keycode;
if (window.event)
  keycode=window.event.keyCode;
else
  if (e)
    keycode=e.which;
  else
    return true;
if (keycode==13)
{
  myfield.form.submit();
  return false;
}
else
  return true;
}
function dezactiveazaenter(evt)
{
var k=evt.keyCode || evt.which;
return k!=13;
}
</script>
<body onload="document.getElementById('s1').focus()">
<form name="forma" method="get" action="test1.php">
Test <select name="s1" id="s1" onkeypress="if (!enter(this,event)) { forma.submit(); return dezactiveazaenter(event); }" size="2"><option>Da</option><option>Nu</option></select>
</form>
</body>
</html>

==================================

File test1.php
--------------
<?php
echo $_GET["s1"];
?>

==================================

Not working in Mozilla Firefox 25.0, but it works in IE, for example.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

    
    

    
  
Thank you, I recalled what was the problem I thought here.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> If you think that keypress event should be fired even if a default action
> occurs, I don't agree with this. See bug 903925. Such behavior causes
> "double action". I don't think that it's better for users.

To be clear, I'm not proposing this.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11)
> On the other browsers, even if preventDefault() of a keydown event
> is called, it doesn't cause preventing default action of <select> element.

It is untrue at least on Chrome for Windows and IE 11 if the dropdown is not open/present and Enter key is pressed. If preventDefault() of the keydown event is called, both browsers prevented the default action (if my understanding is correct, firing a keypress event is a part of the default action of the keydown event).

> If we still used keypress event handler for handling default action, a call
> of preventDefault() of keydown would cause only our users cannot operate
> <select> element on such page because of no keypress event. (E.g., user may
> not be able to close dropdown on such page!)

At least on Chrome for Windows and IE 11, I couldn't operate <select> elements if I called preventDefault() in the keydown event handler. I can agree that it is a bad UX, but our new behavior is not "for more serious compatibility with the other browsers" because it is less compatible with other browsers.

>  not preventing default of keydown event during dropdown open

Again, I'm saying about the case of <select> elements without dropdown. We shouldn't prevent the default action in this case because it looks like that Enter key has no default actions (except for firing a keypress event) for the default keydown handler of <select> elements.

    
    

    
  
To summarize what I'm proposing, we should fire a keypress event if we meet all following conditions:
* The dropdown menu of the <select> element is close or is not present at all,
* the Enter key is pressed,
* and the keydown handler didn't call preventDefault() to prevent the default action.

Nakano-san, do you have any other unclear points?
Flags: needinfo?(masayuki)

    
    

    
  
(In reply to Masatoshi Kimura [:emk] from comment #15)
> Thank you, I recalled what was the problem I thought here.
> 
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> > If you think that keypress event should be fired even if a default action
> > occurs, I don't agree with this. See bug 903925. Such behavior causes
> > "double action". I don't think that it's better for users.
> 
> To be clear, I'm not proposing this.

Okay, then this case is really INVALID as I said.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11)
> > On the other browsers, even if preventDefault() of a keydown event
> > is called, it doesn't cause preventing default action of <select> element.
> 
> It is untrue at least on Chrome for Windows and IE 11 if the dropdown is not
> open/present and Enter key is pressed. If preventDefault() of the keydown
> event is called, both browsers prevented the default action (if my
> understanding is correct, firing a keypress event is a part of the default
> action of the keydown event).

Okay, and yes, your understanding is correct.

> > If we still used keypress event handler for handling default action, a call
> > of preventDefault() of keydown would cause only our users cannot operate
> > <select> element on such page because of no keypress event. (E.g., user may
> > not be able to close dropdown on such page!)
> 
> At least on Chrome for Windows and IE 11, I couldn't operate <select>
> elements if I called preventDefault() in the keydown event handler. I can
> agree that it is a bad UX, but our new behavior is not "for more serious
> compatibility with the other browsers" because it is less compatible with
> other browsers.

Indeed. But I don't want to accept the "bad UX" at least in critical cases (especially, cannot close dropdown is system-wide attack). And I think that this is out of scope of this bug. If we need to "improve" this behavior, let's separate bug.

> >  not preventing default of keydown event during dropdown open
> 
> Again, I'm saying about the case of <select> elements without dropdown. We
> shouldn't prevent the default action in this case because it looks like that
> Enter key has no default actions (except for firing a keypress event) for
> the default keydown handler of <select> elements.

(In reply to Masatoshi Kimura [:emk] from comment #16)
> To summarize what I'm proposing, we should fire a keypress event if we meet
> all following conditions:
> * The dropdown menu of the <select> element is close or is not present at
> all,
> * the Enter key is pressed,
> * and the keydown handler didn't call preventDefault() to prevent the
> default action.
> 
> Nakano-san, do you have any other unclear points?

Only Enter key case? I don't think that it makes sense. If we need to change the behavior, I believe that we should not prevent following keypress events of all keydown events which don't cause any default action. I mean, we should change the behavior as: if default action handler of <select> doesn't handle keydown event, it shouldn't call preventDefault(). Isn't this your suggest?
Flags: needinfo?(masayuki)

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> Indeed. But I don't want to accept the "bad UX" at least in critical cases
> (especially, cannot close dropdown is system-wide attack).

It's impossible that users can't close dropdown because the dropdown is not open :)

> Only Enter key case? I don't think that it makes sense. If we need to change
> the behavior, I believe that we should not prevent following keypress events
> of all keydown events which don't cause any default action. I mean, we
> should change the behavior as: if default action handler of <select> doesn't
> handle keydown event, it shouldn't call preventDefault(). Isn't this your
> suggest?

Yes, Enter key is just an example. We should investigate what keys will consume the event and what keys will not. At least arrow keys and tab key didn't fire keypress events on other browsers (and obviouly they have default actions).
Note that some keys (including Enter) will consume keydown events only if the dropdown is open.

    
    

    
  
Okay, we now get agreement how we should fix this bug, changing the summary.
Summary: select onkeypress size → <select> element shouldn't consume key events which don't cause any default action

    
    

    
  
Hmm, if multiple attribute is specified, Enter key is used for selecting only focused option. So we can change the behavior only when <select size="n"> (n>=2).

    
    

    
      
  

    
    

    
  
(In reply to Liviu PADURARU from comment #21)
> The bug has not been resolved either FF 25.0.1.

Please don't post such spam comment. You should check the bug status.

    
    

    
      
  

    
    

    
  
Liviu:

Please don't post such spam comment.

    
    

    
  


  
Assignee: nobody → masayuki
Status: REOPENED → ASSIGNED

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part.2 Add new tests (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8362908 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8362907 [details] [diff] [review]
part.1 Don't consume key event on <select> element if it's never handled

Unfortunately, the newest part.2 patch in my tree hasn't been tested with B2G. However, this is a regression. Let's fix first. I'd like to land this during 29 cycle.

This patch doesn't call preventDefault() if the key is never handled with the <select> element instance. If we call preventDefault() only when the key event does something default action (e.g., except arrow keys which doesn't actually change selected item), web apps may be confused because it really depends on our behavior change. Therefore, I believe that we should always call preventDefault() if the key event may cause default action.

        Attachment #8362907 -
        Flags: review?(enndeakin)

    
    

    
  
Comment on attachment 8362907 [details] [diff] [review]
part.1 Don't consume key event on <select> element if it's never handled

Sorry, I don't think I should review this.

        Attachment #8362907 -
        Flags: review?(enndeakin)

    
    

    
  
Comment on attachment 8362907 [details] [diff] [review]
part.1 Don't consume key event on <select> element if it's never handled

Thanks, Enn. Smaug, could you review this? See the previous my comment for the detail.

I'd like to land this patch in 29 cycle.

        Attachment #8362907 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8362907 [details] [diff] [review]
part.1 Don't consume key event on <select> element if it's never handled

>     default: // printable key will be handled by keypress event.
>+      incrementalSerchResetter.Cancel();
Serch? I guess it should be Search

>+  AutoIncrementalSearchResetter incrementalSerchResetter;
Search



This is rather scary stuff. Could cause regressions.

        Attachment #8362907 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #31)

Thanks!

> This is rather scary stuff. Could cause regressions.

Indeed. But we need to try do behave similar as other browsers as far as possible...

    
    

    
  


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

Okay, finally, the new tests passed on all environment.

See bug 962029 for the detail of disabling test_bug746272-2.html. This causes moving the boundary between test_bug746272-1.html and test_bug746272-2.html.

        Attachment #8364928 -
        Flags: review?(bugs)

        Attachment #8362909 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8364928 [details] [diff] [review]
part.2 Add new tests

I was expecting to see tests for the cases when preventDefault() is called.
Could you add still defaultPrevented() checks.

        Attachment #8364928 -
        Flags: review?(bugs) → review-
Whiteboard: [bugday-20131111][leave open] → [bugday-20131111]

    
    

    
  
Ah, I'll remove the diff of dom/tests/mochitest/localstorage/mochitest.in since the test has been disabled on Android (bug 962029 comment 4). Please ignore it.

        Attachment #8366422 -
        Flags: review?(bugs) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/1ab856ce8afe
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

    
    

    
  
It was resolved in Mozilla Firefix 29.0.

    
    

    
  
v. per comment #42.
Status: RESOLVED → VERIFIED

    
  
Depends on: 1008244

    
    

    
  
The error is resolved in Mozilla Firefix 29.0(select with size=2,3 etc.), but it does not work any more select with size=1.

    
  

          status-firefox29:
          fixed → ---

    
  
Severity: normal → major

    
    

    
  
(In reply to Liviu PADURARU from comment #46)
> The error is resolved in Mozilla Firefix 29.0(select with size=2,3 etc.),
> but it does not work any more select with size=1.

Your issue, apparently a regression, is tracked in Bug 1008244. This bug itself is fixed in 29.
Severity: major → normal

          status-firefox29:
          --- → fixed

    
    

    
  
All the errors are resolved in Mozilla Firefox 30.0.
Component: Event Handling → User events and focus handling

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: