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

VERIFIED FIXED in Firefox 22

Status

()

Core
Disability Access APIs
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: cwiiis)

Tracking

({access, regression})

Trunk
mozilla23
ARM
Android
access, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ fixed, firefox23 fixed)

Details

(Whiteboard: [NavBar])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
This problem also exists in the 2013-03-20 nightly build which had a few additional fixes to bug 716403 regressions.
(Reporter)

Comment 2

5 years ago
I also reproduced this on a Galaxy Nexus also running Jelly Bean 4.2.2. So this is not specific to the tablet UI.
(Assignee)

Comment 3

5 years ago
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.
Blocks: 716403
Keywords: regressionwindow-wanted
(Reporter)

Comment 4

5 years ago
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
(Reporter)

Comment 5

5 years ago
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
Keywords: regressionwindow-wanted
(Assignee)

Comment 6

5 years ago
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?
(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.
(Reporter)

Comment 8

5 years ago
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.
(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).
(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
(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)
(Assignee)

Comment 12

5 years ago
(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.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
(Reporter)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
Created attachment 727704 [details] [diff] [review]
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.
Attachment #727704 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

5 years ago
Assignee: chrislord.net → nobody
Status: ASSIGNED → NEW
status-firefox21: --- → unaffected
status-firefox22: --- → affected
tracking-firefox22: --- → ?
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.
Attachment #727704 - Flags: review?(bugmail.mozilla) → review-

Updated

5 years ago
Assignee: nobody → chrislord.net

Updated

5 years ago
tracking-firefox22: ? → +
Created attachment 728534 [details] [diff] [review]
Bug 852955 - Use margin offsets for explore by touch and highlight.
(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.
Created attachment 728544 [details] [diff] [review]
Bug 852955 - Use margin offsets for explore by touch and highlight.
Attachment #728534 - Attachment is obsolete: true
(Reporter)

Comment 19

5 years ago
Eitan, why are we abandoning the approach to turn off the dynamic toolbar?
Assignee: chrislord.net → eitan
Created attachment 730495 [details] [diff] [review]
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.
Attachment #728544 - Attachment is obsolete: true
(Reporter)

Comment 21

5 years ago
Created attachment 730598 [details] [diff] [review]
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.
(Reporter)

Comment 22

5 years ago
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.
Attachment #727704 - Attachment is obsolete: true
(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.
(Reporter)

Updated

5 years ago
Flags: needinfo?(chrislord.net)
(Reporter)

Comment 24

5 years ago
Chris, see comment #23. Any way you can help us solve this?
(Assignee)

Comment 25

5 years ago
(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.
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 26

5 years ago
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...
(Assignee)

Comment 27

5 years ago
Created attachment 736798 [details] [diff] [review]
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.
Assignee: eitan → chrislord.net
Attachment #730598 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #736798 - Flags: review?(bugmail.mozilla)
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.
Attachment #736798 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/dce64e93ed0c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 862049

Updated

5 years ago
Whiteboard: [NavBar]
(Reporter)

Comment 31

5 years ago
Verified fixed with my STR in 2013-04-15 Firefox 23.0a1 on a Nexus 4 and a Nexus 7.
Status: RESOLVED → VERIFIED
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.
Attachment #736798 - Flags: approval-mozilla-aurora?
Attachment #736798 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox23: --- → fixed
Chris, before I land this in Aurora, just want to check in with you that it is possible.
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 34

5 years ago
(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.
Flags: needinfo?(chrislord.net)
status-firefox22: affected → fixed
You need to log in before you can comment on or make changes to this bug.