Apologies for spam-- marking AP4 to reflect accessibility impact.

Apologies for spam... ensuring Sun a11y folks are cc'ed on all current accessibility bugs.

Ok, after reading all(?) related mails and bugs, it seems we need
some meduim-complex solution here in order to make everybody
happy:

- bug #70986 asks for a beep when keynav fails.
- bug #318827, bug #334726 and bug #334742 ask for simply
  leaving the widget when keynav fails.

IMHO this bug and the related mail mentioned in the original
report offer the best solution to the problem.

I propose to do the following:

1. add a new signal:

   gboolean GtkWidget::keynav_failed (GtkWidget*,GtkDirectionType)

   where the return value has the same meaning as the return
   value of GtkWidget::focus(), namely:

   TRUE == stay in the widget, the key event is handled
   FALSE == leave the widget

2. add a setting that specifies that we have only cursor keys
   (see bug #334742, e.g. "gtk-cursor-only-focus")

3. add a default handler which looks at the setting and either
   beep()s and returns TRUE, or returns FALSE so the widget
   can return FALSE from GtkWidget::focus() and keynav continues
   in the parent container.

4. in all widgets which currently stop navigating, look at the
   return value of gtk_widget_keynav_failed() instead in order
   to decide what to do.

5. add special code to some widgets like GtkRadioButton which
   must modify their behavior if there are only cursor keys
   (see bug #334742 again).

Would this be an acceptable solution? And would it work for all
keynav problems people have in e.g. phone environments or on
the Nokia 770?

It would be good to get a comment on it from some phone people (maemo?), 
and from the a11y guys, but the approach sounds very reasonable to
me. 

Created attachment 65245 [details] [review]
Patch implementing above infrastructure plus two widgets using it

The patch nicely shows a case where my proposal doesn't really work.
Look at the change in gtkrange.c, intra-widget keynav done by bindings
is somewhat tricky...

based on the solution from Michael, I added the following patch to gtkentry.c to enable the same behaviour desired by Markku:

--- gtk/gtkentry.c      2005-08-18 22:10:57.000000000 +0800
+++ gtk/gtkentry.c      2006-05-29 14:42:55.980731414 +0800
@@ -2386,6 +2386,13 @@ gtk_entry_move_cursor (GtkEntry       *e
          break;
        case GTK_MOVEMENT_VISUAL_POSITIONS:
          new_pos = gtk_entry_move_visually (entry, new_pos, count);
+         if (entry->current_pos == new_pos && !extend_selection)
+         {
+           if (!gtk_widget_keynav_failed (GTK_WIDGET(entry), count > 0 ? GTK_DIR_RIGHT : GTK_DIR_LEFT))
+           {
+               gtk_widget_child_focus (gtk_widget_get_toplevel (GTK_WIDGET(entry)), count > 0 ? GTK_DIR_TAB_FORWARD : GTK_DIR_TAB_BACKWARD);
+           }
+         }
          break;
        case GTK_MOVEMENT_WORDS:
          while (count > 0)

Created attachment 72374 [details] [review]
Updated patch

After some discussion with Tim, we came to the conclusion that the
beeping needs a separate setting. Attached patch factors it out to
a new GtkWidget function so it can be used from places where keynav
fails and which are not affected by the gtk-cursor-only-focus setting
(like when wrap-around in menus is disabled).

Created attachment 73147 [details] [review]
New, combined patch

This new patch addresses several keynav related issues because they are
all related:

- Keynav with cursor keys only
- wrap-around
- Beeping on keynav failures

It introduces 3 new settings:

- gtk-keynav-cursor-only
- gtk-keynav-wrap-around
- gtk-keynav-failed-beep

Related bugs, additionally to the ones listed in comment #3 are
#309291 and #322640

Some initial comments from reading the patch:

- Could the value for GtkSettings:gtk-keynav-cursor-only 
  be found by looking at GdkKeymap ?

- GtkSettings:gtk-keynav-failed-beep - the docs should perhaps
  mention that "beep" may be interpreted by the system, and e.g.
  displayed visually.

- GtkSettings:gtk-keynav-wrap-around - if we had an overview
  section over keynav in the docs, it should probably contain
  a list of widgets affected by this...

- keynav_failed is a slight misnomer, considering you are using
  it for failed mouse actions too.

- I think the names of the new GtkWidget functions are
  somewhat confusing. When am I supposed to use 
  gtk_widget_keynav_failed, and when gtk_widget_notify_keynav_failed ?
  Maybe docs would help...

Other than this, the patch looks like a reasonable approach to me

Created attachment 73561 [details] [review]
Next version

- GtkSettings: s/gtk-keynav-error-beep/gtk-error-bell/
- GtkWidget:   s/gtk_widget_notify_keynav_failed/gtk_widget_error_bell/
- GtkIconView: added beeping
- GtkComboBox: added beeping and gtk-keynav-cursor-only support

Created attachment 74569 [details] [review]
Next one

- applies cleanly again after CVS changes
- adapted to the new combo box bindings
- doesn't disable navigation in the combo box any more since
  it can now be configured arbitrarily

Created attachment 76488 [details] [review]
Get rid of offsets and fix a conflict

gtk_widget_keynav_failed 
gtk_widget_error_bell
GtkWidget::keynav-failed

need api docs, and the docs for the new settings could be a little
more detailed:

"When TRUE, some widgets will wrap around when doing keyboard navigation."

I think it would be good to list the known widgets which are affected
by this: ...such as menus, menu bars and radio groups...



"When TRUE, keyboard navigation and other errors will cause a beep."

would it be more precise to say "other input-related errors" here ?
Might be worth mentioning that the system may be configured to e.g.
display the bell visually.


Also, please use %TRUE in docstrings for consistent doc formatting.

Othat than that (and without trying it), the patch looks good and impressive to me.

Created attachment 76571 [details] [review]
Added docs

(In reply to comment #14)
> Created an attachment (id=76571) [edit]
> Added docs

the patch looks very good to me. other than some IRC comments of mine which already got fixed, i'd just like to suggest getting rid of using up a class method pointer to preserve gtk_reserved_4 (since the need to override this signal in a derived class seems to be pretty seldom).

*** Bug 99650 has been marked as a duplicate of this bug. ***

Created attachment 76644 [details] [review]
Next version with some fixes

- don't use a reserved vtale entry, use _gtk_binding_signal_new() instead
- beep on failed home/end in treeview
- beep on failed column reordering in treeview
- beep on failed search up/down in treeview
- s/TRUE/%TRUE/ in settings
- add missing break in gtk_widget_real_focus()

Created attachment 76647 [details] [review]
Argh, and one more

Treeview serach beeped way too often.

Spurious whitespace change in gtkwidget.h, and I believe we just say
"setting" in the docs, not "settings property", 
Other than that, it looks good to me. 

When you commit this, it might be a good idea to prompt the reporters
of the various keynav bugs that we hope to close with this, and ask
them for some feedback.

Fixed in CVS. Bug reporters, please check your use cases and file
new bugs against the new features if anything doesn't work. It doesn't
make sense to keep 6 bugs open.

2006-11-16  Michael Natterer  <mitch@imendio.com>

	Add new infrastructure for notifications of failed keyboard
	navigation and navigation with restricted set of keys.

	The patch handles configurable beeping, navigating the GUI with
	cursor keys only (as in phone environments), and configurable
	wrap-around. Fixes bugs #322640, #70986, #318827, #334726, #334742
	and #309291.

	* gtk/gtksettings.c: added properties gtk-keynav-cursor-only,
	gtk-keynav-wrap-around and gtk-error-bell.

	* gtk/gtkwidget.[ch]: added new signal "keynav-failed" and public
	API to emit it. Added New function gtk_widget_error_bell() which
	looks at the gtk-error-bell setting and calls gdk_window_beep()
	accordingly.

	* gtk/gtk.symbols: add the new widget symbols.

	* gtk/gtkcellrendereraccel.c
	* gtk/gtkimcontextsimple.c
	* gtk/gtkmenu.c
	* gtk/gtknotebook.c: use gtk_widget_error_bell() or look at the
	gtk-error-bell setting instead of calling gdk_display_beep()
	unconditionally.

	* gtk/gtkcombobox.c
	* gtk/gtkentry.c
	* gtk/gtkiconview.c
	* gtk/gtklabel.c
	* gtk/gtkmenushell.c
	* gtk/gtkspinbutton.c
	* gtk/gtktextview.c
	* gtk/gtktreeview.c: call gtk_widget_error_bell() on failed keynav.

	* gtk/gtkentry.c
	* gtk/gtklabel.c
	* gtk/gtkrange.c
	* gtk/gtktextview.c: consult gtk_widget_keynav_failed() on failed
	cursor navigation and leave the widget if it returns FALSE.

	* gtk/gtkmenushell.c
	* gtk/gtknotebook.c: only wrap around if gtk-keynav-wrap-around
	is TRUE.

	* gtk/gtkradiobutton.c: ask gtk_widget_keynav_failed() to decide
	whether to to wrap-around, and don't select active items on cursor
	navigation if gtk-keynav-cursor-only is TRUE. Should look at
	gtk-keynav-wrap-around too, will look into that.

Made radio buttons honor all relevant settings correctly:

2006-11-16  Michael Natterer  <mitch@imendio.com>

	* gtk/gtkradiobutton.c (gtk_radio_button_focus): don't use
	gtk_widget_keynav_failed(). Instead, look at gtk-keynav-cursor-only
	and gtk-keynav-wrap-around and wrap around, beep or continue outside
	the group manually (bug #322640).

Looks nice, guys :) 

However, did I miss something, or is one still stuck within GtkTreeView when only arrow keys are used? I didn't notice any keynav_failed calls within treeview.

Created attachment 77060 [details] [review]
Add keynav-failed support to treeview

Attached patch seems to work fine, but I'd like to hear Kris' word
on that, since looking at tree_view->private->shift_pressed looks
like a bad hack to me.

Fixed the treeview:

2006-11-24  Michael Natterer  <mitch@imendio.com>

	* gtk/gtktreeview.c (gtk_tree_view_move_cursor_up_down): if we
	can't go up/down, consult gtk_widget_keynav_failed() and leave the
	widget if it returns FALSE (bug #322640).