GNOME Bugzilla – Bug 706386
System-status slider values cannot be modified via the keyboard
Last modified: 2013-08-22 14:31:36 UTC
Steps to reproduce: 1. Get into the new system-status menu 2. Arrow to an item with a slider (e.g. volume, brightness) 3. Attempt to change the value using the keyboard Expected results: Something (ideally left/right arrow) would change the value of the slider. Actual results: The keys tried (left/right unmodified and modified, home/end, page up/page down) did not change the value. In addition, pressing left arrow caused my calendar menu to open whilst leaving the system-status menu open as well.
FWIW, this is a regression, at least on the volume slider (brightness and others are new sliders). On previous releases (e.g. 3.8) you can navigate to the volume slider and use left-right to change the volume value.
Created attachment 252619 [details] [review] WIP: Fix the key navigation The problem was that the code that manage modifying the slider value based on the left-right key arrows needs the slider to get the focus (as it is obviously based on ClutterActor::key-press-event). Although the slider has can_focus to true, it didn't receive the focus as the parent container (popupbasemenuitem) also had it. So this patch sets can_focus to false on the menu items that contains the slider, and also fix a bug on Slider:_onKeyPressEvent. *But*, and the reason it is still a WIP: doing just what I said on previous paragraph has the problem that removes the hightlighting. So I initially added a similar highlighting code to the slider and added a active style to the css. But doing that is also wrong, as just highlights the slider, when before all the container (so also the space with the icon at the left) was highlighted. So Im putting here this tentative patch, so anyone with more experience on all the style stuff could give me a hint (Jasper?). FWIW, I still think that giving the focus to the slider is the right thing to do, because after all is the element the user is interacting with.
Review of attachment 252619 [details] [review]: Not a giant fan of the patch; I'd rather that we relay key press events on the focused item to the slider underneath like we do with startDragging. ::: js/ui/slider.js @@ +177,3 @@ let delta = key == Clutter.KEY_Right ? 0.1 : -0.1; this._value = Math.max(0, Math.min(this._value + delta, 1)); + this.actor.queue_repaint(); Please put this in a separate patch. @@ +215,3 @@ + + setActive: function (active) { + if (active) You shouldn't need this; we already have a :focus style added by StWidget.
Created attachment 252716 [details] [review] Using the correct variable name > ::: js/ui/slider.js > @@ +177,3 @@ > let delta = key == Clutter.KEY_Right ? 0.1 : -0.1; > this._value = Math.max(0, Math.min(this._value + delta, 1)); > + this.actor.queue_repaint(); > > Please put this in a separate patch. Done (this patch)
Created attachment 252717 [details] [review] Fix key navigation but delegating on the slider (not giving focus) (In reply to comment #3) > Review of attachment 252619 [details] [review]: > > Not a giant fan of the patch; I'd rather that we relay key press events on the > focused item to the slider underneath like we do with startDragging. I also thought on that. But that has two (in my opinion) problems. 1) The first is that you need somehow to disable the code that move between menus when you press right-left key arrows, because if not, while changing the value of the slider you will move to a different menu. In other words, avoid PanelMenuButton to connect to the menu key-press at setMenu. This patch does that, by adding a 'allowMenuChange' boolean at the menu. It works, but seems somewhat hacky to me. 2) As I mentioned, as from the user pov, he is interacting with the slider, for me logically the focus should be at the slider. > @@ +215,3 @@ > + > + setActive: function (active) { > + if (active) > > You shouldn't need this; we already have a :focus style added by StWidget. Well, the fact is that without my change, the slider didn't have any highlighting at all. I assumed that was because it was missing. But taking into account your comment, probably it is being overriden somewhere. I will take a look.
Review of attachment 252716 [details] [review]: OK.
(In reply to comment #5) > I also thought on that. But that has two (in my opinion) problems. > > 1) The first is that you need somehow to disable the code that move between > menus when you press right-left key arrows, because if not, while changing the > value of the slider you will move to a different menu. In other words, avoid > PanelMenuButton to connect to the menu key-press at setMenu. This patch does > that, by adding a 'allowMenuChange' boolean at the menu. It works, but seems > somewhat hacky to me. I don't see why. Events in Clutter bubble -- they go to the key focus first (the menu item), and then bubble up if they're not handled. If you just change your code to "return this._slider.onKeyPressEvent(...);", then we shouldn't need the "allowMenuChange". > 2) As I mentioned, as from the user pov, he is interacting with the slider, for > me logically the focus should be at the slider. The menu item has the highlight, not the slider. Putting interactive controls inside other interactive controls has always been a bit confusing about what has key focus, but the same is true for a switch menu item too -- the focus is on the menu item, not the switch.
Review of attachment 252717 [details] [review]: marking reviewed
Created attachment 252739 [details] [review] Fix key navigation delegating on the slider (not giving focus) (In reply to comment #7) > I don't see why. Events in Clutter bubble -- they go to the key focus first > (the menu item), and then bubble up if they're not handled. If you just change > your code to "return this._slider.onKeyPressEvent(...);", then we shouldn't > need the "allowMenuChange". Oh, really smart. Thanks for the tip. This updated patch does that, making the patch simpler, and works. As that can be non-trivial, I added a comment pointing to this bug. Anyway, if consiedered superfluous, I can remove it on the commit. > > 2) As I mentioned, as from the user pov, he is interacting with the slider, for > > me logically the focus should be at the slider. > > The menu item has the highlight, not the slider. Putting interactive controls > inside other interactive controls has always been a bit confusing about what > has key focus, but the same is true for a switch menu item too -- the focus is > on the menu item, not the switch. Yes, it is true that on those cases it is always debatable who should have the key focus. Probably I was biased because I was thinking on the accessibility implementation at the AT. "Element hightlighted should be the one with the focus" also looks sensible to me, and it doesn't make sense to discuss too much about that.
Review of attachment 252739 [details] [review]: Anybody familiar with Clutter should know about stopping bubbling propagation, so I think the comment is a bit superfluous. Otherwise, looks good.
(In reply to comment #10) > Review of attachment 252739 [details] [review]: > > Anybody familiar with Clutter should know about stopping bubbling propagation, > so I think the comment is a bit superfluous. Otherwise, looks good. Ok, just committed without the comment. Closing bug