Closed Bug 803021 Opened 7 years ago Closed 6 years ago

Doorhanger notifications do not retain accessibility focus

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 24
Tracking Status
relnote-firefox --- 24+

People

(Reporter: MarcoZ, Assigned: maxli)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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.
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.
Component: Disability Access APIs → General
Keywords: access
Product: Core → Firefox for Android
Summary: [AccessFu] AccessFu captures swipe gestures in doorhanger notifications, redirects them to web content → Doorhanger notifications do not retain accessibility focus
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → maxli
Attachment #761354 - Flags: review?(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.
Attachment #761354 - Flags: review?(margaret.leibovic) → review-
Attached patch PatchSplinter Review
> 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.
Attachment #761354 - Attachment is obsolete: true
Attachment #762041 - Flags: review?(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.
Attachment #762041 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b90fc95d1a5d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.