282002 - Spacebar doesn't drop down <select> popup with focus

Status: RESOLVED FIXED

(Camino Graveyard :: HTML Form Controls, defect, P3)

Description

[Simon Fraser [no longer active]]

If you tab to a popup in an HTML form (i.e. a <select> element), thus giving it
keyboard focus, you should be able to hit the spacebar to drop down the popup,
so that you can navigate it with the arrow keys.

Popups in Cocoa UI behave this way, so I think our form control widget should too.

Comment 1

[Mike Pinkerton (not reading bugmail)]

this doesn't work in firefox either, isn't this a core bug?

Comment 2

[Simon Fraser [no longer active]]

(In reply to comment #1)
> this doesn't work in firefox either, isn't this a core bug?

That's a (serious accessibility) regression. It used to work there.

I don't think it's core because of camino's hacked up popup handling.

Comment 3

[Ryan Cannon]

Attachment: HTML Form with SELECT Menu

Added a test case. Attempting to change this bug to Core, as I have duplicated it in Firefox 2.0 for Windows and Mac.

Comment 4

[Stuart Morgan]

Camino shows its menu through the use of a mousedown handler. There may be a core bug, but this is not it, because it would still be broken in Camino even if it worked in Core.

Comment 5

[froodian (Ian Leue)]

Attachment: Patch v1

Implements a keyDown handler, the same way we're currently doing with mouseDown.

Comment 6

[timeless]

Comment on attachment 260241 [details] [diff] [review]
Patch v1

>Index: src/embedding/CHBrowserView.mm
>@@ -227,27 +228,32 @@
>+    // hookup the key and click listeners for creating our own native menus on <SELECTS>
>     CHClickListener* clickListener = new CHClickListener();
>     if (!clickListener)
>       return nil;
>+    CHKeyListener* keyListener = new CHKeyListener();
>+    if (!keyListener)
>+      return nil;
this leaks clickListener unless I'm missing something about ObjC++.

Comment 7

[Stuart Morgan]

(In reply to comment #6)
> this leaks clickListener unless I'm missing something about ObjC++.

Or about the basics of the memory model used by all of the core code.

It leaks only in an error case that should never happen and where Camino would be very functionally broken.

Comment 8

[Simon Fraser [no longer active]]

Comment on attachment 260241 [details] [diff] [review]
Patch v1

You could use a single object as both the click listener and the key listener.

Comment 9

[froodian (Ian Leue)]

Attachment: Patch v2

Comment 10

[froodian (Ian Leue)]

Attachment: Readable Patch v2 for review

Since using a single object for the click listener and key listener meant renaming CHClickListener, the previous patch is pretty unreadable.  This is a hacked together pseudo-patch so you can see the actual differences.  It won't apply, so don't try, but it should give a good idea of what's going on.

Comment 11

[Stuart Morgan]

Comment on attachment 260764 [details] [diff] [review]
Patch v2

>+nsresult BuildNativePopupMenuForSel(nsIDOMHTMLSelectElement* sel);

ShowNativeMenuForSelect

>+NS_IMETHODIMP
>+CHSelectHandler::KeyDown(nsIDOMEvent* aKeyEvent)

Put this up above with the mouse handler.

r=me with those changes (although I didn't actually test it).

Comment 12

[froodian (Ian Leue)]

Attachment: Patch V2.1

Debitrots, incorporates smorgan's comments.

Smokey, can you make up a project patch for this? (add CHSelectHandler.*, remove CHClickListener.*)

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Essentially you're just renaming that file, right?

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: project patch

This will apply both places :)

However, the big complex hunk of the CHBrowserView change in Ian's patch won't apply on the branch, so I haven't tested this on the branch.  Everything works nicely on the trunk, though; finally, the spacebar works as expected :)

Comment 15

[Mike Pinkerton (not reading bugmail)]

+ * Contributor(s):
+ *   David Hyatt <[e-mail]> (Original Author)

really?

+//#include <Carbon/Carbon.h>
+#include <Cocoa/Cocoa.h>

can you remove the commented out include?

+  // DOM mouse listener interface
+  NS_IMETHOD MouseDown(nsIDOMEvent* aMouseEvent);
+  NS_IMETHOD MouseUp(nsIDOMEvent* aMouseEvent) { return NS_OK; };
...

aren't there macros for these method decls generated by IDL, or does the DOM stuff not have IDL?

+private:
+
+};

remove?

Index: src/embedding/CHSelectHandler.h
===================================================================

can you add a class-level comment on why this class is here? 

+  nsIDOMHTMLSelectElement* mSelectElt;

Should this be a strong reference? Not sure, but seems a bit fragile.

+  // y-flip and subtract the control height to convert to cocoa coords
+  NSRect mainScreenFrame = [mainScreen frame];
+  selectScreenRect.origin.y = NSMaxY(mainScreenFrame) - selectScreenRect.origin.y - selectScreenRect.size.height;

do you still need this on trunk?

Comment 16

[Josh Aas]

Comment on attachment 268629 [details] [diff] [review]
Patch V2.1

+#ifndef __CHSelectHandler_h__

Double underscore at the beginning is generally regarded as reserved for compiler use, better to just not do it. I suggest using the following convention for include guards which has no underscores at the beginning and only one on each side of the ending 'h'.

#ifndef CHSelectHandler_h_

Might as well start getting into a better habit in the Camino code now.

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 268629 [details] [diff] [review]
Patch V2.1

+ * Contributor(s):
+ *   David Hyatt <[e-mail]> (Original Author)
+ *   Ian Leue <[e-mail]>

not sure how this works, aren't you the original author of this file? 

sr=pink

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #17)
> (From update of attachment 268629 [details] [diff] [review])
> + * Contributor(s):
> + *   David Hyatt <[e-mail]> (Original Author)
> + *   Ian Leue <[e-mail]>
> 
> not sure how this works, aren't you the original author of this file? 

I explained to pink in the channel (way back when) that what happened is that CHClickHandler* was getting renamed--it's not strictly a new file or new code--and Ian was adding stuff to it, so this is correct.

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 268649 [details] [diff] [review]
project patch

The project patch here won't work anymore, obviously.

Comment 20

[froodian (Ian Leue)]

Attachment: Patch v2.2

> +  // DOM mouse listener interface
> +  NS_IMETHOD MouseDown(nsIDOMEvent* aMouseEvent);
> +  NS_IMETHOD MouseUp(nsIDOMEvent* aMouseEvent) { return NS_OK; };
> ...
> 
> aren't there macros for these method decls generated by IDL, or does the DOM
> stuff not have IDL?

Nope.

> +  nsIDOMHTMLSelectElement* mSelectElt;
> 
> Should this be a strong reference? Not sure, but seems a bit fragile.

Yes, yes it should.  This is bug 373482, and because it's a bug in pre-existing code, I'm leaving it for that bug (and because it isn't necessarily straight-forward, see bug 373482 comment 6)

> 
> +  // y-flip and subtract the control height to convert to cocoa coords
> +  NSRect mainScreenFrame = [mainScreen frame];
> +  selectScreenRect.origin.y = NSMaxY(mainScreenFrame) -
> selectScreenRect.origin.y - selectScreenRect.size.height;
> 
> do you still need this on trunk?

Yeah.

This addresses all other comments, debitrots, and includes the project patch.  Carrying r/sr from previous patches.

Comment 21

[froodian (Ian Leue)]

Checked in on trunk.

Comment 22

[froodian (Ian Leue)]

Attachment: Possible branch patch

This is a branch version, which is mostly the same (but moved to branch), except for

  // We only care about spaces
  if (keyCode != nsIDOMKeyEvent::DOM_VK_SPACE)
    return NS_ERROR_FAILURE;

On trunk, I'm returning NS_OK in that case, which doesn't work on branch (it won't register any keyevents after the menu is open).  Smorgan said

[9:23pm] <smorgan> Looks like you should (on trunk and branch) be returning PR_FALSE in that case
[9:24pm] <smorgan> Er, what? But other code says: return NS_OK; // means I am NOT consuming event
[9:26pm] <smorgan> Oh, duh, NS_OK = 0 = PR_FALSE
[9:27pm] <smorgan> You'll have to ask someone smarter than I, I'm afraid :(
[9:28pm] <smorgan> Seems like what you are seeing is backwards (should be returning a true value for a handled space, and false for everything else)

but if I return PR_FALSE, it continues to work, which leads me to believe that at least on branch PR_FALSE != NS_OK.  Does anybody know what's up here?

Comment 23

[Nick Kreeger]

(In reply to comment #22)
> Created an attachment (id=278355) [details]
> Possible branch patch
> 
> This is a branch version, which is mostly the same (but moved to branch),
> except for
> 
>   // We only care about spaces
>   if (keyCode != nsIDOMKeyEvent::DOM_VK_SPACE)
>     return NS_ERROR_FAILURE;
> 
> On trunk, I'm returning NS_OK in that case, which doesn't work on branch (it
> won't register any keyevents after the menu is open).  Smorgan said
> 

Josh recently reworked the keyevents, I remember because I sat right next to him when he did it ;-)

Comment 24

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 278355 [details] [diff] [review]
Possible branch patch

rs=pink

Comment 25

[Nick Kreeger]

Attachment: Branch Patch V2

Here is an updated patch (fixes bitrotting from checkins from the other day).

I tried this:
+  // We only care about spaces
+  if (keyCode != nsIDOMKeyEvent::DOM_VK_SPACE)
+    return NS_OK;

And I was able to pull up the menu with the spacebar and move up and down the menu with the arrow keys (plus select and enter worked to pick an element).

Froodian, this was what you were talking about right? You used the terms |NS_ERROR_FAILURE| <-> |PR_FALSE| and |NS_OK| <-> |PR_TRUE| openly, which confused me a bit.

Comment 26

[Nick Kreeger]

Also, if no one objects to this action now, I'd like to remove the inline functions (I guess it's really a portability concern, which isn't an issue with us, but does match Mozilla style) and move them into the implementation file:

+class CHSelectHandler :  public nsIDOMMouseListener,
+                         public nsIDOMKeyListener
+{
+public:
+  CHSelectHandler();
+  virtual ~CHSelectHandler();
+
+  NS_DECL_ISUPPORTS
+
+  // DOM event listener interface.
+  NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent) { return NS_OK; };
+
+  // DOM mouse listener interface
+  NS_IMETHOD MouseDown(nsIDOMEvent* aMouseEvent);
+  NS_IMETHOD MouseUp(nsIDOMEvent* aMouseEvent) { return NS_OK; };
+  NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent) { return NS_OK; };
+  NS_IMETHOD MouseDblClick(nsIDOMEvent* aMouseEvent) { return NS_OK; };
+  NS_IMETHOD MouseOver(nsIDOMEvent* aMouseEvent) { return NS_OK; };
+  NS_IMETHOD MouseOut(nsIDOMEvent* aMouseEvent) { return NS_OK; };
+
+  // DOM key listener interface
+  NS_IMETHOD KeyDown(nsIDOMEvent* aKeyEvent);
+  NS_IMETHOD KeyUp(nsIDOMEvent* aKeyEvent) { return NS_OK; };
+  NS_IMETHOD KeyPress(nsIDOMEvent* aKeyEvent) { return NS_OK; };

If not, I can file a followup bug.

Comment 27

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

With kreeger's patch, I get the following (the first when tabbing to a select, and the second when closing/tabbing away from it):

frame: Text(-1) (0x3bf46b8) style: 0x3bf46f8 :-moz-non-element {}
Wrong parent style context:  style: 0x3bf45c8 :-moz-display-comboboxcontrol-frame {}
should be using:  style: 0x3bfba20 {}

frame: Text(-1) (0x3bf46b8) style: 0x3bf46f8 :-moz-non-element {}
Wrong parent style context:  style: 0x3bf45c8 :-moz-display-comboboxcontrol-frame {}
should be using:  style: 0x3bfba20 {}

Otherwise, arrows and space/return worked for me in the popped-up select.

Comment 28

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Actually, I see that crap without the patch, so it's extraneous.

Comment 29

[Nick Kreeger]

Checked into the MOZILLA_1_8_BRANCH.