GNOME Bugzilla – Bug 650666
Required to provide a11y support for StButton
Last modified: 2021-07-05 14:30:32 UTC
Two reasons: * Expose a proper role (see bug 648598) * Define proper "press", "release" and "click" actions (see bug 650665) Also discussed on bug 626660 comment 10 and on bug 644253 comment 11
Created attachment 188416 [details] [review] Skeleton for StButtonAccessible
Created attachment 188417 [details] [review] Adds common actions on StButtonAccessible Specifically: "press", "release" and "click"
The first patch seems safe, but I have some doubts about second one: * This supposes that patch on bug 650665 is applied. So not sure if in order to apply it I need to wait to a bump on the clutter requirement (that includes that version). Anyway, the worst-case would be that both cally-actor and st-button would be adding those actions. So, the simplest approach would apply both patches. * Those action are mostly the same that I had on CallyActor, but some minor changes. But I have been testing it with the "Activities" button, and those doesn't work always. After check the code, it seems that the "clicked" event on StButton is only emitted if the "release" event is considered a click. And it is considered a click if the hover is over the button. It seems that is a attempt of filtering real clicks from any other release events. Anyway, both press and release actions are fine, as they do what asked, press and release over the button, so I will not modify it. After all a11y framework allow to move the cursor. The one that should be modified is "click", as by logic, it involves a real interaction with the user. Right now is a "press" and a "release". So in order to work: * Move the cursor in order to be over the button * Implement it using the click internals. I guess that just emit the signal "clicked" would be enough, but I need to test it.
Comment on attachment 188416 [details] [review] Skeleton for StButtonAccessible looks right
Comment on attachment 188417 [details] [review] Adds common actions on StButtonAccessible >+ ClutterEvent tmp_event; you need to do memset(&tmp_event, 0, sizeof (ClutterEvent)); each time you do this. otherwise looks good
(In reply to comment #5) > (From update of attachment 188417 [details] [review]) > >+ ClutterEvent tmp_event; > > you need to do memset(&tmp_event, 0, sizeof (ClutterEvent)); each time you do > this. Ok > otherwise looks good Ok, I will solve the "click" thing and update the patch, but just a question, what do you think about my question about the coordination with bug 650665 ? Should I need to wait for a clutter dependency bump or just apply the patch once we got a final version?
Oh, duh. I missed your comment about click not working, and then re-checked the source and somehow convinced myself that the hover thing wasn't a problem... must still be asleep. So yeah, for click, just poke into the internals. as for 650665, it doesn't really matter much; no one is really going to be using a11y in git master shell anyway. As long as it gets fixed before the next real release.
Created attachment 188607 [details] [review] Adds common actions on StButtonAccessible (update) In the end, to solve that hover problem I used a similar approach used on GailButton, and I just add some extra CLUTTER_ENTER and CLUTTER_LEAVE, when required. I also think that it would be easier than start to use StButton internals, as most of them are callbacks to those events. I also added those on the actions "press" and "release". Although it is true that in order to get the desired effect you could move the mouse to that position, when you use that action, you usually want to behave as the cursor being over the button.
Comment on attachment 188607 [details] [review] Adds common actions on StButtonAccessible (update) > In the end, to solve that hover problem I used a similar approach used on > GailButton, and I just add some extra CLUTTER_ENTER and CLUTTER_LEAVE, when > required. I also think that it would be easier than start to use StButton > internals, as most of them are callbacks to those events. GailButton had no choice, since it did not have access to GtkButton's internals. StButtonAccessible doesn't have that problem. If the existing internal methods are in some way hard to use, we can write new internal methods.
(In reply to comment #9) > (From update of attachment 188607 [details] [review]) > > In the end, to solve that hover problem I used a similar approach used on > > GailButton, and I just add some extra CLUTTER_ENTER and CLUTTER_LEAVE, when > > required. I also think that it would be easier than start to use StButton > > internals, as most of them are callbacks to those events. > > GailButton had no choice, since it did not have access to GtkButton's > internals. StButtonAccessible doesn't have that problem. > > If the existing internal methods are in some way hard to use, we can write new > internal methods. I guess that you don't like the idea of simulating a clicking by emitting some virtual events ;) So I will try to re-implement that method using just internal methods.
Discussion from #a11y regarding the accessible hierarchy: <API> is it a problem if the label is exposed as the button child? <API> the button would be the one that would receive the focus <API> and the relation is properly set <API> (or should be) <joanie> So the button's name is what? <joanie> derived from the label or nothing? <joanie> as for is it a problem. It's not so much a problem per se <joanie> But remember what we talked about at the hackfest <joanie> we want consistency <joanie> so already your hierarchy is not consistent with Gtk's <API> aha, point taken <joanie> and whether or not a button is implemented via a label or some other magic pixie dust is irrelevant to an AT <joanie> the button has a functional name <joanie> expose the name <joanie> (please and thank you) <joanie> :-) <joanie> And *this* is why we need docs <API> joanie, ok, so being consistent with gtk <API> we shouldn't expose the label as a different object
ftr, we plan to drop press and release in the gtkbutton accessible, and make it just follow the buttons state updates instead of listening for enter/leave itself.
(In reply to comment #12) > ftr, we plan to drop press and release in the gtkbutton accessible, and make it > just follow the buttons state updates instead of listening for enter/leave > itself. Yes, I was CCed, and about avoiding those enter/leaves Dan also agrees (see comment 9). In summary, it is still pending: * Implement click action using exclusively StButton internals * Expose a proper object hierarchy (see comment 11). This would require to redefine ref_child * Implement AtkImage (but probably I will manage this in a different bug) First patch is already accepted. I will try to work on those this weekend. If not I will apply it, so we could have at least the proper role here.
(In reply to comment #13) > First patch is already accepted. I will try to work on those this weekend. If > not I will apply it, so we could have at least the proper role here. In the end I was not able to find that time, so I have just committed the approved patch as it solves the main problem (wrong role). I will work on the other aspects of the bug later.
This has been fixed now, right?
(In reply to comment #15) > This has been fixed now, right? From the description: (In reply to comment #0) > * Expose a proper role (see bug 648598) Fixed. > * Define proper "press", "release" and "click" actions (see bug 650665) Not fixed. So the bug is half-fixed. The main reason the second part was not fixed is because was not a top priority at the moment. Anyway, StButton has right now some accessibility support (manage two different roles + update accessible name based on button label). So if you prefer, we could close this bug, and create a new bug with the missing features (as eventually, I would like to solve them).
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.