GNOME Bugzilla – Bug 683509
double clicking in search box doesn't select text
Last modified: 2012-09-12 17:16:04 UTC
I expect that double clicking in the search entry would select text. What seems to happen is that it pops up the context menu. Another bug is that pressing escape doesn't cancel the context menu. Another bug is that when the context menu pops up the Paste item is preselected. It should probably not have any item selected.
Thanks for the bug report. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find. *** This bug has been marked as a duplicate of bug 681021 ***
Reopening this. I don't think we can just blame ClutterText for the suckiness of entries in the shell. They just don't work right. In particular the shell search entry is kinda central to the user experience, and it is just crappy. Emmanueles comment in the clutter bug make it pretty clear that we're not going to see a clutter-level fix in the short term (if ever). We need to discuss ways to make this work better in the shell, regardless.
The basic answer is that we can't have long press on a text entry. If you want to just have left/right-click, that's perfectly OK with me, and that's doable. I asked if we should write our own long press tracking. After investigation, that's impossible for the exact same reasons that his ClutterClickAction fix is impossible.
*** Bug 683510 has been marked as a duplicate of this bug. ***
Created attachment 223694 [details] [review] shellEntry: Don't use a ClutterClickAction to pop up a menu This removes support for long press, but fixes the brokenness of other types of event handling.
What long-press action did we even have there, and why ?
We showed a menu on long-press due to the lack of menu button or right-click on touch devices.
Yeah, I don't think out touch support is far enough along to let this block fixing basic entry functionality. If we are serious about touch, we need to merge the XI2 port and do such long-press things only when the event comes from an actual touch device.
Created attachment 223704 [details] [review] shellEntry: Make the entry have a fake focus state when a context menu is open This means that right-clicking on an entry shouldn't visibly change the theme, which is unexpected.
Created attachment 223705 [details] [review] shellEntry: Correct the position of the context menu I don't understand this, it just works. Someone else try to figure out why this works, because it doesn't make sense to me. We attach the menu to the entry, so maybe this is a PopupMenu bug where the position is incorrect for an actor with padding?
(In reply to comment #0) > I expect that double clicking in the search entry would select text. What seems > to happen is that it pops up the context menu. > > Another bug is that pressing escape doesn't cancel the context menu. Another > bug is that when the context menu pops up the Paste item is preselected. It > should probably not have any item selected. Escape works for me, except if you click outside with the context menu to select some blank space. I would investigate further, but I'm just about to leave for my flight. Paste item preselected is because we navigate inside the menu before showing it. I tried just grabbing the menu item (two-line fix, just removes a line of code), but then you can't navigate inside it with the keyboard keys. I believe that's the same bug as https://bugzilla.gnome.org/show_bug.cgi?id=683530
Can I please get a review on this?
Review of attachment 223694 [details] [review]: I think you should use button-press-event instead and return true from the handler when things are handled so that there are no side-effects. It would also be nice to not lose the selection when key focus goes away but that's because we are re-inventing a toolkit. In this case, ClutterText would have to still paint the selection in that case but then we'd also something like GtkClipboard to unset it when it gets set in another widget...
Review of attachment 223704 [details] [review]: ::: js/ui/shellEntry.js @@ +81,3 @@ + + close: function() { + this._entry.remove_style_pseudo_class('focus'); this._entry.grab_key_focus() - has the same effect and the added benefit of returning the key focus which is the right behavior here IMO.
Review of attachment 223705 [details] [review]: This gets called from the event handler is connected to both entry and entry.clutter_text so the alignment should be set relative to the actual actor that got the event.
(In reply to comment #15) > Review of attachment 223705 [details] [review]: > > This gets called from the event handler is connected to both entry and > entry.clutter_text so the alignment should be set relative to the actual actor > that got the event. I don't know what this means. There's nothing specific to the actor that got the event in here; event.get_coords() returns stage coords.
(In reply to comment #14) > Review of attachment 223704 [details] [review]: > > ::: js/ui/shellEntry.js > @@ +81,3 @@ > + > + close: function() { > + this._entry.remove_style_pseudo_class('focus'); > > this._entry.grab_key_focus() - has the same effect and the added benefit of > returning the key focus which is the right behavior here IMO. I don't know. Closing the entry by clicking on something else like a panel menu or something shouldn't return focus to the entry.
(In reply to comment #17) > I don't know. Closing the entry by clicking on something else like a panel menu > or something shouldn't return focus to the entry. I suppose you mean s/Closing the entry/Closing the menu/ because that's what a click away from a popup menu means. Try it on a gtk+ app.
(In reply to comment #16) > I don't know what this means. There's nothing specific to the actor that got > the event in here; event.get_coords() returns stage coords. Right, but then _setMenuAlignment(entry, stageX); which is wrong because the event might have happened on entry or on entry.clutter_text.
(In reply to comment #19) > Right, but then > > _setMenuAlignment(entry, stageX); > > which is wrong because the event might have happened on entry or on > entry.clutter_text. But why does that matter? We have the entry, and the stage coords of where the user clicked. Where does the specifics of the event come into play from here on out?
Created attachment 223935 [details] [review] shellEntry: Don't use a ClutterClickAction to pop up a menu This removes support for long press, but fixes the brokenness of other types of event handling.
Created attachment 223936 [details] [review] shellEntry: Make the entry have a fake focus state when a context menu is open This means that right-clicking on an entry shouldn't visibly change the theme, which is unexpected. Make sure that closing the menu refocused the entry, too.
(In reply to comment #20) > (In reply to comment #19) > > Right, but then > > > > _setMenuAlignment(entry, stageX); > > > > which is wrong because the event might have happened on entry or on > > entry.clutter_text. > > But why does that matter? We have the entry, and the stage coords of where the > user clicked. Where does the specifics of the event come into play from here on > out? Ah yes, you are right. Actual investigation reveals it to be the box pointer positioning taking into account only the actual source actor content, this patch makes it work as you intended I think: --- a/js/ui/boxpointer.js +++ b/js/ui/boxpointer.js @@ -430,11 +430,9 @@ const BoxPointer = new Lang.Class({ _reposition: function(sourceActor, alignment) { // Position correctly relative to the sourceActor - let sourceNode = sourceActor.get_theme_node(); - let sourceContentBox = sourceNode.get_content_box(sourceActor.get_allocation_box()); let sourceAllocation = Shell.util_get_transformed_allocation(sourceActor); - let sourceCenterX = sourceAllocation.x1 + sourceContentBox.x1 + (sourceContentBox.x2 - sourceContentBox.x1) * this._ - let sourceCenterY = sourceAllocation.y1 + sourceContentBox.y1 + (sourceContentBox.y2 - sourceContentBox.y1) * this._ + let sourceCenterX = sourceAllocation.x1 + (sourceAllocation.x2 - sourceAllocation.x1) * this._sourceAlignment; + let sourceCenterY = sourceAllocation.y1 + (sourceAllocation.y2 - sourceAllocation.y1) * this._sourceAlignment; let [minWidth, minHeight, natWidth, natHeight] = this.actor.get_preferred_size(); // We also want to keep it onscreen, and separated from the But I think the current behavior is OK.
Review of attachment 223936 [details] [review]: Seems good.
Review of attachment 223935 [details] [review]: ::: js/ui/shellEntry.js @@ +135,3 @@ entry.menu.open(); } + return true; Should be false here. And true inside the if branches. @@ +157,3 @@ // event processing of ClutterText prevents event-bubbling. + entry.clutter_text.connect('button-release-event', Lang.bind(null, _onButtonReleaseEvent, entry)); + entry.connect('button-release-event', Lang.bind(null, _onButtonReleaseEvent, entry)); Did you try button-press-event? I think we should keep it as close to gtk+'s behavior as possible.
Created attachment 224061 [details] [review] shellEntry: Don't use a ClutterClickAction to pop up a menu This removes support for long press, but fixes the brokenness of other types of event handling.
Review of attachment 224061 [details] [review]: I think it's good to go.
Review of attachment 223705 [details] [review]: Shouldn't be needed so let's remove it.
Attachment 223936 [details] pushed as 2226689 - shellEntry: Make the entry have a fake focus state when a context menu is open Attachment 224061 [details] pushed as 9dfd1bf - shellEntry: Don't use a ClutterClickAction to pop up a menu