924480 - With talkback, Swiping right from toolbar brings you to content, event when about:home is visible.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Eitan Isaacson [:eeejay]]

This seems to be similar to bug 906670, and I am wondering if bug 911830 reintroduced this, or if it is another issue.

Comment 1

[Lucas Rocha (:lucasr)]

mcomella, could you have a look?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

From what I can repro, this only occurs if about:home is entered from the current tab (with content) - i.e. not a new tab.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

So definitive repro steps:

1) Open a page with content (e.g. https://apple.com)
2) Click the url bar to open about:home
3) Dismiss the keyboard via the back button

Expected: Some item in about:home is selected (probably the toolbar?)
Actual: The page content is selected.

Navigating left and right will navigate through this page content.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01: Hide the web content when the home pager is visible and vice versa

eeejay: Please make sure the visibility elements make sense.

lucasr: Please make sure the side effect of hiding the layer_view doesn't seem like it'll go wrong at any point.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> eeejay: Please make sure the visibility elements make sense.

I mean accessibility elements.

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 817553 [details] [diff] [review]
01: Hide the web content when the home pager is visible and vice versa

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

This looks right. I hope it doesn't have the same pitfalls as the patch in bug 906670, but lucasr would probably know better.

::: mobile/android/base/BrowserApp.java
@@ +1605,5 @@
> +     * Disables screen reader access to the web content (Gecko).
> +     *
> +     * An important side effect is that this method hides the web content view.
> +     */
> +    private void setGeckoScreenReaderAccessEnabled(final boolean enabled) {

IMHO, setting the visibility directly is a one liner and does not require a new method. But I am not a module peer, so what to I know :)

Comment 7

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

Comment on attachment 817553 [details] [diff] [review]
01: Hide the web content when the home pager is visible and vice versa

> setGeckoScreenReaderAccessEnabled

I'd be fine with the raw one-liner used along with a comment as to why we show/hide the layer. We seem to use the raw setVisibility calls for other UI parts.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 817553 [details] [diff] [review]
01: Hide the web content when the home pager is visible and vice versa

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

And I'm fine with a raw one-liner too.

::: mobile/android/base/BrowserApp.java
@@ +1560,5 @@
>              return;
>          }
>  
> +        // This has the side effect of hiding the web content view.
> +        setGeckoScreenReaderAccessEnabled(false);

You have to account for the fade-in animation here i.e. only hide layerview once the animation ends.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Disable TalkBack access to Gecko with HomePager displayed.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Disable TalkBack access to Gecko with HomePager displayed.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Make search_container focusable before browser_toolbar.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 818100 [details] [diff] [review]
Part 1: Make search_container focusable before browser_toolbar.

Applying the patch in part 2, I found a bug where hitting back on the BrowserSearch screen would dismiss the virtual keyboard but refocus the edit text, presumably because the layer_view would be gaining the focus previously. I reordered the view hierarchy in this patch to solve this.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 818099 [details] [diff] [review]
Part 2: Disable TalkBack access to Gecko with HomePager displayed.

I like the function because it cements the motivation for the code in code, rather than a comment, which is more likely to be overlooked, not kept up-to-date, etc., but I don't feel it's very important in this case.

I made the layer_view hide on animation end and also used the cached mLayerView object in this latest version.

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 818099 [details] [diff] [review]
Part 2: Disable TalkBack access to Gecko with HomePager displayed.

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

Looks good but needs to account for the potential race in the animator.

::: mobile/android/base/BrowserApp.java
@@ +1574,5 @@
>              mHomePager = (HomePager) homePagerStub.inflate();
>          }
> +
> +        // Hide the web content so it cannot be focused by screen readers.
> +        hideWebContentOnPropertyAnimationEnd(animator);

I'd prefer something simpler like:

hideWebContentWithAnimator(PropertyAnimator animator) {
    if (animator == null) {
         mLayerView.setVisibility(View.INVISIBLE);
         return;
    }

    animator.addPropertyAnimationListener(new PropertyAnimator.PropertyAnimationListener() {
         ...
    }
}

I would be ok to inline this code in showHomePagerWithAnimator() too.

@@ +1596,5 @@
> +            public void onPropertyAnimationStart() { /* Do nothing. */ }
> +
> +            @Override
> +            public void onPropertyAnimationEnd() {
> +                hideWebContent();

There's a potential race here. If you show and hide homepager very quickly, the animator callback will end up being called last, causing the homepager *and* the layerview to be hidden. You'll have to add code to guard against that.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> I'd prefer something simpler like:

Then you're hiding Gecko before you're displaying the HomePager. Are you okay with that?

> I would be ok to inline this code in showHomePagerWithAnimator() too.

I disagree - imo, the more lines and indents in a function, the harder it is to understand what it's doing and thus skimming code becomes a chore (which means devs do it less, resulting in worse code). Additionally, if everyone makes a habit of inlining these single-use functions, the containing functions start becoming longer than a screen-full and thus are even harder to understand (e.g. BrowserApp.onCreate is about 2.5 screen-fulls, which could be cut down to 1.75 with a "addBrowserToolbarListeners" function - it's pretty obvious what the intention is, unlike .75 screen-fulls of code).

> There's a potential race here.

Working on a fix.

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #15)
> > I'd prefer something simpler like:
> 
> Then you're hiding Gecko before you're displaying the HomePager. Are you
> okay with that?

How so? I only suggested putting hideWebContent() and hideWebContentOnPropertyAnimationEnd() into one method...

> > I would be ok to inline this code in showHomePagerWithAnimator() too.
> 
> I disagree - imo, the more lines and indents in a function, the harder it is
> to understand what it's doing and thus skimming code becomes a chore (which
> means devs do it less, resulting in worse code). Additionally, if everyone
> makes a habit of inlining these single-use functions, the containing
> functions start becoming longer than a screen-full and thus are even harder
> to understand (e.g. BrowserApp.onCreate is about 2.5 screen-fulls, which
> could be cut down to 1.75 with a "addBrowserToolbarListeners" function -
> it's pretty obvious what the intention is, unlike .75 screen-fulls of code).

Fair enough ;-)

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Disable TalkBack access to Gecko with HomePager displayed.

This patch prevents the race mentioned in comment 14 (good call on that, by the
way). I did not address the other issues you mentioned because I would like to
hear your response first.

I also updated the comments a bit.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 818568 [details] [diff] [review]
Part 2: Disable TalkBack access to Gecko with HomePager displayed.

Spoke too soon. :)

> How so? I only suggested putting hideWebContent() and
> hideWebContentOnPropertyAnimationEnd() into one method...

Ah, I didn't realize `animator.start()` was called by the parent method, which means we can set the animation properties after `mHomePager.show()`. Fixing...

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Disable TalkBack access to Gecko with HomePager displayed.

Moved hiding web content to a single method.

Comment 20

[Brian Nicholson (:bnicholson)]

Comment on attachment 818100 [details] [diff] [review]
Part 1: Make search_container focusable before browser_toolbar.

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

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +63,5 @@
> +                     android:visibility="invisible"/>
> +
> +        <!-- When focus is cleared from from BrowserToolbar's edit text to
> +             lower the virtual keyboard, focus will be returned to the root
> +             view. To make sure the root view is not the edit text,

Don't you mean "To make sure the EditText is not the first focusable view in the root view"? There's only one root view, and it doesn't change.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Make search_container focusable before browser_toolbar.

> Don't you mean "To make sure the EditText is not the first focusable view in
> the root view"? There's only one root view, and it doesn't change.

Why yes, yes I do. :)

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 818577 [details] [diff] [review]
Part 2: Disable TalkBack access to Gecko with HomePager displayed.

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

Nice.

::: mobile/android/base/BrowserApp.java
@@ +1608,5 @@
> +    }
> +
> +    private void hideWebContent() {
> +        // The view is set to INVISIBLE, rather than GONE, to avoid the additional requestLayout()
> +        // call.

OCD nit: maybe break this comment into a couple lines? ;-)

@@ +1622,5 @@
>          if (tab != null && isAboutHome(tab)) {
>              return;
>          }
>  
> +        // Display the previously hidden web content (which prevented screen reader access).

nit: move this comment to just before the setVisibility() call.

@@ +1623,5 @@
>              return;
>          }
>  
> +        // Display the previously hidden web content (which prevented screen reader access).
> +        mHideWebContentOnAnimationEnd = false; // Prevent race - see declaration for more info.

nit: move this comment just before the mHideWebContentOnAnimationEnd assignment. We don't usually add comments on the same line.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Disable TalkBack access to Gecko with HomePager displayed.

Fixed nits.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 819079 [details] [diff] [review]
Part 2: Disable TalkBack access to Gecko with HomePager displayed.

Move r+.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/30b7ebdf5c99
https://hg.mozilla.org/integration/fx-team/rev/d8fd745a0095

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30b7ebdf5c99
https://hg.mozilla.org/mozilla-central/rev/d8fd745a0095

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Thanks to a bunch of test restarts, I missed that this was the cause of robocop-3 perma-fail. Backed out.
https://hg.mozilla.org/mozilla-central/rev/565c8f1ba90b

https://tbpl.mozilla.org/php/getParsedLog.php?id=29326049&tree=Fx-Team

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Repro for test failure:

0) On tablet (I used a TF101)
1) Open the browser (fresh start)
2) Click the url bar
3) Quickly scroll to the "Reading List"

Expected: The "Articles you save for later..." screen shows up and the toolbar's edit box is deselected
Actual: The above screen shows up and the toolbar's edit box is selected

This has the effect that if you tab on the url bar (to type in a url, perhaps), rather than selecting *all* the text (which was already highlighted), the cursor is placed within the text. Therefore, in the test, the default "about:home" url does not clear, and the test url is appended.

I imagine this happens because the "Articles you save for later..." screen has to be inflated and so what I presume to be the only focusable item on the page, the edit text, is focused instead.

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

On the latest pull from fx-team, I cannot repro the test failure from comment 27 nor the steps in comment 28, thus a try push with the previous patches:

https://tbpl.mozilla.org/?tree=Try&rev=8d9329139096

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

There were tree troubles and it didn't seem like the build that I needed was building properly so I cancelled and restarted:

https://tbpl.mozilla.org/?tree=Try&rev=2d92b498cb94

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1.5: Make HomePager container focusable.

I was able to repro again (I'm not sure why I was unable to before). The
problem should be well described between comment 28 and the comments in this
patch. Here's a try push of the whole patch stack:

https://tbpl.mozilla.org/?tree=Try&rev=9c434f889a4f

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 820058 [details] [diff] [review]
Part 1.5: Make HomePager container focusable.

This should be just feedback on the selectTab fix until I get your response in comment 17.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 820058 [details] [diff] [review]
Part 1.5: Make HomePager container focusable.

Ah, sorry, wrong bug!

Comment 34

[Lucas Rocha (:lucasr)]

Comment on attachment 820058 [details] [diff] [review]
Part 1.5: Make HomePager container focusable.

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

This solution works fine but wouldn't it be more correct to ensure each page is focusable even they are empty? The home_pager_container view is just an implementation detail, not a meaningful UI element. We should probably remove it as we don't need a container for sub-pages anymore.

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Without creating a maintenance nightmare, the best way I can think to do that is to override HomeFragment.onViewCreated to set the created view as focusable. 

I don't mind the outer container even its only purpose is to be focusable (though it should probably get a name change in that case). In my opinion, it's cleaner from a coding perspective (no need to remember to call `super.onViewCreated(...)`, one xml element rather than a subtle change in an obscure method), but probably less efficient than using onViewCreated.

However, I'm not sure what all of the implications of being focusable actually are (particularly given different hardware configurations), so this may be inaccurate.

What do you think?

Comment 36

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #35)
> Without creating a maintenance nightmare, the best way I can think to do
> that is to override HomeFragment.onViewCreated to set the created view as
> focusable. 
> 
> I don't mind the outer container even its only purpose is to be focusable
> (though it should probably get a name change in that case). In my opinion,
> it's cleaner from a coding perspective (no need to remember to call
> `super.onViewCreated(...)`, one xml element rather than a subtle change in
> an obscure method), but probably less efficient than using onViewCreated.

This home_pager_container view shouldn't be necessary anymore, we can probably remove it (I filed bug 929993 to track this btw). I added it as a container for about:home sub-pages from one of our early designs.

I'd be ok with doing this in onViewCreated() but the intended focusable might be different depending on the view hierarchy of each page, not sure you can generalize it?

> However, I'm not sure what all of the implications of being focusable
> actually are (particularly given different hardware configurations), so this
> may be inaccurate.

Generally speaking, only elements that can actually receive user input should be focusable i.e. what would be the point in focusing a layout? This is why I'm reluctant about having a container just to receive focus.

Have you tried making the homepager itself focusable? That might a slightly better approach. It's probably worth having a look at ViewPager source code to see how it handles focus.

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1.5: Make HomePager focusable.

> Have you tried making the homepager itself focusable?

I don't know why that didn't occur to me - this updated patch does just that,
although I was unable to get the associated attribute to work properly. I
tried looking through the Android source to find out why, but I decided
that I was spending too much time when the method call worked fine (though
I can continue looking if you want).

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1.5: Make HomePager focusable.

OCD comment fix.

Comment 39

[Lucas Rocha (:lucasr)]

Comment on attachment 821200 [details] [diff] [review]
Part 1.5: Make HomePager focusable.

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

This looks better, thanks.

Comment 40

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/36eded3813d6
https://hg.mozilla.org/integration/fx-team/rev/1c70214e37d0
https://hg.mozilla.org/integration/fx-team/rev/29fb758db402

It's worth mentioning that I'd honestly be a bit concerned about uplifting this to 26 because we can get into some strange edge cases where the web content does not reappear (since we're manually hiding and showing the view as we enter and exit editing mode respectively).

Comment 41

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Backout because, as lucasr mentioned, I shouldn't be pushing before merge day.

https://hg.mozilla.org/integration/fx-team/rev/7c559d8ccbb7
https://hg.mozilla.org/integration/fx-team/rev/7b8e9988e6c7
https://hg.mozilla.org/integration/fx-team/rev/e0a2db32d0e8

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Into 28:

remote:   https://hg.mozilla.org/integration/fx-team/rev/e5c9305301da
remote:   https://hg.mozilla.org/integration/fx-team/rev/13048fd980ed
remote:   https://hg.mozilla.org/integration/fx-team/rev/85784e9ecbf0

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5c9305301da
https://hg.mozilla.org/mozilla-central/rev/13048fd980ed
https://hg.mozilla.org/mozilla-central/rev/85784e9ecbf0

Comment 44

[Marco Zehe (:MarcoZ)]

We definitely want these for Firefox 27 and 26 as well, since this is a quite nasty and confusing a11y regression that stares users in the face often.

Comment 45

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 818651 [details] [diff] [review]
Part 1: Make search_container focusable before browser_toolbar.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  new-about:home (landing in 26)

User impact if declined:
  Major accessibility regression, the state of the awesomescreen may be inconsistent when users attempt to navigate it, where the web content of the current page (drawn underneath the awesomescreen) will also be named and navigated through. I imagine this is especially problematic because the layout for the awesomescreen is changing in this version which is a learning curve that may confuse users further.

Testing completed (on m-c, etc.):
  On m-c two days
 
Risk to taking this patch (and alternatives if risky):
  Low risk, this just changes the order that certain Views are focused 
 
String or IDL/UUID changes made by this patch:
  None

Comment 46

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 821200 [details] [diff] [review]
Part 1.5: Make HomePager focusable.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): See comment 45
User impact if declined: See comment 45
Testing completed (on m-c, etc.): See comment 45

Risk to taking this patch (and alternatives if risky):
  Low risk, this patch changes some comments and makes a certain view focusable, which does not affect most users

String or IDL/UUID changes made by this patch: None

Comment 47

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 819079 [details] [diff] [review]
Part 2: Disable TalkBack access to Gecko with HomePager displayed.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): See comment 45
User impact if declined: See comment 45
Testing completed (on m-c, etc.): See comment 45

Risk to taking this patch (and alternatives if risky): 
  High risk. This patch hides the View containing the web content when the awesomescreen is shown meaning that if the awesomescreen is hidden through a different code path than the ones that explicitly show the web content, the browser can be put into an inconsistent state where neither the awesomescreen nor the web content is shown, bricking the browser until the next browser restart (which may need to be an OOM restart).

String or IDL/UUID changes made by this patch: None

Comment 48

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 821200 [details] [diff] [review]
Part 1.5: Make HomePager focusable.

Tracking this, we'll want to make sure there's a lot of testing here before we ship and if necessary, we'll back out of beta if there are any unforseen issues during the cycle.

Comment 49

[Marco Zehe (:MarcoZ)]

Verified fixed in Firefox 28 (2013-11-04).

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18c9ab1860ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/c13c00bbc3f6
https://hg.mozilla.org/releases/mozilla-aurora/rev/5eb72f55ec87

https://hg.mozilla.org/releases/mozilla-beta/rev/c32c858c525c
https://hg.mozilla.org/releases/mozilla-beta/rev/7830b0edf7c9
https://hg.mozilla.org/releases/mozilla-beta/rev/c19cc7933a63

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c32c858c525c
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7830b0edf7c9
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c19cc7933a63

Comment 52

[Marco Zehe (:MarcoZ)]

Verified fixed in Aurora 27.0a2 (2013-11-06).

Comment 53

[Marco Zehe (:MarcoZ)]

Is this fix in 26.0b2, too? If so, I saw some strange behaviour which I could not reproduce on Aurora and Nightly, where both the home screen and active tab were touchable, but only once. Later I could not reproduce this. I will keep testing, but would also appreciate clarity on whether all relevant parts landed, or if this was a partial landing only.

Comment 54

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> Is this fix in 26.0b2, too?

It should be - [1] is the changelog of 26.0b2 and my commits are present. I tested and verified that the changes work as expected (using the build from [2]).

In my testing, the version from the Play Store does not yet have these changes.

[1]: https://hg.mozilla.org/releases/mozilla-beta/shortlog/160990
[2]: ftp://ftp.mozilla.org/pub/mobile/candidates/26.0b2-candidates/build1/android/en-US/

Comment 55

[Marco Zehe (:MarcoZ)]

Thanks Michael, I was indeed testing the Play Store version, since that is what users will get. I will watch the update channel there and verify as soon as the changes arrive there.

Comment 56

[Flaviu Cos]

Verified as fixed in build 26;
Device: Asus Transformer Tab (Android 4.0.3).

Comment 57

[Flaviu Cos]

Based on comment 56 will mark the bug as Verified fixed.