803021 - Doorhanger notifications do not retain accessibility focus

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Go to a site that triggers a doorhanger like "Firefox prevented this page from opening a pop up". I saw this on http://mobile.twitter.com when double-tapping a tweet.
2. Touch a button or the notification text in that doorhanger notification.
3. Swipe left or right.

Expected: Should swipe to the previous or next item within the doorhanger.
Actual: Grabs accessibility focus and redirects it to the previous or next element within the web page instead.

Comment 1

[Max Li [:maxli]]

This issue is not restricted to AccessFu and web content; it happens too if you're initialally focused on part of the native UI.
Changing title to something more appropriate.

Comment 2

[Max Li [:maxli]]

Attachment: Patch

So the issue is that all the necessary initialization was not happening if the popup didn't have focus when being shown. Since bug 774209 seems to indicate setting focusability before the popup being shown causes crashes in Gingerbread, I think the solution would be to set the popup to be focusable before only in ICS and above, and leave it as is otherwise. This should fix the issue since Explore by Touch (and thus this bug) only exists for ICS and above anyways.

Comment 3

[:Margaret Leibovic]

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

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

Android focus is always such a pain :/

You should also put an |if (Build.VERSION.SDK_INT < 14)| around the call to setFocusable(false) in dismiss():
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java#313

Otherwise, popups on ICS+ will never regain focus.

::: mobile/android/base/DoorHangerPopup.java
@@ +286,5 @@
>              showAtLocation(mActivity.getView(), Gravity.TOP, 0, 0);
> +            if (Build.VERSION.SDK_INT < 14) {
> +                // Make the popup focusable for keyboard accessibility.
> +                setFocusable(true);
> +            }

To avoid needing to do this twice, let's just turn this if/return block into an if/else, with the else block containing the alternate logic down below.

Comment 4

[Max Li [:maxli]]

Attachment: Patch

> You should also put an |if (Build.VERSION.SDK_INT < 14)| around the call to
> setFocusable(false) in dismiss():

I'm not sure I follow this. Since it is set focusable right before the popup is shown in updatePopup() as far as I can tell from testing the popup remains focusable.

I made the other change though.

Comment 5

[:Margaret Leibovic]

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

I'm sorry, I think I was confused when I made that last comment :) This looks good.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b90fc95d1a5d

Comment 7

[Ed Morley [:emorley]]

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