1052026 - Missing contentDescription on menu item between Bookmark and Share

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Marco Zehe (:MarcoZ)]

There seems to be a new menu item which is missing a contentDescription, right between the Bookmark and Share items on the menu when opened on a Nexus 5.

1. Launch Fennec Nightly on a Nexus 5 running KitKat.
2. Load a page.
3. Open the menu.
4. Between the Bookmark and Share items, there is another item which TalkBack only announces as "Graphic" plus some number.

Trivial fix: Add a localized contentDescription describing what this thing is.

Comment 1

[Aaron Train [:aaronmt]]

This is probably one or two quick share items that will appear after you initially share

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to Aaron Train [:aaronmt] from comment #1)
> This is probably one or two quick share items that will appear after you
> initially share

Interesting! Then the contentDescription should be the name of the share target that's being offered. :)

Comment 3

[Wesley Johnston (:wesj)]

We add those here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java#107

but we only pass an icon there! We should probably modify that method to take a string as well:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/MenuItemActionView.java#138

i.e. you should never be creating an action view that has no text for the contentDescription.

Comment 4

[ramasamy.zephyr]

Attachment: Bug_1052026_Add_contentDescription_To_ActionButton.patch

I tested in Nexus 5 and seems to work fine.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8474271 [details] [diff] [review]
Bug_1052026_Add_contentDescription_To_ActionButton.patch

Review of attachment 8474271 [details] [diff] [review]:
-----------------------------------------------------------------

Nice :) And thanks. One little nit. Can you upload a fixed patch and mark checkin-needed :)

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +104,5 @@
>              return view;
>          }
>  
>          for (int i = 0; i < historySize; i++) {
> +            view.addActionButton(dataModel.getActivity(i).loadIcon(packageManager),dataModel.getActivity(i).loadLabel(packageManager));

space after the comma. I'd probably put the second argument on a new line and line it up with the argument above too. It makes it easier to parse the two arguments.

Comment 6

[ramasamy.zephyr]

Attachment: Bug_1052026_Fix.patch

Thanks for reviewing.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8474766 [details] [diff] [review]
Bug_1052026_Fix.patch

Can we get a Try run first please? :)

Also, you can just set the checkin-needed bug keyword when your patch is ready to land.

Comment 8

[ramasamy.zephyr]

I don't have level-1 access to do a try run.

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=429f013f11c8

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

Try builds green, ran the Android performance tests. Java only change so building other platforms is a waste of CPU.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5be7f9b2bef3

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5be7f9b2bef3