Last Comment Bug 768494 - Provide an accessibility string for the menu button on ICS
: Provide an accessibility string for the menu button on ICS
: access
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: -- major (vote)
: Firefox 16
Assigned To: Marco Zehe (:MarcoZ)
: Sebastian Kaspari (:sebastian)
Depends on:
Blocks: 758431
  Show dependency treegraph
Reported: 2012-06-26 09:03 PDT by Marco Zehe (:MarcoZ)
Modified: 2012-07-03 04:06 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Something like this (1.87 KB, patch)
2012-06-26 09:36 PDT, Marco Zehe (:MarcoZ)
margaret.leibovic: review-
Details | Diff | Splinter Review
Patch 2 (2.83 KB, patch)
2012-06-26 10:35 PDT, Marco Zehe (:MarcoZ)
margaret.leibovic: review+
sriram.mozilla: feedback-
Details | Diff | Splinter Review
Patch v3 (6.53 KB, patch)
2012-06-26 12:09 PDT, Marco Zehe (:MarcoZ)
sriram.mozilla: review+
Details | Diff | Splinter Review
Patch4 (10.96 KB, patch)
2012-06-26 12:36 PDT, Marco Zehe (:MarcoZ)
sriram.mozilla: review+
Details | Diff | Splinter Review
Patch re-based for Aurora (17.76 KB, patch)
2012-06-28 03:25 PDT, Marco Zehe (:MarcoZ)
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) 2012-06-26 09:03:45 PDT
The button to open the menu in Fennec Native on ICS needs an accessibility string so it can be found via Explore By Touch by blind users. It currently has no label and is skipped over when exploring the screen.
Comment 1 Marco Zehe (:MarcoZ) 2012-06-26 09:36:44 PDT
Created attachment 636743 [details] [diff] [review]
Something like this

Untested, but basically copied from setting the tabs contentDescription.
Comment 2 :Margaret Leibovic 2012-06-26 10:25:06 PDT
Comment on attachment 636743 [details] [diff] [review]
Something like this

It's easy to miss this, but you'll also need to add the string to

We just use that file as an intermediary to get the strings from the dtd into Java resources, so it's a mechanical change to just use the same entity names in there.
Comment 3 Marco Zehe (:MarcoZ) 2012-06-26 10:35:02 PDT
Created attachment 636777 [details] [diff] [review]
Patch 2

Thanks, added.
Comment 4 :Margaret Leibovic 2012-06-26 11:34:09 PDT
Comment on attachment 636777 [details] [diff] [review]
Patch 2

This looks good to me, but I'd also like Sriram to take a look to make sure I didn't miss any gotchas in this BrowserToolbar code.
Comment 5 Sriram Ramasubramanian [:sriram] 2012-06-26 11:36:35 PDT
Comment on attachment 636777 [details] [diff] [review]
Patch 2

Review of attachment 636777 [details] [diff] [review]:

I would suggest doing this in the layout files.

Anything in XML is precompiled and hence faster to load, than doing in java.
And while this is done, I would suggest adding content descriptions for "back" and "forward" buttons too.
Comment 6 :Margaret Leibovic 2012-06-26 11:43:15 PDT
I agree with Sriram about optimizing for performance. Marco, if you need an example of how to do it in the XML, we already do it for the find in page bar:
Comment 7 Marco Zehe (:MarcoZ) 2012-06-26 11:57:21 PDT
Thanks to you both! Any other buttons that can be interacted with that I can fix here besides menu, back and forward? I mean, while I'm here...
Comment 8 Marco Zehe (:MarcoZ) 2012-06-26 12:09:40 PDT
Created attachment 636823 [details] [diff] [review]
Patch v3

Used XML as suggested. If these are all that are needed for handhelds, I could add others in the related tablet bug 758431.
Comment 9 :Margaret Leibovic 2012-06-26 12:12:46 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #7)
> Thanks to you both! Any other buttons that can be interacted with that I can
> fix here besides menu, back and forward? I mean, while I'm here...

The "reader", "site_security", and "stop" ImageButtons can all be interacted with, and those are available on all devices.
Comment 10 Sriram Ramasubramanian [:sriram] 2012-06-26 12:26:58 PDT
Comment on attachment 636823 [details] [diff] [review]
Patch v3

Review of attachment 636823 [details] [diff] [review]:

This looks good to me.
Comment 11 Marco Zehe (:MarcoZ) 2012-06-26 12:36:08 PDT
Created attachment 636834 [details] [diff] [review]

I also added the other ones Margaret suggested. I also reused the "forward" entity that was already defined instead of creating a duplicate one, and I also used the reader_mode entity for the reader button. Re-requesting review to make sure this is all OK.
Comment 12 Sriram Ramasubramanian [:sriram] 2012-06-26 12:40:18 PDT
Comment on attachment 636834 [details] [diff] [review]

Review of attachment 636834 [details] [diff] [review]:

Aah right! Forward is already there with menu items.
So, could you move the entities to be with Forward, so everything stays together?
Also, since we do "@string/forward", could you rename everything to be consistent with it? Like "@string/menu", "@string/back" and so on.
r+ with these changes.
Comment 13 Marco Zehe (:MarcoZ) 2012-06-26 12:55:36 PDT
Pushed with requested changes to inbound:

Comment 14 Ed Morley [:emorley] 2012-06-27 03:36:11 PDT
Comment 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-27 16:32:12 PDT
Axel - does a11y have localization?  Is this a concern for uplift to aurora?
Comment 16 Axel Hecht [:Pike] 2012-06-27 16:39:39 PDT
Yes, the patch has l10n impact, and that's raising a concern for uplift.
Comment 17 Marco Zehe (:MarcoZ) 2012-06-27 21:11:03 PDT
I believe this could become even more of a concern once I verify the fix in today's Nightly build, and request approval for Aurora for the patch. If the patch is not taken, the initial accessible Firefox for Android release would be left with a lot of controls inaccessible to blind users on touch-screen-only devices.

However, having said that, if they were in English for some languages in the initial release, that would still be OK. These strings don't show up anywhere visually, they just get spoken by the text-to-speech engine on the Android device.
Comment 18 Marco Zehe (:MarcoZ) 2012-06-28 03:20:55 PDT
Verified the fix in Nightly/Android 16.0a1 (2012-06-28). The Menu button and others, if visible, are now accessible with TalkBack's Explore By Touch feature in ICS.
Comment 19 Marco Zehe (:MarcoZ) 2012-06-28 03:25:57 PDT
Created attachment 637444 [details] [diff] [review]
Patch re-based for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None, neglegance in initial implementation of Native Android UI.
User impact if declined: Blind users on Ice Cream Sandwich will not be able to find the unlabeled buttons via Explore By Touch. In essence, they won't be able to open the menu, for example. Firefox 15 will be the first accessible release, and while web content won't support the Explore By Touch feature yet, the native UI will. Without this, users will have a hard time setting up Sync and do any other adjustments or access basic functionality like bookmarking pages etc.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): None. Just addition of invisible strings and properties in XML files that are only effective when TalkBack, the Android screen reader for the blind, is running. The strings are being spoken as the labels for the buttons.
String or UUID changes made by this patch: Addition of 4 localizable strings that get spoken by a text-to-speech engine, but never displayed on the screen.
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-02 15:23:56 PDT
[triage comment]
definitely a usability issue for people relying on explore by touch and we do want everyone to be able to set up sync.  I'm for approving for Aurora and I trust that this localization can be managed in time before this gets to release.
Comment 21 Axel Hecht [:Pike] 2012-07-02 15:39:39 PDT
FYI, l10n just won't bother with this change.

Marco, mind posting a note to .l10n on why you're landing this?

Note, having string freeze breaks at this point is a clear sign of problems earlier in the cycle.
Comment 22 Marco Zehe (:MarcoZ) 2012-07-03 04:03:41 PDT
Pushed to Aurora:
Comment 23 Marco Zehe (:MarcoZ) 2012-07-03 04:06:08 PDT
(In reply to Axel Hecht [:Pike] from comment #21)
> Note, having string freeze breaks at this point is a clear sign of problems
> earlier in the cycle.

Yes, the problem was only discovered when I got my Galaxy Nexus (an ICS device without a hardware menu button) that actually showed me the problem. But as I stated earlier, if in a first accessible release the spoken strings are in English, it's better than not being able to discover them at all. Note that these strings are already in Nightly/central for 16, so translators should be able to pick them up easily if they haven't already.

Note You need to log in before you can comment on or make changes to this bug.