GNOME Bugzilla – Bug 683529
Can keynav to invisible indicators, again
Last modified: 2012-09-10 19:25:18 UTC
It seems that we broke keynav for so-called invisible indicators. By clicking on the a11y menu, and pressing the right arrow key, I get shoved to the keyboard layout menu that doesn't have an indicator.
PanelMenuManager looks for actor.visible to check if keynav is possible, not actor.mapped, so it doesn't consider container.visible... Patch coming.
Created attachment 223700 [details] [review] St: don't focus hidden actors If an actors is not mapped (visible and all parents visible), then don't allow navigating focus to it. This fixes a regression in the keyboard navigation of the panel with invisibile items. A long time ago (in a galaxy far, far away) the code handling keynav was moved down to St, but I didn't remember it, so disregard the previous comment.
Review of attachment 223700 [details] [review]: Couldn't you just change get_focus_chain to return MAPPED instead?
Created attachment 223702 [details] Bug Here is what I get if I just change real_get_focus_chain
I'm just curious what was keeping us from focusing invisible actors before.
(In reply to comment #5) > I'm just curious what was keeping us from focusing invisible actors before. get_focus_chain filters on VISIBLE actors.
(In reply to comment #6) > (In reply to comment #5) > > I'm just curious what was keeping us from focusing invisible actors before. > > get_focus_chain filters on VISIBLE actors. Right, so why doesn't changing that to MAPPED have the same effect?
I honestly have no idea. (But I must say, I f. hate the tons of intricated and constantly breaking tons of focus handling code in the shell...)
(In reply to comment #8) > I honestly have no idea. > > (But I must say, I f. hate the tons of intricated and constantly breaking tons > of focus handling code in the shell...) So do I. But the way to fix it and make it less intricate is to stop saying "I have no idea, but please review this patch that fixes it somehow", and start debugging.
Ok, so how it worked before: st_widget_get_focus_chain would filter invisible actors, so they would never end up in the focus chain. Why it's broken: Each indicator's container is visible according to session mode, but the contents may be invisible. The container is in the focus chain, but it's not can_focus, so the panelRight box layout attempts giving focus to container, which forwards it to actor, that happily accepts. This in turn triggers PanelMenuManager into showing the associated menu. Why mapped has no effect: The focus chain filters only immediate children of the actor navigating focus (the right panel box in this case), and those actors are visible and mapped. Except they have no visible or focusable contents. Why mapped is worse than current situation (see above screenshot): I did not investigate this through, but I would bet on a stale allocation of the actor, as StBin does not allocate hidden actors. The right fix: Remove filtering code from get_focus_chain and block st_widget_real_navigate_focus (as well as the other implementations, in the grab_key_focus path) from setting focus to hidden actors. Patch coming.
Created attachment 223939 [details] [review] St: don't focus hidden actors If an actors is not mapped (visible and all parents visible), then don't allow navigating focus to it. This fixes a regression in the keyboard navigation of the panel with invisibile items.
Review of attachment 223939 [details] [review]: Not the biggest fan of this, but it works.
Attachment 223939 [details] pushed as 09e3aed - St: don't focus hidden actors