GNOME Bugzilla – Bug 643687
GrabHelper: new helper for PopupMenuManager and MessageTray
Last modified: 2012-08-20 01:18:06 UTC
MessageTray was handling focus/grabbing in ways that thwarted adding keynav. For instance, when clicking from one summaryitem to another, certain important parts of the process occur as part of the focusGrabber's 'button-pressed' signal, rather than all occurring in _onSummaryItemClicked, which meant that those things weren't happening when the 'clicked' signal came from a key press rather than a button press. As I started fixing this, I realized that it was a lot like fixes we'd already made to PopupMenuManager, and that PopupMenuManager and FocusGrabber were actually very similar. So I merged them together into a single helper class. GrabHelper does not (currently) try to handle the "keep the summary notification visible when entering the overview" thing that FocusGrabber currently does. For one thing, it was already flaky (I think it only works if the pointer is in the tray?). And even if it wasn't flaky, it's still quite odd; as soon as the user does anything in the overview, the notification would go away, so it's not all that useful. And it might trip up the user, who was expecting to be able to do, eg, "[Super] fi [RETURN]" to launch firefox, and finds that instead he just sent "fi" over chat. Anyway, I could add that functionality back if it was really wanted...
Created attachment 182267 [details] [review] GrabHelper: new helper for PopupMenuManager and MessageTray GrabHelper is a helper class for grabbing the pointer and then ungrabbing it and cleaning up either when the caller requests it, or when the user clicks outside the grabbed area. It replaces all of MessageTray.FocusGrabber and half of PopupMenu.PopupMenuManager, merging various bugfixes from each.
Created attachment 182268 [details] [review] MessageTray: keep notification focused through update() If a notification ws updated while one of its widgets was focused, it would lose the grab when that widget was destroyed. Fix that by moving the focus to a safe place before destroying the old widgets. https://bugzilla.gnome.org/show_bug.cgi?id=630847
Created attachment 182270 [details] [review] GrabHelper: fix up StWidget:hover after a modal grab In a modal grab, enter/leave events will have been suppressed, so fix up the hover state of the widgets under the pointer at the start and end of the grab. https://bugzilla.gnome.org/show_bug.cgi?id=642920
depends on bug 641253
*** Bug 642920 has been marked as a duplicate of this bug. ***
Review of attachment 182267 [details] [review]: There are some behavior issues I noticed. I think they have to do with having to do more than just ungrab when there is a button press outside the notification or when Esc is pressed. E.i. we still need to do what 'button-pressed' and 'escape-pressed' callbacks did, which is unset clicked summary item and escape tray respectively. Behavior issues: 1) Hitting Esc when an expanded or summary notification is showing doesn't remove it, but only ungrabs focus. 2) If you hit Esc when an expanded or summary notification is showing in the overview, it escapes the overview, but the notification stays up until you hover away from it. Hitting Esc in this case should probably just remove the notification. 3) When a summary notification is showing in the overview and you click elsewhere in a place that doesn't take you out of the overview, the summary notification does not pop down, but it should. ::: js/ui/grabHelper.js @@ +33,3 @@ + + addActor: function(actor) { + actor.__grabHelperDestroyId = actor.connect('destroy', Lang.bind(this, function() { this._removeActor(actor); })); it's this.removeActor() @@ +121,3 @@ + global.set_stage_input_mode(Shell.StageInputMode.FOCUSED); + if (hadFocus && newFocus) + newFocus.grab_key_focus(); Why does hadFocus matter here? @@ +128,3 @@ + if (!metaDisplay.focus_window) { + metaDisplay.set_input_focus_window(this._prevFocusedWindow, + false, global.get_current_time()); Should we do that even if we have newFocus ? @@ +139,3 @@ + let type = event.type(); + let button = (type == Clutter.EventType.BUTTON_PRESS || + type == Clutter.EventType.BUTTON_RELEASE); Maybe name it buttonTriggered or buttonEvent ? ::: js/ui/messageTray.js @@ -313,3 @@ - let focusedActor = global.stage.get_key_focus(); - if (focusedActor && this.actor.contains(focusedActor)) - global.stage.set_key_focus(null); Do we not want anything like this in the new grabHelper?
Review of attachment 182268 [details] [review]: s/ws/was ; bug link appears twice Good to commit with the commit message fixed up.
Review of attachment 182268 [details] [review]: Nevermind about the bug link - they are two different bugs :).
Review of attachment 182270 [details] [review]: ::: js/ui/grabHelper.js @@ +123,3 @@ + this._syncHover(this._preGrabPointerActor, false); + if (postGrabPointerActor) + this._syncHover(postGrabPointerActor, true); It seems that st_widget_sync_hover() figures out and updates a hover property for a given widget, while this._syncHover() sets a given hover value for a given widget and its parents. Should we perhaps add some version of this._syncHover() to StWidget? @@ +187,3 @@ + }, + + _getPointerActor: function() { _getPointerActor() and _syncHover() should be static functions. @@ +194,3 @@ + let device = event.get_device(); + if (device.get_device_type() == Clutter.InputDeviceType.POINTER_DEVICE) + pointer = device; st_widget_sync_hover() doesn't try to figure out the pointer that way. Should it be added there? @@ +212,3 @@ + if (actor.track_hover) + actor.hover = hover; + actor = actor.get_parent(); This won't set the hover right for all actors if hover is false. Hover value for a parent might be true, even if it s false for a child. It works out with your code changes though since you call this._syncHover(postGrabPointerActor, true) last. I think you should either 1) have this function call st_widget_sync_hover() on parents if passed in value is false until st_widget_sync_hover() sets true for the actor (after which you can set true for the remaining ancestors). OR 2) replace it with a function _moveHover(oldActor, newActor) which updates hover value of oldActor and its ancestors to be false, but then, if necessary, corrects it when setting hover value to true on newActor and its ancestors.
Review of attachment 182268 [details] [review]: Just tested this patch applied alone and it takes away focus from the chat window when you send a message. So we can't commit it as is. I don't know if there is another work-around for this. If necessary, there is also this patch that I made to fix this problem: https://bugzilla.gnome.org/show_bug.cgi?id=630847#c15
Created attachment 184128 [details] [review] MessageTray: keep notification focused through update() fix to not mess up chats
Review of attachment 184128 [details] [review]: Looks good and works at least for RB and chat.
Created attachment 187076 [details] [review] GrabHelper: new helper for PopupMenuManager and MessageTray Updated version. Seems to behave well. This depends on bug 648788 (which simplifies PopupMenuManager a bit, which this patch depends on), and bug 647706 (which in particular makes it so that if you click on a menu when a summarynotification is up, the menu pops up correctly).
Review of attachment 187076 [details] [review]: Since we now have nested menu modals (combobox), this would require significant effort to work recursively, and in a lot of ways would be duplicating pushModal with an explicit stack.
Unless somebody else has a magical idea, (or fixes the message tray to work well with a stack-like approach) it's not going to happen.