Closed Bug 895000 Opened 6 years ago Closed 6 years ago

New unlabelled buttons can be accessibility focused on awesomebar

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 25
Tracking Status
firefox24 --- unaffected
firefox25 + verified
fennec 25+ ---

People

(Reporter: maxli, Assigned: maxli)

References

Details

(Keywords: access, regression)

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 734877
tracking-fennec: --- → ?
Keywords: regression
Assignee: nobody → sbhagat
Attached patch bug895000.patch (obsolete) — Splinter Review
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.
Assignee: sbhagat → maxli
Attachment #777773 - Flags: review?(wjohnston)
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?
Attached patch Patch (obsolete) — Splinter Review
> Could you move this line of code into your new "if/else" block?

Sure.
Attachment #777773 - Attachment is obsolete: true
Attachment #777773 - Flags: review?(wjohnston)
Attachment #777782 - Flags: review?(wjohnston)
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?
Attachment #777782 - Flags: review?(wjohnston) → feedback+
tracking-fennec: ? → 25+
Attached patch Patch v2Splinter Review
> I'd even be fine if we had a special page action for the overflow button

I'll leave this to a separate bug.
Attachment #777782 - Attachment is obsolete: true
Attachment #777979 - Flags: review?(wjohnston)
Attachment #777979 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/7b3d37195619
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Verified fixed in Fennec 25.0a1 (2013-07-20)
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.