Many keyboard shortcuts don't work when focus is on a <select> (listbox or drop-down)

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
18 years ago
2 months ago

People

(Reporter: jonasj, Assigned: john)

Tracking

({testcase})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

18 years ago
When a <select> has focus, many keyboard shortcuts doesn't work.

Steps to reproduce:
* Open attached testcase
* Select one of the <select>s
* Press Ctrl + W, Ctrl + N, Ctrl + T, Ctrl + 2, etc.
Reporter

Comment 1

18 years ago
Posted file testcase
Reporter

Updated

18 years ago
Keywords: testcase
Reporter

Comment 2

18 years ago
Forgot to mention: This is build 2002011103.

*** This bug has been marked as a duplicate of 72352 ***
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → DUPLICATE
Reporter

Comment 4

18 years ago
I don't think this is a dup. Bug 72352 seems to be about being able to turn off
emacs keybindings on Linux/Unix and being able to turn them on on other
platforms. The reporter of bug 72352 mentions not being able to close window by
pressing Ctrl+W when focus is an a textfield, but that works fine for me here on
Win2k. I'm talking about the <select> element - list boxes and dropdown lists.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
bryner?
Assignee: aaronl → bryner
Status: REOPENED → NEW
Reporter

Comment 6

18 years ago
See also bug 123008, "Many keyboard shortcuts doesn't work after switching
themes from View menu".
Reporter

Comment 7

18 years ago
[fixing my stupid grammatical error in summary. sorry for the spam.]
Summary: Many keyboard shortcuts doesn't work when focus is on a <select> → Many keyboard shortcuts don't work when focus is on a <select>
-> 1.0, nominating for beta1.
Status: NEW → ASSIGNED
Keywords: nsbeta1
Target Milestone: --- → mozilla1.0

Comment 9

18 years ago
*** Bug 128313 has been marked as a duplicate of this bug. ***

Updated

18 years ago
Summary: Many keyboard shortcuts don't work when focus is on a <select> → Many keyboard shortcuts don't work when focus is on a <select> (listbox or drop-down)

Comment 10

18 years ago
nsbeta1- per ADT triage team
Keywords: nsbeta1nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.2

Comment 11

17 years ago
Potentially related to bug 128995.

Comment 12

17 years ago
Actually, forget I said that.  I can't reproduce this using 2002032003 and XBL
form controls.

Comment 13

17 years ago
*** Bug 122989 has been marked as a duplicate of this bug. ***

Comment 14

17 years ago
This problem is more general and not specific to Windows, see the dupe. 
Updating summary and OS accordingly.
OS: Windows 2000 → All
Summary: Many keyboard shortcuts don't work when focus is on a <select> (listbox or drop-down) → form controls block keyboard shortcuts

Comment 15

17 years ago
Ctrl+N wfm in a textarea.  I think that's a Linux-specific problem.  *No*
shortcuts work for me in <select> elements (Ctrl+N, Ctrl+T, etc) and I've run
into this bug several times.
Summary: form controls block keyboard shortcuts → Many keyboard shortcuts don't work when focus is on a <select> (listbox or drop-down)

Comment 16

17 years ago
Jesse: on what OS?  Shortcuts work fine for me in <select>s using 2002033009 on
Win2K.
Reporter

Comment 17

17 years ago
Well, they don't work here -- Win2k, build 2002-03-26-03. Are you sure your
focus is on a <select> element, e.g. the Target Milestone dropdown on this page?

I don't think bug 122989 is a dup of this, btw...

Comment 18

17 years ago
Oh... tee hee.  I forgot that I'm using XBL form controls.  Works fine with
them, though!
Reporter

Comment 19

17 years ago
If this bug doesn't occur with XBL form controls, we can mark it FIXED once
XBLFC is turned on by default. Marking dependent.
Depends on: 57209

Comment 20

17 years ago
We still need this fixed for non-XBL form controls, which are likely to be used
in a variety of important embedding products.

Don't assume bug 57209 will fix this.
Reporter

Comment 21

17 years ago
Removing dependency, then. I was under the impression that the old form controls
would be removed once the XBL ones were turned on.
No longer depends on: 57209

Comment 22

17 years ago
So was I, and more than a couple of bugs have been marked as fixed since they
work using XBLFC.  Which is it?

Comment 23

17 years ago
We need to find those bugs that were marked fixed because of XBLFC and reopen
them, ASAP. XBLFC is not as far enough along as was hoped.

Comment 24

17 years ago
Things like the focus indicator for the active item in a listbox, for example.
Reporter

Comment 25

17 years ago
Aaron: Look at bug 57209's OtherBugsDependingOnThis field. A lot of them should
be removed from that field if non-XBL form controls will continue to exist.

Updated

17 years ago
Blocks: 135206

Updated

17 years ago
Keywords: topembed

Comment 26

17 years ago
marking nsbeta1, adt2. Removing minus since xbl form controls not being turned
on has changed the playing field
Keywords: nsbeta1-, topembednsbeta1
Whiteboard: [adt2]

Updated

17 years ago
Blocks: 75785

Comment 27

17 years ago
Nav triage team: nsbeta1+/adt2
Keywords: nsbeta1nsbeta1+
This should be reassigned if the bug is to be fixed in the current form controls.

Comment 29

17 years ago
rods, for retriage
Assignee: bryner → rods
Status: ASSIGNED → NEW

Comment 30

17 years ago
Tom, is this another manifestation of the event targeting bug?

Comment 31

17 years ago
same as bug 131921?

Comment 32

17 years ago
*** Bug 143431 has been marked as a duplicate of this bug. ***

Comment 33

17 years ago
Isn't this a dup of another bug somebody is working on?
Status: NEW → ASSIGNED
Target Milestone: mozilla1.2alpha → mozilla1.0

Comment 34

17 years ago
Are you thinking of bug 128995?  That's for XBL form controls.
nsbeta1-. engineers are overloaded with higher priority bugs.
Keywords: nsbeta1+nsbeta1-

Updated

17 years ago
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 36

17 years ago
Also the case when focus is on a text or password input.  Mac OS
10.1.5/Mozilla1.1a build ID:2002061014.

Updated

17 years ago
Target Milestone: mozilla1.0.1 → mozilla1.1alpha

Updated

17 years ago
Blocks: atfmeta

Comment 37

17 years ago
-->
Assignee: rods → jkeiser
Status: ASSIGNED → NEW
Target Milestone: mozilla1.1alpha → ---
Assignee

Comment 38

17 years ago
Perhaps we're eating keystrokes and not giving them to other people 
Status: NEW → ASSIGNED
Reporter

Comment 39

17 years ago
> Perhaps we're eating keystrokes and not giving them to other people

Nooooo! Think of the starving children in Africa!

Comment 40

17 years ago
*** Bug 172192 has been marked as a duplicate of this bug. ***

Comment 41

17 years ago
Posted patch patch (obsolete) — Splinter Review
the function PreventBubble() will eat up the "Control" key event,
so skip it if "Control" pressed.

Comment 42

17 years ago
seek r=

Updated

17 years ago
Keywords: nsbeta1-patch, review

Comment 43

17 years ago
Simford, good try! But this bug is not for control key only, it's for all
shortcut keys, such as Ctrl+D, F11. I guess it not only blocks the shortcuts in
UI, but also the accesskey attribute in HTML page.

Comment 44

17 years ago
Due to comment 43, I suggest skip the call PreventBubble().

Comment 45

17 years ago
Posted patch new patch as comment 44 says (obsolete) — Splinter Review
seek r= again.
Attachment #103445 - Attachment is obsolete: true

Comment 46

17 years ago
Works as expected now, Ctrl-t, F11, etch all do their job even when a form
control element is selected.  Nice job.

Comment 47

17 years ago
What about selecting options in the <select> using the first letter of the item?
 Are there any adverse effects from that?

Comment 48

17 years ago
It'll be better that use a flag here to see whether or not we handled the
keyevent, then call PreventBubble() if really handled. Otherwise, you might mess
up the keyboard shortcuts.

Comment 49

17 years ago
Posted patch new patch due to comment 48 (obsolete) — Splinter Review
seek r=

Comment 50

17 years ago
That doesn't seem right to me.  We also need to handle anything for selecting
items by the first letter (eg. using 'W' in the Platform select on this page). 
Can it be done in a better way than hard-coding keys?

Babbling...

What about putting the PreventBubble in the default section of the switch, maybe
by adding an else somewhere around the |if (wasChanged)| check?

Alternatively, maybe after |if (!isControl)| check.  We could add an |isAlt|
flag and have |if (!isControl and !isAlt and !isShift) nsevent->preventBubble|.

Or are these too late in the process?  Is there a specific reason that
preventBubble is called before anything else?  I notice we have code in |#ifdef
FIX_FOR_BUG_62425| section that calls preventBubble and preventDefault separately. 

Comment 51

17 years ago
Posted patch patch (obsolete) — Splinter Review
It is a good idea to put PreventBubble() after |if (wasChanged)|
Hope it works this time
Attachment #103494 - Attachment is obsolete: true
Attachment #103548 - Attachment is obsolete: true

Comment 52

17 years ago
Posted patch better one (obsolete) — Splinter Review
Thanks Kyle!

seek r=

Comment 53

17 years ago
Comment on attachment 103563 [details] [diff] [review]
better one

Index: html/forms/src/nsListControlFrame.cpp
===================================================================
RCS file:
/export/src/cvs/mozilla_1/mozilla/layout/html/forms/src/nsListControlFrame.cpp,
v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 nsListControlFrame.cpp
--- html/forms/src/nsListControlFrame.cpp	2002/10/21 00:17:08	1.1.1.1
+++ html/forms/src/nsListControlFrame.cpp	2002/10/21 10:03:42
@@ -3323,14 +3323,9 @@
     return rv;
   }

-  nsCOMPtr<nsIDOMNSEvent> nsevent(do_QueryInterface(aKeyEvent));
-
-  if (nsevent) {
-    // We are handling this so don't let it bubble up
-
-    nsevent->PreventBubble();
-  }
-
+  // Whether we need PreventBubble or not
+  PRBool needPreventBubble = PR_TRUE;
+  
   // Whether we did an incremental search or another action
   PRBool didIncrementalSearch = PR_FALSE;

@@ -3456,6 +3451,8 @@
	 }
	 return NS_OK;
       }
+      
+      needPreventBubble = PR_FALSE;

       PRUnichar uniChar = ToLowerCase(NS_STATIC_CAST(PRUnichar, charCode));

@@ -3505,13 +3502,25 @@
	       if (wasChanged) {
		 UpdateSelection(); // dispatch event, update combobox, etc.
	       }
+	       needPreventBubble = PR_TRUE;
	       break;
	     }
	   }
	 }
-      } // while
+      } // for
     } break;//case
   } // switch
+
+  //if current key has been processed then we need to call PreventBubble()
+  if (needPreventBubble){
+      nsCOMPtr<nsIDOMNSEvent> nsevent(do_QueryInterface(aKeyEvent));
+
+      if (nsevent) {
+	 // We are handling this so don't let it bubble up
+
+	 nsevent->PreventBubble();
+      }
+  }

   // If we didn't do an incremental search, clear the string
   if (!didIncrementalSearch) {

Updated

17 years ago
Attachment #103562 - Attachment is obsolete: true

Comment 54

17 years ago
jkeiser, could you r=?
Assignee

Comment 55

17 years ago
Comment on attachment 103563 [details] [diff] [review]
better one

Wow, we suck, thanks for catching this.

First of all, we should do preventDefault() instead of preventBubble() to keep
shortcuts from working.    PreventDefault() indicates that the event has been
handled by the browser already, but does not stop it from propagating to user
onKeyPress handlers.

Second, there are *many* cases that need preventDefault() in this method, not
just the A-Z, though I understand why you did this ... could you add a
PreventDefault() to the other cases in this method as well?
Attachment #103563 - Flags: needs-work+

Comment 56

17 years ago
jkeiser, simford uses |needPreventBubble| to check whether we need to call
|preventDefault| and set it to TRUE by default, so it covered the all case in
|switch(keyCode)|, not only the A-Z.
Assignee

Comment 57

17 years ago
Ah, I see, you are right.  It still needs to call PreventDefault() instead of
PreventBubble().  That's the only thing that needs changing, everything else is
good.  All keyboard shortcuts should work properly with that on (they are
supposed to check whether we called PreventDefault() before doing anything). 
This will make it so that onkeypress handlers get called properly.

Comment 58

17 years ago
Posted patch patchSplinter Review
make changes due to comment 57

seek r= & sr=
Attachment #103563 - Attachment is obsolete: true
Assignee

Comment 59

17 years ago
Comment on attachment 104261 [details] [diff] [review]
patch

r=jkeiser
Attachment #104261 - Flags: review+
Comment on attachment 104261 [details] [diff] [review]
patch

sr=bzbarsky
Attachment #104261 - Flags: superreview+

Comment 62

17 years ago
checked into trunk!
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED
vrfy'd fixed with 2002.11.19 comm trunk bits.
Status: RESOLVED → VERIFIED
Hardware: PC → All

Updated

17 years ago
Blocks: 195190
Component: Keyboard: Navigation → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.