After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 633853 - Fix up StButton and add keyboard support
Fix up StButton and add keyboard support
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Owen Taylor
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-11-02 20:32 UTC by Dan Winship
Modified: 2010-11-08 18:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
StButton: fix code style (3.77 KB, patch)
2010-11-02 20:32 UTC, Dan Winship
committed Details | Review
StButton: fix hover and grab tracking (6.10 KB, patch)
2010-11-02 20:32 UTC, Dan Winship
committed Details | Review
St: add keyboard support to StClickable and StButton (3.33 KB, patch)
2010-11-02 20:32 UTC, Dan Winship
committed Details | Review

Description Dan Winship 2010-11-02 20:32:30 UTC
Split off from bug 618885. StButton's hover/grab tracking seemed
fairly broken, but I think this should fix it.

Unlike the earlier patch, we now only grab/ungrab the pointer on
button press/release, not on key press/release as well. In particular,
if you buttonpress on the button and then type space/enter, it will
activate the button but it will NOT ungrab the pointer. This matches
gtk
Comment 1 Dan Winship 2010-11-02 20:32:33 UTC
Created attachment 173713 [details] [review]
StButton: fix code style
Comment 2 Dan Winship 2010-11-02 20:32:38 UTC
Created attachment 173714 [details] [review]
StButton: fix hover and grab tracking

Use StWidget:track-hover rather than doing it ourselves. Don't assume
that hover is always TRUE after an enter_event or FALSE after a
leave_event, since we have a pointer grab and will be getting other
actors' events.

Don't ungrab the pointer when it leaves the button, since that
destroys the whole point of getting a grab in the first place.

Only consider the button to have been clicked when it has both grab
(meaning the mouse was pressed over the button) and hover (meaning the
mouse was released over the button).

Also remove the virtual pressed/released methods, which weren't being
used anyway.
Comment 3 Dan Winship 2010-11-02 20:32:42 UTC
Created attachment 173715 [details] [review]
St: add keyboard support to StClickable and StButton

Allow triggering clicks with space/return
Comment 4 Owen Taylor 2010-11-03 21:23:33 UTC
Review of attachment 173713 [details] [review]:

Looks good. Was going to ponder the question of making MX re-merge hard, but it looks like almost every line there has already been touched.
Comment 5 Owen Taylor 2010-11-03 21:34:10 UTC
Review of attachment 173714 [details] [review]:

Looks full functional to me.

::: src/st/st-button.c
@@ +210,3 @@
+        st_button_press (button);
+      else
+        st_button_release (button, FALSE);

Harmless, in any case, but can a enter event ever result in a release? (That is, it might not result in a press, but when would it result in a release?)
Comment 6 Owen Taylor 2010-11-03 21:35:22 UTC
Review of attachment 173715 [details] [review]:

Looks good
Comment 7 Dan Winship 2010-11-03 21:46:34 UTC
(In reply to comment #4)
> Looks good. Was going to ponder the question of making MX re-merge hard, but it
> looks like almost every line there has already been touched.

I did look at MxButton, and you'll be glad to know that removing the virtual press/release methods brings us closer to it. (Although I couldn't bring myself to use "push"/"pull" for the internal method names like they do...)

(In reply to comment #5)
> Harmless, in any case, but can a enter event ever result in a release? (That
> is, it might not result in a press, but when would it result in a release?)

It can only happen if we failed to get a leave event for some reason, and we have a pointer grab, and then we get an enter event on some other (non-descendant) actor. (st_widget_enter_event will clear hover in that case.) Pretty sure that used to happen when exiting the actor through the edge of the stage input area, but I think that was fixed and maybe there aren't any other ways to trigger it?
Comment 8 Dan Winship 2010-11-08 18:08:14 UTC
left the hover stuff as-is for now

Attachment 173713 [details] pushed as 4c6dd64 - StButton: fix code style
Attachment 173714 [details] pushed as 4f1f226 - StButton: fix hover and grab tracking
Attachment 173715 [details] pushed as 35f806d - St: add keyboard support to StClickable and StButton