906670 - [Fig] Can sometimes accessibility focus hidden content from about:home

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Max Li [:maxli]]

I haven't been able to replicate this too reliably, but when using TalkBack and by continuously swiping right (to go to the next element) I can eventually land on the content on the page I'm on. That is, when I reach the end of the awesomescreen, swiping more can bring me to the invisible web content underneath.

Comment 1

[Max Li [:maxli]]

After a little more investigation, this only seems to happen when I'm already focused on an item on a web page and then go to the awesomescreen.

Comment 2

[Max Li [:maxli]]

In addition, when on the search suggestion page, I can actually explore to the tab buttons like Bookmarks and Reading list. From there I can see all that content too.

Changing the title to be a little more general.

Comment 3

[Marco Zehe (:MarcoZ)]

I can confirm all of the above, and also if I just use Explore By Touch, and no swiping, it feels like web content leaks through between the items of the new awesome screen. It is as if the widget that displays the web content is not properly marked hidden.

This is a big usability issue because blind users can no longer distinguish between what is actually being displayed currently, and what is not. Bumping importanceto Major.

Comment 4

[:Margaret Leibovic]

This is an accessibility regression we need to fix.

Comment 5

[Max Li [:maxli]]

Attachment: Patch

Not entirely sure this is correct... (It does fix the problem and doesn't seem to break anything.)

Comment 6

[:Margaret Leibovic]

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

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

I'm going to redirect this to lucasr because he's more familiar with this part of the code.

Comment 7

[Lucas Rocha (:lucasr)]

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

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

::: mobile/android/base/BrowserApp.java
@@ +1431,5 @@
>      }
>  
>      void filterEditingMode(String searchTerm, AutocompleteHandler handler) {
>          if (TextUtils.isEmpty(searchTerm)) {
> +            mHomePager.setVisibility(View.VISIBLE);

I assume this is to avoid being able to focus the about:home views underneath the search results UI? These new setVisibility calls should probably be moved to hideBrowserSearch/showBrowserSearch to guarantee consistent behavior.

::: mobile/android/base/GeckoAccessibility.java
@@ +300,5 @@
>                                  break;
>                              default:
>                                  info.setParent(host);
>                                  info.setSource(host, virtualDescendantId);
> +                                info.setVisibleToUser(host.isFocused());

I need a bit more context to review this change in GeckoAccessibility.

Comment 8

[Max Li [:maxli]]

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

(In reply to Lucas Rocha (:lucasr) from comment #7)
> I assume this is to avoid being able to focus the about:home views
> underneath the search results UI? These new setVisibility calls should
> probably be moved to hideBrowserSearch/showBrowserSearch to guarantee
> consistent behavior.

Yes. I tried that originally, but it doesn't seem like we want that. There are cases where we hide the browser search but don't want to bring up the home pager (it seems like pretty much all calls but the one in filterEditingMode don't need it).


> ::: mobile/android/base/GeckoAccessibility.java
> @@ +300,5 @@
> >                                  break;
> >                              default:
> >                                  info.setParent(host);
> >                                  info.setSource(host, virtualDescendantId);
> > +                                info.setVisibleToUser(host.isFocused());
> 
> I need a bit more context to review this change in GeckoAccessibility.

So the change here is to make the AccessibilityNodeInfos we generate for web content only visible to the screen reader if the layerView (host variable above) is actually focused. It's focused whenever you're actually moving around in the web content and not when you're focused on the awesomescreen. (This wasn't necessary before since I don't think it was possible to have both web content and the awesomescreen shown.)

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Max Li [:maxli] from comment #8)
> Comment on attachment 796693 [details] [diff] [review]
> Patch
> 
> (In reply to Lucas Rocha (:lucasr) from comment #7)
> > I assume this is to avoid being able to focus the about:home views
> > underneath the search results UI? These new setVisibility calls should
> > probably be moved to hideBrowserSearch/showBrowserSearch to guarantee
> > consistent behavior.
> 
> Yes. I tried that originally, but it doesn't seem like we want that. There
> are cases where we hide the browser search but don't want to bring up the
> home pager (it seems like pretty much all calls but the one in
> filterEditingMode don't need it).

Ah, you're right.

> > ::: mobile/android/base/GeckoAccessibility.java
> > @@ +300,5 @@
> > >                                  break;
> > >                              default:
> > >                                  info.setParent(host);
> > >                                  info.setSource(host, virtualDescendantId);
> > > +                                info.setVisibleToUser(host.isFocused());
> > 
> > I need a bit more context to review this change in GeckoAccessibility.
> 
> So the change here is to make the AccessibilityNodeInfos we generate for web
> content only visible to the screen reader if the layerView (host variable
> above) is actually focused. It's focused whenever you're actually moving
> around in the web content and not when you're focused on the awesomescreen.
> (This wasn't necessary before since I don't think it was possible to have
> both web content and the awesomescreen shown.)

Got it, thanks.

Comment 10

[Lucas Rocha (:lucasr)]

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

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

Looks good.

::: mobile/android/base/BrowserApp.java
@@ +1436,4 @@
>              hideBrowserSearch();
>          } else {
>              showBrowserSearch();
> +            mHomePager.setVisibility(View.INVISIBLE);

My only concern here is that showBrowserSearch is not synchronous. This might cause flickering on slower devices because of a short period of time where neither browser search nor homepager are visible on screen.

Before pushing, please test this on a slower device and see if you notice any visual glitch.

Comment 11

[Max Li [:maxli]]

Didn't see anything out of the ordinary.

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e355c7aae965