852955 - [AccessFu] Trying to activate items at top of pages brings up awesome bar or menu instead

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This is observed on a Jelly Bean NEXUS 7.

STR:
1. With TalkBack on, load a page like http://de.wikipedia.org.
2. Swipe to the right to find the textbox for search.
3. Double-tap to set focus to it.

Expected: Focus moves to the search textbox, and the keyboard comes up.
Actual result: Depending on the location, either the awesome bar opens, or the menu system.

I have also observed this while trying to activate the Log In link on http://alpha.app.net and various other pages.

The problem goes away as soon as one tries to activate an item far enough away from the top of the page, like if it is down one third or towards the middle of the screen.

I suspect this has to do with bug 716403, so CC'ing Chris.

Note that this may also not be the right component eventually, but for now I put it here since it is in AccessFu that the bouns are determined and then sent to the widgets to perform the tap simulation.

Comment 1

[Marco Zehe (:MarcoZ)]

This problem also exists in the 2013-03-20 nightly build which had a few additional fixes to bug 716403 regressions.

Comment 2

[Marco Zehe (:MarcoZ)]

I also reproduced this on a Galaxy Nexus also running Jelly Bean 4.2.2. So this is not specific to the tablet UI.

Comment 3

[Chris Lord [:cwiiis]]

I'm going to assume this is because of bug 716403 and start looking, but it'd be good to have confirmation just in case. Not assigning just yet.

Comment 4

[Marco Zehe (:MarcoZ)]

Info: The bounds to set on the node, which determines where TalkBack simulates the click, are being set in this block of code:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAccessibility.java#151

Comment 5

[Marco Zehe (:MarcoZ)]

This broke between the 2013-03-17 and 2013-03-18 builds, so is a very recent regression. The regression window is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b052daa913c&tochange=b03bb3ce8cee

Comment 6

[Chris Lord [:cwiiis]]

So the problem here is AccessFu uses only the Gecko-side viewport to set the location of page elements on the screen, but the Gecko-side viewport doesn't ever have overscroll, which is how the top of the page is accessibile when the dynamic toolbar is enabled.

Similarly, the coordinates will be wrong during zooming and inaccurate during flings, but these are only fleeting moments.

I'm not sure what the best way to fix this is - I guess ideally, any page-based coordinates would pass through Java first, where we could reconcile this difference, but I haven't figured out how to do this yet...

Alternatively, perhaps we should just disable the dynamic toolbar when TalkBack is on? These features won't ever interact particularly well and it'd be very hard to get Android not to put things underneath the toolbar, but keep the top of the page accessible. I'd also suggest that a toolbar that changes position frequently is not good for accessibility in this situation, regardless of the issues.

Thoughts?

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Chris Lord [:cwiiis] from comment #6)
> So the problem here is AccessFu uses only the Gecko-side viewport to set the
> location of page elements on the screen, but the Gecko-side viewport doesn't
> ever have overscroll, which is how the top of the page is accessibile when
> the dynamic toolbar is enabled.
> 
> Similarly, the coordinates will be wrong during zooming and inaccurate
> during flings, but these are only fleeting moments.
> 
> I'm not sure what the best way to fix this is - I guess ideally, any
> page-based coordinates would pass through Java first, where we could
> reconcile this difference, but I haven't figured out how to do this yet...
> 

I think we do this. The initial hit test is in java, and it is only forwarded to AccessFu if the LayerView is touched. But I haven't looked at this in a while..

> Alternatively, perhaps we should just disable the dynamic toolbar when
> TalkBack is on? These features won't ever interact particularly well and
> it'd be very hard to get Android not to put things underneath the toolbar,
> but keep the top of the page accessible. I'd also suggest that a toolbar
> that changes position frequently is not good for accessibility in this
> situation, regardless of the issues.
> 
> Thoughts?

I agree. This should be disabled in screen reader mode. for usability if nothing else.

Comment 8

[Marco Zehe (:MarcoZ)]

I have no problem with disabling the dynamic toolbar when TalkBack is on. On the contrary. Having played with this a bit, I think for blind users it's actually counterproductive to have to scroll all the way back up to the top of the page to get the toolbar back. There is no quick gesture in TalkBack to scroll all the way to the top in one motion.

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #8)
> I have no problem with disabling the dynamic toolbar when TalkBack is on. On
> the contrary. Having played with this a bit, I think for blind users it's
> actually counterproductive to have to scroll all the way back up to the top
> of the page to get the toolbar back. There is no quick gesture in TalkBack
> to scroll all the way to the top in one motion.

Great, we all agree! This should be done by checking GeckoAccessibility.sEnabled (maybe we need to make that public or add a getter).

Comment 10

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

(In reply to Marco Zehe (:MarcoZ) from comment #8)
> I have no problem with disabling the dynamic toolbar when TalkBack is on. On
> the contrary. Having played with this a bit, I think for blind users it's
> actually counterproductive to have to scroll all the way back up to the top
> of the page to get the toolbar back. There is no quick gesture in TalkBack
> to scroll all the way to the top in one motion.

Bug 708765 (two finger panning) should make it faster to get to the top of the page

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> (In reply to Marco Zehe (:MarcoZ) from comment #8)
> > I have no problem with disabling the dynamic toolbar when TalkBack is on. On
> > the contrary. Having played with this a bit, I think for blind users it's
> > actually counterproductive to have to scroll all the way back up to the top
> > of the page to get the toolbar back. There is no quick gesture in TalkBack
> > to scroll all the way to the top in one motion.
> 
> Bug 708765 (two finger panning) should make it faster to get to the top of
> the page

That isn't available to TalkBack users right now (two fingers is reserved for regular scrolling)

Comment 12

[Chris Lord [:cwiiis]]

(In reply to Eitan Isaacson [:eeejay] from comment #11)
> (In reply to Mark Finkle (:mfinkle) from comment #10)
> > (In reply to Marco Zehe (:MarcoZ) from comment #8)
> > > I have no problem with disabling the dynamic toolbar when TalkBack is on. On
> > > the contrary. Having played with this a bit, I think for blind users it's
> > > actually counterproductive to have to scroll all the way back up to the top
> > > of the page to get the toolbar back. There is no quick gesture in TalkBack
> > > to scroll all the way to the top in one motion.
> > 
> > Bug 708765 (two finger panning) should make it faster to get to the top of
> > the page
> 
> That isn't available to TalkBack users right now (two fingers is reserved
> for regular scrolling)

ftr, you don't ever need to scroll right back to the top to get the toolbar, but the way scrolling works while TalkBack is on doesn't currently interact well with the dynamic toolbar :/

Anyway, almost finished writing the patch to disable dynamic toolbar when accessibility features are enabled.

Comment 13

[Marco Zehe (:MarcoZ)]

Chris, very cool! I'm setup to build Firefox for Android locally so can give this a try in parallel to the regular review process.

Mark, as soon as TalkBack is on, forget everything you know about standard gestures. TalkBack (and assistive technologies on any touch-screen-enabled OS, for that matter) take over *everything* and then may decide to pass certain gestures on as necessary.

Comment 14

[Chris Lord [:cwiiis]]

Attachment: Disable dynamic toolbar when accessibility is enabled

So, this does as suggested, but TalkBack seems to get broken after the toolbar is disabled. The LayerView no longer responds to presses (though you can still swipe next/prev), and the cursor highlight is still offset by the header height.

I don't think this is something I'm going to be able to solve before I go on PTO, so I'm unassigning - I'll pick this back up first thing when I'm back, though I hope that someone else could help in the meantime.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 727704 [details] [diff] [review]
Disable dynamic toolbar when accessibility is enabled

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

Based on comment 14 it sounds like this patch isn't done yet. Minusing to get it out of my queue.

Comment 16

[Eitan Isaacson [:eeejay]]

Attachment: Bug 852955 - Use margin offsets for explore by touch and highlight.

Comment 17

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #16)
> Created attachment 728534 [details] [diff] [review]
> Bug 852955 - Use margin offsets for explore by touch and highlight.

I don't think this is perfect. After some usage it starts to drift a bit. Both the explore by touch accuracy and the highlight. Need to continue looking at this.

Comment 18

[Eitan Isaacson [:eeejay]]

Attachment: Bug 852955 - Use margin offsets for explore by touch and highlight.

Comment 19

[Marco Zehe (:MarcoZ)]

Eitan, why are we abandoning the approach to turn off the dynamic toolbar?

Comment 20

[Eitan Isaacson [:eeejay]]

Attachment: Bug 852955 - Use margin offsets for explore by touch and highlight.

This works better. Together with Chris's patch. I reimplemented minimal scroll into view *sigh*.
I don't think that is necessarily bad. It gives us more control of how and when things are scrolled. For example, it could be good to add some padding.

Comment 21

[Marco Zehe (:MarcoZ)]

Attachment: Fix accessibility screen location during overscroll. r=

During overscroll, the screen location calculated in GeckoAccessibility will
not be offset accordingly.

This is a rebased version of Chris's original patch.

Comment 22

[Marco Zehe (:MarcoZ)]

Comment on attachment 730495 [details] [diff] [review]
Bug 852955 - Use margin offsets for explore by touch and highlight.

A long description of my findings follows, so please bear with me! :)

I ran a local build with this patch, and found that it works for the most part, except when scrolling new stuff into view from the bottom. Even though the ascending sound indicates that something scrolled into view, when double-tapping, the Back, Home, or Recent apps buttons are hit instead. This is on a Galaxy Nexus that does not have hardware soft-keys, but where these buttons are part of the regular touch screen.

Also, when the first of the scrolling tones is heard, it is the low C, the lowest tone current TalkBack produces, which normally indicates everything is scrolled all the way to the top. The first tone when continuously swiping right should be a higher frequency tone. Once this tone is heard and one is on a clickable item and double-taps, the above described clicking of one of the three buttons at the bottom occurs. Subsequently, all ascending tones are basically indicating stuff that isn't in view yet.

Perhaps a call where I can demonstrate this would be best. I reproduced this on the AdBlock Plus add-on page that can be reached from the Firefox Add-Ons for Android page. When swiping through the descriptive text and reaching a "Read More" link, clicking it causes the Home button to be activated instead. If I, instead, swipe further until I hear the next ascending tone, then swipe backwards and double-tap the "Read More" link, it works as expected.

Comment 23

[Eitan Isaacson [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #22)
> Comment on attachment 730495 [details] [diff] [review]
> Bug 852955 - Use margin offsets for explore by touch and highlight.
> 
> A long description of my findings follows, so please bear with me! :)
> 
> I ran a local build with this patch, and found that it works for the most
> part, except when scrolling new stuff into view from the bottom. Even though
> the ascending sound indicates that something scrolled into view, when
> double-tapping, the Back, Home, or Recent apps buttons are hit instead. This
> is on a Galaxy Nexus that does not have hardware soft-keys, but where these
> buttons are part of the regular touch screen.
> 
> Also, when the first of the scrolling tones is heard, it is the low C, the
> lowest tone current TalkBack produces, which normally indicates everything
> is scrolled all the way to the top. The first tone when continuously swiping
> right should be a higher frequency tone. Once this tone is heard and one is
> on a clickable item and double-taps, the above described clicking of one of
> the three buttons at the bottom occurs. Subsequently, all ascending tones
> are basically indicating stuff that isn't in view yet.
> 
> Perhaps a call where I can demonstrate this would be best. I reproduced this
> on the AdBlock Plus add-on page that can be reached from the Firefox Add-Ons
> for Android page. When swiping through the descriptive text and reaching a
> "Read More" link, clicking it causes the Home button to be activated
> instead. If I, instead, swipe further until I hear the next ascending tone,
> then swipe backwards and double-tap the "Read More" link, it works as
> expected.

I have seen exactly that too. Somehow after a full day of testing on Wednesday, I got to a point where it started working OK. But obviously I was wrong. When you set document.documentElement.scrollTop or window.scroll() to something like 10px, it visually scrolls by 70 something px. I have not managed to solve the issue.

I think it may have something to do with the fact that AccessFu messes with the chrome XUL doc and perhaps changes the browser size inadvertently.

Comment 24

[Marco Zehe (:MarcoZ)]

Chris, see comment #23. Any way you can help us solve this?

Comment 25

[Chris Lord [:cwiiis]]

(In reply to Marco Zehe (:MarcoZ) from comment #24)
> Chris, see comment #23. Any way you can help us solve this?

I'm afraid none of the information there really gives me any more clue as to what's going on - I'm having a look again at this though. A summary of how the accessibility stuff hooks in would be handy, I just don't know how any of this works at the moment.

Comment 26

[Chris Lord [:cwiiis]]

ugh, just spotted some silly mistakes in my original patch that would cause it not to work properly - we may be closer than we realised...

Comment 27

[Chris Lord [:cwiiis]]

Attachment: Disable dynamic toolbar when accessibility is enabled

So yeah, this just works... I guess I was just in too much of a rush when I tried to fix it the first time.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 736798 [details] [diff] [review]
Disable dynamic toolbar when accessibility is enabled

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

r=me with a method pulled out as noted below.

::: mobile/android/base/BrowserApp.java
@@ +221,5 @@
>      }
>  
>      @Override
>      public boolean onInterceptTouchEvent(View view, MotionEvent event) {
> +        if (!mDynamicToolbarEnabled || mAccessibilityEnabled || mToolbarPinned) {

This "!mDynamicToolbarEnabled || mAccessibilityEnabled" check occurs in a bunch of places - it might be better to pull out a helper method to do that.

Comment 29

[Chris Lord [:cwiiis]]

Change made, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/dce64e93ed0c

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Marco Zehe (:MarcoZ)]

Verified fixed with my STR in 2013-04-15 Firefox 23.0a1 on a Nexus 4 and a Nexus 7.

Comment 32

[Eitan Isaacson [:eeejay]]

Comment on attachment 736798 [details] [diff] [review]
Disable dynamic toolbar when accessibility is enabled

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 716403
User impact if declined: Screen reader users see a strange vertical offset both in the object highlight, and when exporing by touch.
Testing completed (on m-c, etc.): Yes, it works great (and Marco verfied it as well).
Risk to taking this patch (and alternatives if risky): None that I know of
String or IDL/UUID changes made by this patch: None.

Comment 33

[Eitan Isaacson [:eeejay]]

Chris, before I land this in Aurora, just want to check in with you that it is possible.

Comment 34

[Chris Lord [:cwiiis]]

(In reply to Eitan Isaacson [:eeejay] from comment #33)
> Chris, before I land this in Aurora, just want to check in with you that it
> is possible.

Yes, sorry I let this slip - it should apply to aurora, I think. Note that you'll want to use the updated patch (https://hg.mozilla.org/integration/mozilla-inbound/rev/dce64e93ed0c) rather than the patch attached to this bug.

Comment 35

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1663f486dd70