872986 - New unlabelled, and seemingly invisible, control in awesome bar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Marco Zehe (:MarcoZ)]

Attachment: Screenshot of TalkBack trying to show an invisible and unlabelled control

This screenshot was taken on a Nexus 7 with a current nightly.
1. With TalkBack on, find the "1 tab" button on the top left.
2. Start swiping to the right multiple times, until you land on the "Reader mode" button.
3. Swipe right once again.

Result: TalkBack will say "Image " followed by an arbitrary, and changing, number. This indicates that the element is unlabelled. The visual indicator shows where TalkBack thinks the control is. However, as a sighted person confirmed for me, there does not seem to be an actual control there, and when double-tapping, nothing happens.

Note: If you are on a page that's encrypted, the unlabelled button appears next to the "Site Security" button instead.

What's this button and why does TalkBack expose it?

Comment 1

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

Sriram - Is this a dupe?

Comment 3

[Aaron Train [:aaronmt]]

Sriram, ping?

Comment 4

[Max Li [:maxli]]

Attachment: Patch

Comment 5

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

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

Is importantForAccessibility available on older versions of Android?

Comment 6

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

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 753244 [details] [diff] [review]
> Patch
> 
> Is importantForAccessibility available on older versions of Android?

I guess I should say: As long as it won't break on older versions of Android

Comment 7

[Marco Zehe (:MarcoZ)]

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

The setImportantForAccessibility method was added in API level 16, but I think the XML variant was there before. Unfortunately the reference doesn't specify the version for the attribute, only the one for the method:
http://developer.android.com/reference/android/view/View.html#attr_android:importantForAccessibility

Comment 8

[Max Li [:maxli]]

Attachment: Revised patch

(In reply to Mark Finkle (:mfinkle) from comment #6)
> > Is importantForAccessibility available on older versions of Android?
> 
> I guess I should say: As long as it won't break on older versions of Android

Good point. It's probably worth making this consistent with how it's done for the other elements like this anyways.

Comment 9

[Sriram Ramasubramanian [:sriram]]

(In reply to Marco Zehe (:MarcoZ) from comment #7)
> Comment on attachment 753244 [details] [diff] [review]
> Patch
> 
> The setImportantForAccessibility method was added in API level 16, but I
> think the XML variant was there before. Unfortunately the reference doesn't
> specify the version for the attribute, only the one for the method:
> http://developer.android.com/reference/android/view/View.html#attr_android:
> importantForAccessibility

http://developer.android.com/reference/android/R.attr.html#importantForAccessibility

The global attribute was added in 16, hence the view attribute was added in 16 too.

The way global attribute works is, whenever an attribute is declared for a styleable like,
<declare-styleable name="XYZ">
  <item name="customAttribute"/>
</declare-styleable>

This "customAttribute" will be a global attribute too (meaning -- it can be set globally at a theme level).

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 754035 [details] [diff] [review]
Revised patch

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

Looks good to me.
r+ after review changes.

::: mobile/android/base/BrowserToolbar.java
@@ +344,5 @@
>              public void onClick(View v) {
>              }
>          });
> +        if (Build.VERSION.SDK_INT >= 16)
> +            mShadow.setImportantForAccessibility(View.IMPORTANT_FOR_ACCESSIBILITY_NO);

Add a new line before "if".
Use braces for the "if block".

Comment 11

[Max Li [:maxli]]

Attachment: Patch with nits fixed

Comment 12

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

Are we sure the contentDescription = @null won't work for jellybean too? I'd like to avoid the Java part of this patch if possible.

Comment 13

[Max Li [:maxli]]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Are we sure the contentDescription = @null won't work for jellybean too? I'd
> like to avoid the Java part of this patch if possible.

The Java portion of the patch is necessary for Jellybean.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59d7396f8e2d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59d7396f8e2d