GNOME Bugzilla – Bug 368641
Make rating widget accessable
Last modified: 2018-05-24 12:00:15 UTC
Please describe the problem: This is an a11y bug for rhythmbox blind access. Tab browsing is the basic requirement for blind access, each item should be accessible with Tab browsing. Steps to reproduce: 1. Launch rhythmbox. 2. Open a media file in rhythmbox, select it and choose righ-click menu item properties. 3. Select Details tab in properties window, press Tab to move focus to each file property. Actual results: The Duration, File size and Rating can not be tabbed to. Expected results: These widgets should be accessible for tab browsing. Does this happen every time? Yes. Other information: No.
Confirming.
I've made the duration and file-size labels selectable in cvs, but the rating widget will probably need more extensive changes to be properly accessable.
Created attachment 89821 [details] [review] Add a11y keynav and focusability to rating widget
With the above patch, a user may change the the rating via keyboard navigation, via the left/right arrow keys, or home/end, or -/+ or keypad -/+ or by keypad left/right.
Any review, please?
My lack of accessibility knowledge has so far scared me away from reviewing this, but I'll see what I can do. I'm currently reading the "GNOME Accessibility for Developers" guide (http://developer.gnome.org/projects/gap/guide/gad/) - is there any other documentation you'd recommend?
The main part of it seems to be about adding the ability to get/set the rating, and keyboard shortcuts. However: - The get/set rating and refresh_rating could just be a "rating" property. - The key navigation should be in the widget itself - Making the widget descend from a GtkSpin should fix quite a few of the problems already
The key event handling code should probably go into rb-rating-helper.c, rather than rb-song-info.c, since we use the rating widget in a number of places, such as the iradio and podcast properties dialogs. I guess we should also set the accessible object name for the rating cell renderer, which is used in the track listing when the user has chosen to display the rating column. The key event handling stuff probably doesn't belong there. set_rating_button_accessible_name() leaks the aname string; it should only format the string if it's going to use it, and it should free it afterwards. Code style wise, rather than '<modifier> <return type> <name>(<args>) {', we use this: '<modifier> <return type> <name>(<args>) {'
*** Bug 553199 has been marked as a duplicate of this bug. ***
I've got the patch (thanks)! It works well on my source out of debian lenny, and I am able to set ratings with orca now! Thanks. One thing though, when i go to smart playlist: music>playlists>new automatic playlist And I choose rating as the criteria it should match, I can't choose a rating that it should match with orca... Has this dialog got the same issue as the properties one had? Because it looks similar too me. I'm blind and just using braille, and speech, so I can't get any visual information sorry. So if someone could perhaps make this rating scale in the new automatic playlist screen accessible just like was done to the properties one I would greatly appreciate it. Thanks very much, and btw, rhythmbox is great for my music and is very good with orca. So accessibility is really good, so thanks to everyone that worked or is working on this! Dan
Created attachment 119477 [details] [review] another attempt This adds key bindings to the rating widget itself, so it works anywhere the widget is used (including the automatic playlist editor). With this patch, orca doesn't read out the updated value when you modify a rating, but it seems like it probably should.
(In reply to comment #11) > This adds key bindings to the rating widget itself, so it works anywhere the > widget is used (including the automatic playlist editor). Awesome! I tried it in the properties dialog and when creating a new automatic playlist and it does indeed work like a charm. Thanks!! > With this patch, orca doesn't read out the updated value when you modify a > rating, but it seems like it probably should. Yeah... probably.... ;-) ;-) You're absolutely correct. I just opened bug 554111 for that issue and attached a patch that seems to work nicely with your patch. In testing the two together, I noticed that in the properties dialog the widget starts out with an accessible name (e.g. 'No Stars'), but when creating an automatic playlist the widget has no name until you alter the rating. (Thus Orca will say nothing when the widget first gains focus in the automatic playlist dialog.) If you could cause it to start out with an accessible name as well, that would be great. On a similar note, the name does not seem to be exposed in the table cell in the tree of songs. Would it be difficult to add the name there as well? Jonathan, thanks again very much for doing this!
I'm having trouble figuring out how to expose the rating information for table cells. We use a custom cell renderer for the rating column, and I can't really see where you provide accessibility information for those. Should I be setting a role for the rating widget, or would that just make it harder to handle in orca?
Adding Willie Walker to the CC list just in case he's not on the GNOME-a11y-bugs Sun alias. (Hi Will!) (In reply to comment #13) > I'm having trouble figuring out how to expose the rating information for table > cells. We use a custom cell renderer for the rating column, and I can't really > see where you provide accessibility information for those. I don't know the answer to this one. I'll look around though. > Should I be setting a role for the rating widget, or would that just make it > harder to handle in orca? Harder to handle? Shouldn't be. As for the right thing to do, I'm not sure. (Hence my adding Will). On the one hand, this rating widget is functioning a heck of a lot like a GtkSpin (as Bastien suggested). And if you exposed it as being of that role I *suspect* that Orca might JustWork(tm) without any changes on our part because we expect focused GtkSpin's to change value and we present the changes. A quick glance at our code for handling them suggests that, lacking any text to present, we present the name -- which we're getting from y'all thanks to your patch. On the other hand, the rating widget doesn't seem to be a GtkSpin. So, Will, do we want the GtkSpin role for the sake of simplicity, a Rhythmbox custom role (e.g. ROLE_RATING) for the sake of specificity, or the existing ROLE_UNKNOWN for ... the sake of honesty? :-) Jonathan, thanks for asking!
First of all -- many thanks to Jonathan and the Rhythmbox (In reply to comment #14) > Adding Willie Walker to the CC list just in case he's not on the > GNOME-a11y-bugs Sun alias. (Hi Will!) Oh...I'm on the list alright. I get double the e-mail as a result, too. :-) > > Should I be setting a role for the rating widget, or would that just make it > > harder to handle in orca? > > Harder to handle? Shouldn't be. As for the right thing to do, I'm not sure. > (Hence my adding Will). Well....IMO, this is an odd widget that really should be a combobox instead of a custom widget. It's not really a spin button because you can edit those things like text if you want. Is there a strong reason why this cannot just be replaced with a combobox?
(In reply to comment #15) > First of all -- many thanks to Jonathan and the Rhythmbox and the Rhythmbox team for looking at this. Sorry for the incomplete sentence. I'm getting IM'd and phone-called to death right now.
Thanks Will. I'll leave it up to the Rhythmbox guys to answer the question of converting the rating widget to a combo box. I will, however, toss out one "big picture" thing FWIW (probably not much, but....): Should there be some way/process/whathaveyou by which custom widgets which potentially have wider-reaching applicability in the Gtk+ world could be proposed for inclusion by Gtk+? Put another way: The rating widget is cool (both functionally and aesthetically), other apps might want such a rating widget, Jonathan worked out how to make the rating widget accessible. Therefore, maybe Gtk+ should consider adding this widget to its collection of standard widgets. Were they to do that, everyone could use the widget, Orca (and other AT's) would know what to do when it encountered the widget -- and do that independent of the current app -- and the banking crisis would be over. OK, perhaps not, but we could rate stories about the banking crisis in an accessible fashion. ;-) Getting back to the immediate situation: I assume that making the rating widget a combo box constitutes a UI change and thus, if deemed an appropriate change by the Rhythmbox guys, can't be done for 2.24.1/2/3. Jonathan's patch as-is, combined with a change on the Orca side of things, makes this widget usable by folks who are blind in the two dialogs in which it appears. So.... We can have a pretty darned accessible rating widget now and work out the other issues for 2.26, or we can go another 6 months with a completely inaccessible rating widget. My vote, again FWIW, is for getting it working now.
(In reply to comment #17) > Should there be some way/process/whathaveyou by which custom widgets which > potentially have wider-reaching applicability in the Gtk+ world could be > proposed for inclusion by Gtk+? This would be a good thing, since the three main rules for accessibility are: 1) Use the toolkit 2) Use the toolkit 3) Use the toolkit I think the process you propose might exist, and it's called "Bugzilla". ;-) > Getting back to the immediate situation: I assume that making the rating > widget a combo box constitutes a UI change and thus, if deemed an appropriate > change by the Rhythmbox guys, can't be done for 2.24.1/2/3. Freeze breaks are allowed. We've actually done them for Orca to fix serious issues. However, I'm going to guess that with your glowing review of the rating widget, there will be a very slim chance that a change will be considered. So...I'm with you. Let's pursue this route: 1) get the existing rating widget accessibility stuff in, knowing that it will still be exposed with role UNKNOWN, and 2) work on your Orca patch in bug #554111 for 2.24.1.
I'm going through my open bugs/uncommitted patches trying to decide what to aim for for the upcoming 2.24.1 release. Jonathan, if you're planning on checking in the patch you've got as it is (i.e. widget remains custom, role remains UNKNOWN) for 2.24.1, I'll go ahead and check in the one I have for Orca; if you're planning on changing what you've done in the patch, I won't.
I think I'm going to commit the patch as is. I'm reluctant to change the existing mouse interaction, and it sounds tricky to maintain that while converting the widget into a combo box (or a spin button, for that matter), so I probably won't get to it any time soon.
(In reply to comment #20) > I think I'm going to commit the patch as is. Awesome! I've just committed the Orca patch so we should be good to go.
Hi, I've been out of the country for a couple of weeks with no internet access so sorry about the delay in getting back. I tried both patches, the one for orca and the latest one for rhythmbox and with them both it works extremely well, so thanks very much Jonathan for all the work you have done on this and your patch! And thanks very much Joanie for doing the orca patch... And again thanks to everyone who works on rhythmbox it is a really nice music player and manager and accessibility is very good and keeps getting better... So thanks... Oh and I should probably say thanks to the other orca developers for all their work on rhythmbox, so thanks! :-)... Ok, so it works very well for me, the only thing i noticed was that the rating column in the songs table isn't spoken. That is pretty much the only bug I see... Thanks everyone for looking at this greatly appreciated.
Just a gentle nudge to see where we are with this. If the patch seems good, it would be nice to get it in for 2.25.1 and 2.24.2. Thanks so much for your hard work!
I've committed the patch. I still haven't figured out what we can do about accessibility for the rating column in the track list.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/rhythmbox/issues/261.