GNOME Bugzilla – Bug 707778
The redesigned "Universal Access" and "Date & Time" panels cannot be navigated by the keyboard
Last modified: 2013-09-11 09:45:00 UTC
The summary pretty much says it all. The "(?)" is because I'm testing in my jhbuild environment and am hoping that something in my environment is broken because: * Tab seems to only give focus to the top-most switch * Trying to give focus to the list below it via the mouse and then arrowing doesn't work; it activates the element clicked on
With my release team hat on: setting target to 3.10. Having the accessibility panel inaccessible is enough to justify a hard-code freeze break, so even more to prioritize it when we are still before that freeze.
I just verified this problem is present in F20, so removing the "(?)". In addition, this problem is present for the Date & Time panel as well.
Created attachment 254583 [details] [review] WIP: Sets can_focus to true on several rows This patch sets can_focus on several rows, that make those rows to be focusable, but: 1. On rows with a switch, you need to focus on the row, and then press righ arrow to select it. I guess that the idea was that keep can_focus to false on the row, and can_focus to true on the switch, so the focus will go automatically to the switch. In that case, that's it not working. 2. On rows without a switch (like "zoom off", "screen reader off"), so makes total sense to set can_focus to true, I was not able to activate them with the keyboard (I tried both enter and space). 3. List box rows has a panel role, something that is not really informative. They should be list_item 4. I was not able to get the name of the list box row So, right now, I think that the problem is a mix of not proper can_focus on the builder file, and some bugs on GtkListBox itself
This patch is fine to fix the focus issues. We need the same for the datetime panel, I guess. And then we need a separate bug for making row activation work as expected in all these panels.
filed bug 707853 now for activation problems.
Created attachment 254590 [details] [review] listbox: Set activate_signal on listboxrow class keyboard navigation didn't support activation since we moved the keynav to the child row widgets. We fix this by adding a activate signal handler for the row and setting widget_class->activate_signal to it.
Review of attachment 254590 [details] [review]: Other than that, looks great ::: gtk/gtklistbox.h @@ +93,3 @@ /* Padding for future expansion */ void (*_gtk_reserved1) (void); void (*_gtk_reserved2) (void); sure you don't want to remove padding here ? thats what its for...
Review of attachment 254583 [details] [review]: ok
(In reply to comment #3) > 4. I was not able to get the name of the list box row If you mean the accessible name so that Orca would present it, that was the case with the egg-list-box too. Long term we should see about fixing that properly (i.e. in Gtk). In the meantime, with code freeze nearly here, I just modified Orca's hack for these beasts. With your patch and my updated hack, Orca now presents the items in the UA panel. https://git.gnome.org/browse/orca/commit/?id=9790d71abbcdc00a1fc83fe8a5a4722a852f742a
Comment on attachment 254590 [details] [review] listbox: Set activate_signal on listboxrow class Attachment 254590 [details] pushed as 9038330 - listbox: Set activate_signal on listboxrow class
Review of attachment 254590 [details] [review]: From the description at bug 707853 (probably we should move the discussion there, but as the patch is here), it seems to imply that activating a row will also activate a switch, if the row contain one. After testing this, that is not the case. If the row contains one switch, and the focus is on the row, activating it doesn't do anything on the switch. You need to move the focus to the switch.
(In reply to comment #8) > Review of attachment 254583 [details] [review]: > > ok If the idea (as described on bug 707853) is being able to toggle the switch by activating the row, probably it would be better to modify the patch, and set can_focus to false on the switch, as it would not be needed. Waiting to see the reply to comment 11.
Created attachment 254606 [details] [review] datetime: Set can_focus to true on several rows Equivalent patch for datetime. It is curious, but in this case, activating the row with an switch causes it to be toggled. So I guess that patch at comment 3 is just somehow incomplete.
In cc-datetime-panel.c:list_box_row_activated() it does: if (!g_strcmp0 (widget_name, "auto-datetime-row")) { toggle_switch (W ("network_time_switch")); } I guess the a11y panel needs something similar.
(In reply to comment #14) > In cc-datetime-panel.c:list_box_row_activated() it does: > > if (!g_strcmp0 (widget_name, "auto-datetime-row")) > { > toggle_switch (W ("network_time_switch")); > } > > I guess the a11y panel needs something similar. Ok, thanks. I will work on a patch doing something similar.
Created attachment 254613 [details] [review] toggle child switch if row is activated Based on what datetime is doing as suggested on comment 14. So based that some code (like toggle_switch and define W) is basically C&P. Not sure if it makes sense to put that in a common place anyway. Notes: a) In order to work this needs patch uploaded on comment 6 (or in other words, bug 707853 fixed). b) As I say on the commit message I'm focusing on the main view. I suspect that it would be needed some tweaking on the sub-dialogs, but I prefer to keep things tidy.
Created attachment 254614 [details] [review] Fixing can_focus value on several elements of the main view of uap This patch sets can_focus to true on some list box rows, but also to false on the switchers. Thanks to the previous patch, you can toggle those switchers, so giving the focus to them sounds somewhat useless (debatable in any case). Note: as in previous patch, it is pending a deep review of some of the subdialogues.
(In reply to comment #17) > Note: as in previous patch, it is pending a deep review of some of the > subdialogues. FWIW: just tested the subdialogues and they seem to work fine.
Review of attachment 254613 [details] [review]: ::: panels/universal-access/cc-ua-panel.c @@ +89,3 @@ #define SCROLL_HEIGHT 490 +#define W(x) (GtkWidget*) gtk_builder_get_object (priv->builder, x) cc-ua-panel.c already has a WID() macro that does the same. @@ +363,3 @@ const gchar *dialog_id; + gchar *widget_name; + CcUaPanelPrivate *priv = self->priv; Minor nitpick: the priv variables in other functions are always first in the variable declaration list. @@ +365,3 @@ + CcUaPanelPrivate *priv = self->priv; + + // Check switchs to toggle "switches" @@ +366,3 @@ + + // Check switchs to toggle + widget_name = g_strdup (gtk_buildable_get_name (GTK_BUILDABLE (row))); This isn't freed anywhere. Would be better to change widget_name to be a 'const gchar *' and just avoid the strdup here.
Created attachment 254617 [details] [review] toggle child switch if row is activated (In reply to comment #19) > Review of attachment 254613 [details] [review]: > > ::: panels/universal-access/cc-ua-panel.c > @@ +89,3 @@ > #define SCROLL_HEIGHT 490 > > +#define W(x) (GtkWidget*) gtk_builder_get_object (priv->builder, x) Fixed. As I used datetime as reference, I assumed that that was the name, and took it when I didn't find it at uap. FWIW, most of the panels have a similar macro, using WID as name. datetime is somewhat on his own way. > > cc-ua-panel.c already has a WID() macro that does the same. > > @@ +363,3 @@ > const gchar *dialog_id; > + gchar *widget_name; > + CcUaPanelPrivate *priv = self->priv; > > Minor nitpick: the priv variables in other functions are always first in the > variable declaration list. Fixed. > @@ +365,3 @@ > + CcUaPanelPrivate *priv = self->priv; > + > + // Check switchs to toggle > > "switches" Fixed. > @@ +366,3 @@ > + > + // Check switchs to toggle > + widget_name = g_strdup (gtk_buildable_get_name (GTK_BUILDABLE (row))); > > This isn't freed anywhere. Would be better to change widget_name to be a 'const > gchar *' and just avoid the strdup here. Fixed.
Review of attachment 254606 [details] [review]: Looks good to me.
Review of attachment 254614 [details] [review]: Looks good to me.
Review of attachment 254617 [details] [review]: Looks good now, thanks.