895000 - New unlabelled buttons can be accessibility focused on awesomebar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Max Li [:maxli]]

As of the July 17 Nightly build, there's (at least) a couple of unlabelled buttons that can be focused using Talkback. One of these appears to be the Reader mode button (which was previously properly labelled); I don't know what the other one is. These buttons can be focused even when they aren't visible.

Seems to have been regressed by bug 734877.

Comment 1

[Max Li [:maxli]]

Attachment: bug895000.patch

Bug 895056 fixed that you could focus invisible elements. All that is needed here is setting the content description. So I'm stealing this bug.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 777773 [details] [diff] [review]
bug895000.patch

drive-by

>diff --git a/mobile/android/base/PageActionLayout.java b/mobile/android/base/PageActionLayout.java

>                     public void run () {

>                         v.setImageDrawable((mPageActionList.size() > mMaxVisiblePageActions) ? resources.getDrawable(R.drawable.icon_pageaction) : d);

Could you move this line of code into your new "if/else" block?

Comment 3

[Max Li [:maxli]]

Attachment: Patch

> Could you move this line of code into your new "if/else" block?

Sure.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 777782 [details] [diff] [review]
Patch

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

This looks like the right fix, but I want to consolidate all of this code into one place so we don't have to update three all the time.

::: mobile/android/base/PageActionLayout.java
@@ +185,5 @@
> +                            v.setContentDescription(resources.getString(R.string.page_actions));
> +                        } else {
> +                            v.setImageDrawable((pageAction != null) ? pageAction.getDrawable() : null);
> +                            v.setContentDescription((pageAction != null) ? pageAction.getTitle() : null);
> +                        }

Would you mind consolidating this stuff a bit into a helper function. i.e. something like:

private void setActionForView(View view, Pageaction pageaction) {
  if (pageaction == null) {
    // hide the view, remove its description, icon, and tag
    return;
  }
  // show the view,
  // add a description
  // and an icon
  // set the pageAction.id as a tag on the view
}

I'd even be fine if we had a special page action for the overflow button if you wanted too (maybe not stored in our normal list. So this became

if (size > max) {
  pageaction = overflowAction;
}
setActionForView(v, pageaction);

but we can do that in a separate bug if you're uncomfortable with it here.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +252,5 @@
>  <!ENTITY reading_list_duplicate "Page already in your Reading List">
> +
> +<!-- Localization note : These strings are used as alternate text for accessibility.
> +     They are not visible in the UI. -->
> +<!ENTITY page_actions "Additional Actions">

lets make this entity more descriptive. page_action_dropmarker_description?

Comment 5

[Max Li [:maxli]]

Attachment: Patch v2

> I'd even be fine if we had a special page action for the overflow button

I'll leave this to a separate bug.

Comment 6

[Max Li [:maxli]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3d37195619

Comment 7

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/7b3d37195619

Comment 8

[Marco Zehe (:MarcoZ)]

Verified fixed in Fennec 25.0a1 (2013-07-20)