Bug 18363 - [GTK] Combo boxes cannot be opened pressing space
Summary: [GTK] Combo boxes cannot be opened pressing space
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P3 Normal
Assignee: Jan Alonzo
URL:
Keywords: Gtk
: 26818 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-04-08 06:57 PDT by Marco Barisione
Modified: 2009-07-17 20:29 PDT (History)
5 users (show)

See Also:


Attachments
Take away space out of ARROW_KEYS_POP_MENU and into SPACE_POP_MENU for platforms that display the popup when spacebar is pressed (1.92 KB, patch)
2009-07-10 15:52 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
addressed eric's feedback and added a manual test (2.55 KB, patch)
2009-07-13 01:28 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
updated: return key should popup the menu list too for gtk (3.13 KB, patch)
2009-07-17 15:52 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
Make it if-elif-else-endif as suggested by Holger (3.28 KB, patch)
2009-07-17 19:25 PDT, Jan Alonzo
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marco Barisione 2008-04-08 06:57:53 PDT
In GTK combo boxes can be opened by pressing space when they are focused. I tried to implement this behaviour in WebKit but I got lost while debugging keyboard events handling :(
Comment 1 Jan Alonzo 2009-06-30 04:10:06 PDT
*** Bug 26818 has been marked as a duplicate of this bug. ***
Comment 2 Jan Alonzo 2009-07-10 15:52:39 PDT
Created attachment 32586 [details]
Take away space out of ARROW_KEYS_POP_MENU and into SPACE_POP_MENU for platforms that display the popup when spacebar is pressed

The patch takes away space (' ') out of ARROW_KEYS_POP_MENU and into SPACE_POP_MENU for platforms that want to display a popup menu when spacebar is pressed.

No change in behaviour except for Gtk.
Comment 3 Eric Seidel (no email) 2009-07-10 23:08:01 PDT
Comment on attachment 32586 [details]
Take away space out of ARROW_KEYS_POP_MENU and into SPACE_POP_MENU for platforms that display the popup when spacebar is pressed

This looks like it will break mac.  You seem to want #if and #endif then another #if, not #elif

This also needs a manual test.
Comment 4 Jan Alonzo 2009-07-13 01:28:27 PDT
Created attachment 32652 [details]
addressed eric's feedback and added a manual test
Comment 5 Gustavo Noronha (kov) 2009-07-14 14:34:28 PDT
Comment on attachment 32652 [details]
addressed eric's feedback and added a manual test

Looks good to me, after the changes. While you're at it, you could also have enter also open the combo box, to match what GTK+ does. Should be a matter of adding || keyCode == 'r' to the if inside SPACE_POP_MENU. Make it SPACE_AND_RETURN_POP_MENU =P.
Comment 6 Adam Barth 2009-07-17 00:22:42 PDT
Will land.
Comment 7 Adam Barth 2009-07-17 00:28:47 PDT
On second thought, I probably should go to sleep...
Comment 8 Jan Alonzo 2009-07-17 15:52:16 PDT
Created attachment 32984 [details]
updated: return key should popup the menu list too for gtk
Comment 9 Holger Freyther 2009-07-17 18:17:06 PDT
(In reply to comment #8)
> Created an attachment (id=32984) [details]
> updated: return key should popup the menu list too for gtk

Now a ' ' will show the popup twice?
Comment 10 Holger Freyther 2009-07-17 18:33:13 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Created an attachment (id=32984) [details] [details]
> > updated: return key should popup the menu list too for gtk
> 
> Now a ' ' will show the popup twice?

To be more precise this can happen when both ARROW_KEYS_POP_MENU and SPACE_OR_RETURN_POP_MENU are set. It should be mutual exclusive given that the current code is #if #else #endif and we turn it into #if #endif, #if #else #endif.
Comment 11 Jan Alonzo 2009-07-17 19:25:32 PDT
Created attachment 33001 [details]
Make it if-elif-else-endif as suggested by Holger
Comment 12 Holger Freyther 2009-07-17 19:41:55 PDT
Comment on attachment 33001 [details]
Make it if-elif-else-endif as suggested by Holger

OKay, the next thing boils down to taste. To reduce code duplication one could do

if (
#if SPACE_OR_..
keyCode == ' ' || keyCode == '\r'
#elif
keyCode == ' '
#endif
) {
  common Code
}
Comment 13 Jan Alonzo 2009-07-17 20:29:34 PDT
(In reply to comment #12)
> (From update of attachment 33001 [details])

Landed as http://trac.webkit.org/changeset/46081

> OKay, the next thing boils down to taste. To reduce code duplication one could
> do
> 
> if (
> #if SPACE_OR_..
> keyCode == ' ' || keyCode == '\r'
> #elif
> keyCode == ' '
> #endif
> ) {
>   common Code
> }

I'll do this in a separate bug.