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

RESOLVED FIXED in Firefox 26

Status

()

defect
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: maxli, Assigned: maxli)

Tracking

({access})

unspecified
Firefox 26
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec26+)

Details

Attachments

(1 attachment)

Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
Assignee

Comment 1

6 years ago
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.
Assignee

Comment 2

6 years ago
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.
Summary: [Fig] Can sometimes accessibility focus web content from about:home → [Fig] Can sometimes accessibility focus hidden content from about:home
OS: All → Android
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.
Severity: normal → major
This is an accessibility regression we need to fix.
tracking-fennec: --- → ?
Assignee

Comment 5

6 years ago
Posted patch PatchSplinter Review
Not entirely sure this is correct... (It does fix the problem and doesn't seem to break anything.)
Assignee: nobody → maxli
Attachment #796693 - Flags: review?(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.
Attachment #796693 - Flags: review?(margaret.leibovic) → review?(lucasr.at.mozilla)
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.
Attachment #796693 - Flags: review?(lucasr.at.mozilla) → review-
Assignee

Comment 8

6 years ago
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.)
Attachment #796693 - Flags: review- → review?(lucasr.at.mozilla)
tracking-fennec: ? → 26+
(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 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.
Attachment #796693 - Flags: review?(lucasr.at.mozilla) → review+
Assignee

Comment 11

6 years ago
Didn't see anything out of the ordinary.

https://hg.mozilla.org/integration/fx-team/rev/e355c7aae965
https://hg.mozilla.org/mozilla-central/rev/e355c7aae965
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26

Updated

6 years ago
Depends on: 911830
You need to log in before you can comment on or make changes to this bug.