Closed Bug 291082 Opened 19 years ago Closed 11 years ago

preventDefault doesn't block keyboard navigation in select-one drop-down lists

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: thatguy, Assigned: almasry.mina)

Details

(Whiteboard: [mentor=Neil][lang=c++])

Attachments

(2 files, 16 obsolete files)

Better test cases
2.21 KB, text/html
Details
patch
8.82 KB, patch
neil
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

When a JavaScript event listener handles a keyboard event in a <select>
drop-down list, it should prevent the browser from handling the event itself by
calling e.preventDefault(), where e is the event object.

Keyboard navigation (including arrow keys and "type what you want") in a
<select> drop-down list occurs even if an event listener calls preventDefault on
the event(s) in question.

Reproducible: Always

Steps to Reproduce:
1. Load this document:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">
<html>
  <head>
    <title>Dropdown list</title>
  </head>
  <body>
  <form>
    <select id="s1">
      <option>abcdef</option>
      <option>ghijkl</option>
      <option>mnopqr</option>
      <option>stuvwx</option>
    </select>
  </form>
  <script type="text/javascript">
    var s1 = document.getElementById('s1');
    s1.addEventListener('keypress', function(e) { e.preventDefault(); }, false);
    s1.addEventListener('keyup', function(e) { e.preventDefault(); }, false);
    s1.addEventListener('keydown', function(e) { e.preventDefault(); }, false);
  </script>
  </body>
</html>

2. Tab to give focus to the drop-down list.
3. Type 'g'.

Actual Results:  
The second option in the list is selected.

Expected Results:  
Nothing. The script should prevent the default actions for the keyboard events.
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Confirmed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20051005 Firefox/1.6a1.
Status: UNCONFIRMED → NEW
Component: Keyboard Navigation → Keyboard: Navigation
Ever confirmed: true
Product: Firefox → Core
QA Contact: jruderman → keyboard.navigation
Version: unspecified → Trunk
Attached file Testcase (obsolete) — 
The Bug is still there! 

I also adapted the example code of "Aaron Leventhal" and put it as attachment.
Attached file simple testcase (obsolete) — 

    
  

        Attachment #207804 -
        Attachment is obsolete: true

    
    

    
  
This is still not fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 or latest nightly build of firefox.
I also added another example that works in both ie7 and ie6.

    
    

    
  

      
      
      
      

        Attached file
          
        example showing the bug (obsolete)
        — 
      

    


  

    
    

    
  
This bug also occurs when the <select> is multiple (some comments on the web about this seem to indicate the problem didn't occur on Linux, though I'm using Windows as with the above users).

    
    

    
  
Oh, and this is occurring in FF3b4 as well as in FF2

    
    

    
  
Mass un-assigning bugs assigned to Aaron.
Assignee: aaronleventhal → nobody

    
    

    
  
Still broken:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6

    
    

    
  
bug still present in FF 4.0.1

    
    

    
  
Still present in Firefox 7.

My company is creating a spreadsheet-like application where some cells are represented as <select> element. We implement cell navigation using the arrow keys to improve accessibility, but this bug causes problems.

Since event.preventDefault is not preventing the browser's default behavior for the arrow keys (i.e. change the selected option), both a value change and cell navigation take place in Firefox. See attachment for example.

Note that Internet Explorer (tested with 8 and 9), Chrome (tested with 14) and Safari (tested with 5.1) all respect event.preventDefault. and that Firefox perfectly respects event.preventDefault for other form controls like <input type="text">.

(please no comments whether navigation using arrow keys is bad practice; Excel and Google Docs supports it, and our client wants it; the popup can be called with ALT+DOWN when the <select> has focus.)

    
    

    
  


  
Added attachment.

    
    

    
  


  

        Attachment #564884 -
        Attachment is obsolete: true

    
    

    
  
Can't seem to set the attachment type to text/html, sorry.

    
    

    
  
Is this as simple as checking defaultPrevented in nsListControlFrame::KeyPress?

    
  

        Attachment #564892 -
        Attachment mime type: text/plain → text/html

    
  

        Attachment #564888 -
        Attachment is obsolete: true

        Attachment #564888 -
        Attachment mime type: text/plain → text/html

    
    

    
  
(In reply to neil@parkwaycc.co.uk from comment #17)
> Is this as simple as checking defaultPrevented in
> nsListControlFrame::KeyPress?

Yes Neil, according to Boris on stackoverflow: http://stackoverflow.com/questions/8870037/preventdefault-not-working-on-select-elements-in-firefox-9-0-1
OS: Windows XP → All

    
  
Whiteboard: [mentor=Neil][lang=C++]

    
  
Whiteboard: [mentor=Neil][lang=C++] → [mentor=Neil][lang=c++]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patchV1 (obsolete)
        — 
          — Splinter Review
      

    


  
This patch seems to work except for the second test which also doesn't work on Chrome.

        Attachment #599342 -
        Flags: feedback?(neil)

    
    

    
  
Sorry It's the first test case that doesn't work

    
    

    
  
Comment on attachment 599342 [details] [diff] [review]
patchV1

This fixes the case of <select onkeydown="event.preventDefault();">. Unfortunately it doesn't appear to fix the case of <select onkeypress="event.preventDefault();">, this seems to be because the list frame isn't using system event listeners (compare the file control frame).

The first test case fails because it uses the wrong attribute and variable.

At some point this will need a mochitest but I'm no good at writing those.

        Attachment #599342 -
        Flags: feedback?(neil) → feedback+

    
    

    
  
Neil, can you attach your test case? I'm testing with <select onkeypress|onkeydown|onkeyup="event.preventDefault();"> and with this patch only the onkeyup let me move through the list of options.

    
    

    
  
I don't have it handy but it scripted the choice of attribute. I can see how having the attribute in the HTML would result in different behaviour.

    
    

    
  

      
      
      
      

        Attached file
          
        Some more test cases (obsolete)
        — 
      

    


  
In particular this adds the keydown and keypress event listeners in different ways to try and catch as many issues as I can. But it turns out that patchV1 fixes all the keydown event listeners and none of the keypress event listeners.

    
    

    
  
Oh, and the test case only blocks using digits, the arrow keys still work. I did this because I didn't want to block the tab key ;-)

    
    

    
  

      
      
      
      

        Attached file
          
        Better test cases
        — 
      

    


  
Ah, that's because of a bug in my test. This one gets it right, so patchV1 now fixes four out of six of these tests.

        Attachment #648803 -
        Attachment is obsolete: true

    
    

    
  
Neil, I want to fix this bug but I need help, how I contact you on IRC?

    
    

    
  
Hi I'm new here. This bug looks open. Can I get assigned this?

    
  
Assignee: nobody → almasry.mina

    
    

    
  
Sorry for the late followup.

I'm a little confused here. I'm testing patchV1, and I see it fixes test in comment #0

In the "Better test cases" attachment, I can see the lists but there is no expected outcome. The behavior of firefox seems to be exactly that of google chrome, which is the arrows change the values. What is needed here?

    
    

    
  
(In reply to Mina Almasry from comment #29)
> In the "Better test cases" attachment, I can see the lists but there is no
> expected outcome. The behavior of firefox seems to be exactly that of google
> chrome, which is the arrows change the values. What is needed here?

As alluded to in comment #25, the expected outcome is that pressing the digits 0-9 does not change the selection.

    
    

    
  
When I run gdb with the patch applied, I get two things:

1: in the cases where the event is prevented, preventDefault() runs _before_ the event is handled, and therefor the expected result happens

2: in the case where the event is not prevented, preventDefault() runs _after_ the event is handled, and therefore the event is not prevented.

What controls which gets called before which? Any ideas on how to fix this?

    
    

    
  
(In reply to Mina Almasry from comment #31)
> When I run gdb with the patch applied, I get two things:
> 
> 1: in the cases where the event is prevented, preventDefault() runs _before_
> the event is handled, and therefor the expected result happens
> 
> 2: in the case where the event is not prevented, preventDefault() runs
> _after_ the event is handled, and therefore the event is not prevented.
> 
> What controls which gets called before which?
I think there are five phases of event processing in Gecko:
1. ESM pre-event handling
2. Normal event dispatch
3. Frame event handling
4. System event dispatch
5. ESM post-event handling

Phase 2 is interesting to us here because that's what script uses. Events can be captured before they reach the target, then they are handled on the target itself, then they bubble out again.

Because the list control frame code is also trying to use phase 2, the order of event processing depends on how the event listener is attached.

Note that there is an extra consideration for keydown and keypress events - the preventDefault state on a keydown event is passed to the keypress event before it is even dispatched.

> Any ideas on how to fix this?
As I mentioned in comment #21, this can be fixed by switching the list control frame code to use phase 4, which is therefore always after the web page.

    
    

    
  
Okay I understand that nsListControlFrame uses normal event dispatching and we would like it to use system event dispatching instead, like nsFileControlFrame. But I don't know what both of these are and I looked for guides on the subject but haven't found any. How do I switch nsListControlFrame to use system event dispatching?

    
    

    
  
It looks like the procedure for adding a system event listener has markedly improved since they were first introduced, so all you need do is to use the AddSystemEventListener method instead of the AddEventListener method.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proposed patch v2 with test (obsolete)
        — 
          — Splinter Review
      

    


  
Yep, changing the event listener to system ones did the trick. Here is a patch with a mochitest.

    
    

    
  
Comment on attachment 769952 [details] [diff] [review]
proposed patch v2 with test

You'll want to fix up the removal of the event listeners too. Then you'll be able to ask :mounir or :dbaron for review (or there are other potential requestees too, if you already had one in mind).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proposed patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Here is the finished patch then, ready for review, afaict. There is also a mochitest.

I'm not complaining, but was there a way for me to figure out that I needed to do that on my own? I feel like a lot of what I do around here is follow very detailed instructions on which lines of code to change. Are there guides/docs I'm missing? Or do people usually find their way around the code by reading it?

        Attachment #769952 -
        Attachment is obsolete: true

        Attachment #770082 -
        Flags: superreview?

        Attachment #770082 -
        Flags: review?

    
  

        Attachment #770082 -
        Flags: review? → review?(mounir)

    
    

    
  
Comment on attachment 770082 [details] [diff] [review]
proposed patch v3

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

::: layout/forms/nsListControlFrame.cpp
@@ +149,5 @@
> +                                      mEventListener, false);
> +  mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"),  
> +                                      mEventListener, false);
> +  mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"), 
> +                                      mEventListener, false);

Could you revert those changes? It seems to be only about re-indent and adding trailing whitespaces.

@@ +1003,5 @@
> +                                   false, false);
> +  mContent->AddSystemEventListener(NS_LITERAL_STRING("mouseup"), mEventListener,
> +                                   false, false);
> +  mContent->AddSystemEventListener(NS_LITERAL_STRING("mousemove"), mEventListener,
> +                                   false, false);

ditto

@@ +2269,5 @@
> +  aKeyEvent->GetDefaultPrevented(&defaultPrevented);
> +  if (defaultPrevented) {
> +    return NS_OK;
> +  }
> +

The current behaviour I see in most browsers is that you can do "type-and-search" in a <select> when the key event is cancelled but you can use the arrows to navigate between the different value or press space to open the list and then navigate as you like. It sounds like this patch is actually prevent any key event to go to the <select> frame if it has been cancelled. Is that correct?

You should add tests for that.

::: layout/forms/test/Makefile.in
@@ +45,4 @@
>  		test_bug672810.html \
>  		test_bug704049.html \
>  		test_listcontrol_search.html \
> +		test_bug291082.html \

Please, have a proper name for the test file like:
test_select_prevent_default.html or whatever you see fit.

        Attachment #770082 -
        Flags: superreview?

        Attachment #770082 -
        Flags: review?(mounir)

        Attachment #770082 -
        Flags: review-

    
    

    
  
Mina, feel free to ping me on IRC (mounir) if you need more information regarding how to fix the bug. I would gladly help.

    
    

    
  
> > +                                      mEventListener, false);
> > +  mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"),  
> > +                                      mEventListener, false);
> > +  mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"), 
> > +                                      mEventListener, false);
> 
> Could you revert those changes? It seems to be only about re-indent and
> adding trailing whitespaces.

RemoveEventListener has been changed to Remove*System*EventListener. The trailing whitespace is because I recently turned off autodeletion of trailing whitespace in my editor, because mozilla-central has a ton of trailing whitespace that is giving me errors. I can resubmit without the trailing whitespace.

> 
> @@ +1003,5 @@
> > +                                   false, false);
> > +  mContent->AddSystemEventListener(NS_LITERAL_STRING("mouseup"), mEventListener,
> > +                                   false, false);
> > +  mContent->AddSystemEventListener(NS_LITERAL_STRING("mousemove"), mEventListener,
> > +                                   false, false);
> 
> ditto

AddEventListener has been changed to Add*System*EventListener

> The current behaviour I see in most browsers is that you can do
> "type-and-search" in a <select> when the key event is cancelled but you can
> use the arrows to navigate between the different value or press space to
> open the list and then navigate as you like. It sounds like this patch is
> actually prevent any key event to go to the <select> frame if it has been
> cancelled. Is that correct?
> 
> You should add tests for that.

Yes the patch prevents any key event, if it has been cancelled. That is the way I was told to fix the bug as per the discussion here. You can still use the arrow keys and space bar, if they haven't been cancelled. If the have been cancelled, however, this patch will block their execution. Isn't that the desired behaviour?

The patch tests that keys that haven't been cancelled do go through, and ones that have been cancelled don't go through.

Clicking space bar to open the list doesn't seem to be a feature of firefox in general. It is not in the firefox I have installed or in nightly. We could work on adding the behaviour of clicking space to open the list, but I would like to do that in another issue, and control the scope of this one, please.

> 
> ::: layout/forms/test/Makefile.in
> @@ +45,4 @@
> >  		test_bug672810.html \
> >  		test_bug704049.html \
> >  		test_listcontrol_search.html \
> > +		test_bug291082.html \
> 
> Please, have a proper name for the test file like:
> test_select_prevent_default.html or whatever you see fit.

I followed convention, but I could change this. Please advise on what to do on all the other issues.

    
    

    
  
(In reply to Mina Almasry from comment #40)
> > > +                                      mEventListener, false);
> > > +  mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"),  
> > > +                                      mEventListener, false);
> > > +  mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"), 
> > > +                                      mEventListener, false);
> > 
> > Could you revert those changes? It seems to be only about re-indent and
> > adding trailing whitespaces.
> 
> RemoveEventListener has been changed to Remove*System*EventListener. The
> trailing whitespace is because I recently turned off autodeletion of
> trailing whitespace in my editor, because mozilla-central has a ton of
> trailing whitespace that is giving me errors. I can resubmit without the
> trailing whitespace.

Sorry, with the indentation change, I missed the Event -> System renaming. My bad.
Why do you need to make those system event listeners?

> > The current behaviour I see in most browsers is that you can do
> > "type-and-search" in a <select> when the key event is cancelled but you can
> > use the arrows to navigate between the different value or press space to
> > open the list and then navigate as you like. It sounds like this patch is
> > actually prevent any key event to go to the <select> frame if it has been
> > cancelled. Is that correct?
> > 
> > You should add tests for that.
> 
> Yes the patch prevents any key event, if it has been cancelled. That is the
> way I was told to fix the bug as per the discussion here. You can still use
> the arrow keys and space bar, if they haven't been cancelled. If the have
> been cancelled, however, this patch will block their execution. Isn't that
> the desired behaviour?
> 
> The patch tests that keys that haven't been cancelled do go through, and
> ones that have been cancelled don't go through.
> 
> Clicking space bar to open the list doesn't seem to be a feature of firefox
> in general. It is not in the firefox I have installed or in nightly. We
> could work on adding the behaviour of clicking space to open the list, but I
> would like to do that in another issue, and control the scope of this one,
> please.

I do not want us to go further than what other browsers do. Currently, it seems that all browsers allow every key interaction when there is prevent default (tested using the "better test case"). The only one not allowed is doing search. I am afraid that if we prevent all key interactions we might break content.

> > ::: layout/forms/test/Makefile.in
> > @@ +45,4 @@
> > >  		test_bug672810.html \
> > >  		test_bug704049.html \
> > >  		test_listcontrol_search.html \
> > > +		test_bug291082.html \
> > 
> > Please, have a proper name for the test file like:
> > test_select_prevent_default.html or whatever you see fit.
> 
> I followed convention, but I could change this. Please advise on what to do
> on all the other issues.

It is indeed a convention to use test_bugXXXXX.html but it is a pretty bad convention because you usually have no idea what the test is about. Having a name that would help another developer to get the gist of the test without opening it would be a win. And really, pick whatever name you think would be good. Look at content/html/content/test/forms/ to see some examples.

    
    

    
  
> Sorry, with the indentation change, I missed the Event -> System renaming.
> My bad.
> Why do you need to make those system event listeners?

as per comment #31 and comment #32, if the listeners aren't system listeners, the event is handled before the code realizes it has been prevented. We had to change the listener to a system listener to delay the handling of the event, as is being done in nsFileControlFrame.cpp

> I do not want us to go further than what other browsers do. Currently, it
> seems that all browsers allow every key interaction when there is prevent
> default (tested using the "better test case"). The only one not allowed is
> doing search. I am afraid that if we prevent all key interactions we might
> break content.

Okay, I see what you mean. You'll have to bear with me, because I am new. Can you point me to how to figure out if the key is pressed as "doing search" or part of something else? Then I can block it if it is doing search, and allow it otherwise.

What I think can do is check whether the list is opened (you can see all the options) or closed (you can't see all the options). If it is open, then allow the event, if it is closed, prevent the event. This, I think, gives us identical behaviour to chrome, except for "press space to open the list", because that's not in firefox at all.

    
    

    
  
(In reply to Mina Almasry from comment #42)
> Okay, I see what you mean. You'll have to bear with me, because I am new.
> Can you point me to how to figure out if the key is pressed as "doing
> search" or part of something else? Then I can block it if it is doing
> search, and allow it otherwise.
> 
> What I think can do is check whether the list is opened (you can see all the
> options) or closed (you can't see all the options). If it is open, then
> allow the event, if it is closed, prevent the event. This, I think, gives us
> identical behaviour to chrome, except for "press space to open the list",
> because that's not in firefox at all.

I think the best way to handle that is to keep your preventDefault check but only do an early return if the key is not part of a specific set. The set could be key_down, key_up, key_left, key_right, page_down, page_up, home and end. You should not check the modifiers. Maybe some of those keys are not used by the element (you can read the code after your preventDefault check to verify that). Note that some keys might be used depending on the platform.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proposed patch v4 with test (obsolete)
        — 
          — Splinter Review
      

    


  
Aye aye, captain. I made the changes you asked for:

- prevent event only if the key is *not* part of the set you specified.
- change name of test file.
- added test for checking that "down" works even though it is blocked

        Attachment #599342 -
        Attachment is obsolete: true

        Attachment #770082 -
        Attachment is obsolete: true

        Attachment #770829 -
        Flags: review?(mounir)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proposed patch v5 with test (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed typo

        Attachment #770829 -
        Attachment is obsolete: true

        Attachment #770829 -
        Flags: review?(mounir)

        Attachment #770833 -
        Flags: review?(mounir)

    
    

    
  
In my webapp, key_left, key_right, page_down, page_up, home and end are the keys I'm trying to prevent the default behavior from happening. The point of preventDefault.

    
    

    
  
(In reply to ricky from comment #46)
> In my webapp, key_left, key_right, page_down, page_up, home and end are the
> keys I'm trying to prevent the default behavior from happening. The point of
> preventDefault.

Are you able to do that with other browsers? Opera, IE, Chrome, Safari?

    
    

    
  
Comment on attachment 770833 [details] [diff] [review]
proposed patch v5 with test

>diff -r 0a10eca0c521 -r 7be441c833e6 layout/forms/nsListControlFrame.cpp
>--- a/layout/forms/nsListControlFrame.cpp	Sat Mar 23 18:45:56 2013 -0400
>+++ b/layout/forms/nsListControlFrame.cpp	Wed Jul 03 11:16:14 2013 -0400
>@@ -2278,6 +2278,28 @@
>   keyEvent->GetKeyCode(&keycode);
>   keyEvent->GetCharCode(&charcode);
>
>+  // if the event has been prevented and the key is not in a specific set,
>+  // return

nit: please use uppercase and dots: "If the event [...] return."
Also, you should explain why some keys are excluded.

>+  bool defaultPrevented;
>+  aKeyEvent->GetDefaultPrevented(&defaultPrevented);
>+  if (defaultPrevented) {
>+    switch (keycode) {
>+      case nsIDOMKeyEvent::DOM_VK_UP:
>+      case nsIDOMKeyEvent::DOM_VK_LEFT:
>+      case nsIDOMKeyEvent::DOM_VK_DOWN:
>+      case nsIDOMKeyEvent::DOM_VK_RIGHT:
>+      case nsIDOMKeyEvent::DOM_VK_PAGE_UP:
>+      case nsIDOMKeyEvent::DOM_VK_PAGE_DOWN:
>+      case nsIDOMKeyEvent::DOM_VK_HOME:
>+      case nsIDOMKeyEvent::DOM_VK_END: {
>+                                         break;
>+                                       }
>+      default: {
>+                 return NS_OK;
>+               }
>+    }
>+  }

I think there is an indentation issue here.

>@@ -45,6 +45,7 @@
> 		test_bug672810.html \
> 		test_bug704049.html \
> 		test_listcontrol_search.html \
>+		test_select_prevent_default.html \
> 		$(NULL)

The patch you attached is showing this as part of nsListControlFrame.cpp, it should be part of the Makefile.
Did you edit the patch manually?

    
    

    
  
Comment on attachment 770833 [details] [diff] [review]
proposed patch v5 with test

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

I tried the test locally and it never succeeded (timeout).

I think generally speaking you could improve the tests a bit by having it more dynamic.
You could start by having an array of "testData" that would contain the ids of the elements to test.
For each element, you would try to press '2', DOWN, UP, LEFT, RIGHT, PAGE_UP, PAGE_DOWN, HOME and END. The first key shouldn't change the value. Every other keys should. You could have all the other keys living in an array and have a for loop. Finally, you should focus the element by calling element.focus(); instead of TAB.

The general code would look like:

var otherKeys = [ "DOWN", "UP", "LEFT", "RIGHT", "PAGE_UP", "PAGE_DOWN", "HOME", "END" ];
testDate.forEach(function(id) {
  var element = document.getElementById(id);
  element.focus();

  var previousValue = element.value;
  synthesizeKey(element, '2', {});
  is(element.value, previousValue, "value should not have changed");
  previousValue = element.value;

  otherKeys.forEach(function(key) {
    synthesizeKey(element, key, {});
    isnot(element.value, previousValue, "value should have changed");
    previousValue = element.value;
  });
});

This code might not work as-is but that is the idea.

Also, because we are using focus, we need the page to be focused so you should have this at the beginning of your script.
SimpleTest.waitForExplicitFinish();
SimpleTest.waitForFocus(function() {
  // your test
});

The callback inside waitForFocus will be called when the window will be focused and waitForExplicitFinish() will require you to call SimpleTest.finish() when you are done.

        Attachment #770833 -
        Flags: review?(mounir) → feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
I made all the changes you asked for and here is an updated patch.

I did actually edit the patch last time. About 1/3 of the files I edit in mozilla-central have trailing whitespace. My editor autocleans those, which gives _me_ whitespace errors in my patches. I got tempted to remove hunks in my patches with only whitespace diffs. Turns out that doesn't work.

        Attachment #207805 -
        Attachment is obsolete: true

        Attachment #261793 -
        Attachment is obsolete: true

        Attachment #564892 -
        Attachment is obsolete: true

        Attachment #770833 -
        Attachment is obsolete: true

        Attachment #771980 -
        Flags: review?(mounir)

    
    

    
  
Comment on attachment 771980 [details] [diff] [review]
patch

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

r=me with my comments applied.

::: layout/forms/nsListControlFrame.cpp
@@ +148,5 @@
> +                                      mEventListener, false);
> +  mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"), 
> +                                      mEventListener, false);
> +  mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"), 
> +                                      mEventListener, false);

nit: you have added trailing whitespaces.

@@ +1002,5 @@
> +                                   mEventListener, false, false);
> +  mContent->AddSystemEventListener(NS_LITERAL_STRING("mouseup"), 
> +                                   mEventListener, false, false);
> +  mContent->AddSystemEventListener(NS_LITERAL_STRING("mousemove"), 
> +                                   mEventListener, false, false);

nit: ditto

@@ +2247,5 @@
>    uint32_t keycode = 0;
>    uint32_t charcode = 0;
>    keyEvent->GetKeyCode(&keycode);
>    keyEvent->GetCharCode(&charcode);
> +  

nit: trailing whitespaces :(

@@ +2249,5 @@
>    keyEvent->GetKeyCode(&keycode);
>    keyEvent->GetCharCode(&charcode);
> +  
> +  // If the event has been prevented and the key is not in a specific set,
> +  // return.

Could you be more explicit about why there is a specific set of keys that are ignored? It would help people understanding what we did here.

What about:
// If the event has been prevent, we should ignore it. However, some keys
// are not ignored because they are required to navigate inside the list.
// Those keys are also not prevented when pressed in most UA so this is also
// a compatibility issue. Basically, keys related to searching into the list
// are ignored.

@@ +2267,5 @@
> +        break;
> +      }
> +      default: {
> +        return NS_OK;
> +      }

nit: you don't need to wrap "break;" and "return NS_OK;" around { and }.

::: layout/forms/test/test_select_prevent_default.html
@@ +15,5 @@
> +
> +  function preventDefault(event) {
> +    // Prevent only '2' and 'down'.
> +    if (event.which == SpecialPowers.Ci.nsIDOMKeyEvent.DOM_VK_2 ||
> +        event.which == SpecialPowers.Ci.nsIDOMKeyEvent.DOM_VK_DOWN) {

nit: I doubt you need to prevent events based on the key anymore.

        Attachment #771980 -
        Flags: review?(neil)

        Attachment #771980 -
        Flags: review?(mounir)

        Attachment #771980 -
        Flags: review+

    
    

    
  
(In reply to Mounir Lamouri (:mounir) from comment #47)
> (In reply to ricky from comment #46)
> > In my webapp, key_left, key_right, page_down, page_up, home and end are the
> > keys I'm trying to prevent the default behavior from happening. The point of
> > preventDefault.
> 
> Are you able to do that with other browsers? Opera, IE, Chrome, Safari?

Chrome Version 27.0.1453.116 does block Home, End, Page up, Page Down, Left and right. So this patch will not be inline with at least Chrome.

    
    

    
  
> nit: you have added trailing whitespaces.

Mounir the trailing whitespace in mozilla-central is a huge pain. About 1/3 of the files in mozilla-central already have trailing whitespace. If I set up my editor to automatically cleanup trailing whitespace, then I get whitespace diffs in files I edit in my patch, because my editor has cleaned them up. If I don't set up auto cleanup of whitespace, then I add trailing whitespace, because it's really hard to track down the whitespaces in the lines I add. Either way I get a comment like this in my review.

What am I supposed to do about this?

    
    

    
  
I recommend disabling the option in your editor, and either turning on whitespace display in your editor or in your hg diff view. I think enabling the color extension does the latter automatically.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
But leaving the trailing whitespace in there would just frustrate the next noob that works with this file. Me cleaning the trailing whitespace should be looked at as a good thing. Besides I don't want to turn off auto cleaning trailing whitespace in my editor. I'd like to have that for other projects. I had to figure out how to make my vim turn off the feature for files with */mozilla-central/* in their path and not for other ones.

Mounir,

Here is a patch with the changes you requested.

        Attachment #771980 -
        Attachment is obsolete: true

        Attachment #771980 -
        Flags: review?(neil)

        Attachment #772176 -
        Flags: review?(mounir)

    
    

    
  
Comment on attachment 772176 [details] [diff] [review]
patch

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

::: layout/forms/nsListControlFrame.cpp
@@ +1906,5 @@
>  
> +  // If the event has been prevent, we should ignore it. However, some keys are not
> +  // ignored because they are required to navigate inside the list. Those keys are
> +  // also not prevented when pressed in most UA so this is also a compatibility
> +  // issue. Basically, keys related to searching into the list are ignored.

Why did you change that comment instead of the other one?

Also, isn't it "If the event has been prevented."?

        Attachment #772176 -
        Flags: review?(mounir)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
I'm sincerely sorry for that. Silly mistake. Here is a patch with the correct comment.

        Attachment #772176 -
        Attachment is obsolete: true

        Attachment #772725 -
        Flags: review?(mounir)

    
    

    
  
Comment on attachment 772725 [details] [diff] [review]
patch

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

r=me but I would like Neil to have a look at that to see if he agrees with the solution given that he is mentoring this bug.

        Attachment #772725 -
        Flags: review?(neil)

        Attachment #772725 -
        Flags: review?(mounir)

        Attachment #772725 -
        Flags: review+

    
    

    
  
Neil,

Just a friendly reminder this patch is waiting for a review from you. Thanks!

    
    

    
  
Comment on attachment 772725 [details] [diff] [review]
patch

Sorry for taking so long to get back to this.

>+  // If the event has been prevented, we should ignore it. However, some keys
>+  // are not ignored because they are required to navigate inside the list.
>+  // Those keys are also not prevented when pressed in most UA so this is also
>+  // a compatibility issue. Basically, keys related to searching into the list
>+  // are ignored.
Fair enough, but in this case, why not move the defaultPrevented check to the block relating to searching into the list (which is about 120 lines down from here)?

    
    

    
  
(In reply to neil@parkwaycc.co.uk from comment #60)
> Comment on attachment 772725 [details] [diff] [review]
> patch
> 
> Sorry for taking so long to get back to this.
> 
> >+  // If the event has been prevented, we should ignore it. However, some keys
> >+  // are not ignored because they are required to navigate inside the list.
> >+  // Those keys are also not prevented when pressed in most UA so this is also
> >+  // a compatibility issue. Basically, keys related to searching into the list
> >+  // are ignored.
> Fair enough, but in this case, why not move the defaultPrevented check to
> the block relating to searching into the list (which is about 120 lines down
> from here)?

You mean in this massive switch statement? 

http://dxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#l2300

I'd rather not do that. I feel like as is it's much easier to read, and less code is duplicated |if (defaultPrevented) { return NS_OK; }|

    
    

    
  
(but I'm happy to do it, if you'd like)

    
    

    
  
(In reply to Mina Almasry from comment #61)
> (In reply to comment #60)
> > (From update of attachment 772725 [details] [diff] [review])
> > >+  // If the event has been prevented, we should ignore it. However, some keys
> > >+  // are not ignored because they are required to navigate inside the list.
> > >+  // Those keys are also not prevented when pressed in most UA so this is also
> > >+  // a compatibility issue. Basically, keys related to searching into the list
> > >+  // are ignored.
> > Fair enough, but in this case, why not move the defaultPrevented check to
> > the block relating to searching into the list (which is about 120 lines down
> > from here)?
> 
> You mean in this massive switch statement? 
> 
> http://dxr.mozilla.org/mozilla-central/source/layout/forms/
> nsListControlFrame.cpp#l2300

The keys in this switch are the ones you don't want to block, so listing them twice is just duplication of effort; instead you could perform the check 75 lines further down, where the actual search happens.

    
    

    
  
> The keys in this switch are the ones you don't want to block, so listing
> them twice is just duplication of effort; instead you could perform the
> check 75 lines further down, where the actual search happens.

Aha my bad I see what you mean. But the list I'm skipping 75 lines down also includes TAB, ESCAPE, RETURN and F4. So do I duplicate for those keys, or are those keys also keys we don't want prevented?

    
  
Flags: needinfo?(mounir)

    
    

    
  
I think Mounir's answer would be to do what other browsers do. In chrome, with all the keys prevented (note they aren't in the attached test case), ENTER and ESCAPE work, but TAB doesn't. I haven't tested on chrome so I have no idea what F4 does.

    
    

    
  
Sorry haven't tested on Windows*

    
    

    
  
(In reply to Mina Almasry from comment #64)
> > The keys in this switch are the ones you don't want to block, so listing
> > them twice is just duplication of effort; instead you could perform the
> > check 75 lines further down, where the actual search happens.
> 
> Aha my bad I see what you mean. But the list I'm skipping 75 lines down also
> includes TAB, ESCAPE, RETURN and F4. So do I duplicate for those keys, or
> are those keys also keys we don't want prevented?

Okay to ignore prevent default on the keys listed above.
Flags: needinfo?(mounir)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        prevent-default.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Here is a patch with the change. I wasn't sure if to ask a review from mounir, or neil. I asked from neil because mounir's last review was granted. Feel free to change if wrong. Thanks!

        Attachment #772725 -
        Attachment is obsolete: true

        Attachment #772725 -
        Flags: review?(neil)

        Attachment #778198 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 778198 [details] [diff] [review]
prevent-default.patch

>     default: { // Select option with this as the first character
>                // XXX Not I18N compliant
>-      
>+
>+       // We skip processing all key events in the keys that are not in the
>+       // cases above, if they have been prevented. The keys listed in the cases
>+       // above are required to navigate inside the list. These keys are also
>+       // not prevented in most UAs, so this is also a compatibility issue.
This comment no longer makes sense in its new position, would you mind adjusting it?

>+       bool defaultPrevented;
>+       aKeyEvent->GetDefaultPrevented(&defaultPrevented);
>+       if (defaultPrevented) {
>+         return NS_OK;
>+       }
>+
>       if (isControl && charcode != ' ') {
>         return NS_OK;
>       }
You appear to have one too many spaces of indentation here.

        Attachment #778198 -
        Flags: review?(neil) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
Indentation fixed, sorry didn't notice that myself.

I don't mind changing the comment, of course, but it has been changed to make sense in the new position, and as far as I can tell it reads fine (by "the cases above", I mean the cases above default). If you still want it changed, can you tell me to what? I'm not sure what to change it to.

Asking for another review for the comment.

        Attachment #778198 -
        Attachment is obsolete: true

        Attachment #778457 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 778457 [details] [diff] [review]
patch

Looks good now, thanks!

        Attachment #778457 -
        Flags: review?(neil) → review+

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/952442f77c13
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

    
    

    
  
It seems that bug occurs again, I can change <select> value by pressing arrow keys in every test case from this attachment: https://bugzilla.mozilla.org/attachment.cgi?id=648806

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0

    
    

    
  
Karol, that's actually the intended behavior.  See comment 29 / 30 above.
The test explicitly checks that behavior:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/test/test_select_prevent_default.html?force=1

There's a code comment that says other browsers don't block arrow keys:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#2087
but that's not true anymore.  At least Chrome seems to block all keys,
even tabbing(!), when preventDefault() is called in onkeydown.

(I'm working on related code in bug 1153586, so I'll add an update to the code
comment in that patch.)

    
    

    
  
Shouldn't this bug be reopened ?
From comment 41 and comment 47, I understand that the intended behavior should be consistent with the other browsers.
Today, all major browsers will cancel arrow keys and page up / page down when the keydown is "preventDefaulted".
This is not the case in Firefox, because of this specific patch. 
So shouldn't this behavior be reversed ?

    
    

    
  
In Firefox, if the select dropdown is closed and I press the spacebar, the dropdown opens even if I call preventDefault of the keydown event. In other browsers (Chrome, Opera, IE11), the dropdown does not open. The behavior in Firefox is a real problem for me.
Component: Keyboard: Navigation → 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: