1454081 - Calculate display bounds in Android correctly

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Eitan Isaacson [:eeejay]]

Android uses async zooming and we don't currently account for it when calculating bounds or getting accessibles from a point. AccessFu does some compensating, but not always correctly.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Fix accessible coordinates in APZ viewports. r?surkov r?yzen ?jchen

A lot of reviewers for a small patch, but it touches 3 different modules:
1. Core accessibility - take into account the set resolution for the accessible's presshel + a test.
2. AccessFu - Streamline the bounds stuff. Remove the context menut activation function for now. It was broken.
3. GeckoView - Don't send mousemove from Android hover events when they are for explore by touch.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8968277 [details] [diff] [review]
Fix accessible coordinates in APZ viewports. r?surkov r?yzen ?jchen

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
@@ +515,5 @@
>      }
>  
>      @Override
>      public boolean onHoverEvent(final MotionEvent event) {
> +        // A touchscreen hover event is a screen reader doing explore-by-touch

I wonder if we get touchscreen hover events from one of those devices with pen support, but I guess we don't have to worry about that right now.

@@ +519,5 @@
> +        // A touchscreen hover event is a screen reader doing explore-by-touch
> +        if (SessionAccessibility.Settings.isEnabled() &&
> +            event.getSource() == InputDevice.SOURCE_TOUCHSCREEN &&
> +            mSession != null) {
> +            mSession.getAccessibility().onExploreByTouch(event);

return true here?

@@ +524,3 @@
>          }
>  
> +        return false;

To preserve previous behavior, we should still forward the event to `onMotionEvent()` if it's non-touchscreen.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8968277 [details] [diff] [review]
Fix accessible coordinates in APZ viewports. r?surkov r?yzen ?jchen

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

thanks, I added some comments/questions:

::: accessible/jsat/AccessFu.jsm
@@ +282,5 @@
>        case GECKOVIEW_MESSAGE.ACTIVATE:
>          this.Input.activateCurrent(data);
>          break;
>        case GECKOVIEW_MESSAGE.LONG_PRESS:
> +        // XXX: Advertize long press on supported objects and implement action

nit: these bits feel like a separate bug/patch.

@@ -847,5 @@
>      this.editState = aEditState;
>    },
>  
> -  // XXX: This is here for backwards compatability with screen reader simulator
> -  // it should be removed when the extension is updated on amo.

nit: also feels like not part of this patch/bug.

::: accessible/jsat/Utils.jsm
@@ -314,3 @@
>    },
>  
>    getTextBounds: function getTextBounds(aAccessible, aStart, aEnd,

Do we need to address textBounds too here?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #1)
> Created attachment 8968277 [details] [diff] [review]
> Fix accessible coordinates in APZ viewports. r?surkov r?yzen ?jchen
> 
> A lot of reviewers for a small patch, but it touches 3 different modules:
> 1. Core accessibility - take into account the set resolution for the
> accessible's presshel + a test.

Could you comment the changes please, in particular Accessible::ChildAtPoint/Bounds, and how resolution is different from zoom? Afaik, we have zoom/bounds tests and hittests running, which seem not affected by the patch.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #4)
> (In reply to Eitan Isaacson [:eeejay] from comment #1)
> > Created attachment 8968277 [details] [diff] [review]
> > Fix accessible coordinates in APZ viewports. r?surkov r?yzen ?jchen
> > 
> > A lot of reviewers for a small patch, but it touches 3 different modules:
> > 1. Core accessibility - take into account the set resolution for the
> > accessible's presshel + a test.
> 
> Could you comment the changes please, in particular
> Accessible::ChildAtPoint/Bounds, and how resolution is different from zoom?
> Afaik, we have zoom/bounds tests and hittests running, which seem not
> affected by the patch.

I'll add comments.

The current tests should not be affected by this patch. The APZ zoom is different from the desktop zoom and needs to be factored separately. I added a bounds test for that.

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> Comment on attachment 8968277 [details] [diff] [review]
> Fix accessible coordinates in APZ viewports. r?surkov r?yzen ?jchen
> 
> Review of attachment 8968277 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java
> @@ +515,5 @@
> >      }
> >  
> >      @Override
> >      public boolean onHoverEvent(final MotionEvent event) {
> > +        // A touchscreen hover event is a screen reader doing explore-by-touch
> 
> I wonder if we get touchscreen hover events from one of those devices with
> pen support, but I guess we don't have to worry about that right now.
> 
> @@ +519,5 @@
> > +        // A touchscreen hover event is a screen reader doing explore-by-touch
> > +        if (SessionAccessibility.Settings.isEnabled() &&
> > +            event.getSource() == InputDevice.SOURCE_TOUCHSCREEN &&
> > +            mSession != null) {
> > +            mSession.getAccessibility().onExploreByTouch(event);
> 
> return true here?
> 
> @@ +524,3 @@
> >          }
> >  
> > +        return false;
> 
> To preserve previous behavior, we should still forward the event to
> `onMotionEvent()` if it's non-touchscreen.

So it turns out we already do that with `GeckoView.onGenericMotionEvent`. I think maybe some coalescing in apzc made that not a problem? Not sure. Anyway, I tested this with a mouse, and I'm pretty sure this is right.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Fix accessible coordinates in APZ viewports. r?surkov r=yzen

Addressed Yura and Jim's nits. Added comments.

Comment 8

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #3)
> Comment on attachment 8968277 [details] [diff] [review]
> Fix accessible coordinates in APZ viewports. r?surkov r?yzen ?jchen
> 
> Review of attachment 8968277 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thanks, I added some comments/questions:
> 
> ::: accessible/jsat/AccessFu.jsm
> @@ +282,5 @@
> >        case GECKOVIEW_MESSAGE.ACTIVATE:
> >          this.Input.activateCurrent(data);
> >          break;
> >        case GECKOVIEW_MESSAGE.LONG_PRESS:
> > +        // XXX: Advertize long press on supported objects and implement action
> 
> nit: these bits feel like a separate bug/patch.
> 

I don't know how to separate the context menu because it is intertwined with all the coordinate stuff, so i just removed it.

> @@ -847,5 @@
> >      this.editState = aEditState;
> >    },
> >  
> > -  // XXX: This is here for backwards compatability with screen reader simulator
> > -  // it should be removed when the extension is updated on amo.
> 
> nit: also feels like not part of this patch/bug.

Sounds good.

> 
> ::: accessible/jsat/Utils.jsm
> @@ -314,3 @@
> >    },
> >  
> >    getTextBounds: function getTextBounds(aAccessible, aStart, aEnd,
> 
> Do we need to address textBounds too here?

Yes! Thanks for the reminder.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8968720 [details] [diff] [review]
Fix accessible coordinates in APZ viewports. r?surkov r=yzen

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

::: accessible/tests/browser/bounds/browser_test_resolution.js
@@ +5,5 @@
> +"use strict";
> +
> +/* import-globals-from ../../mochitest/layout.js */
> +
> +async function testScaledBounds(browser, id, accDoc, scale, text = false) {

the order of arguments: "browser, accDoc, scale, id, isText" might be clearer, since scale is applied to a document not id.

@@ +16,5 @@
> +  let [x, y, width, height] = text ?
> +    getRangeExtents(acc, 0, -1, COORDTYPE_SCREEN_RELATIVE) : getBounds(acc);
> +
> +  await ContentTask.spawn(browser, scale, _scale => {
> +    setResolution(document, _scale);

I'm curious if this is a sync procedure, if not they it may cause intermittents

@@ +41,5 @@
> +  await testScaledBounds(browser, "p2", accDoc, 0.5);
> +  await testScaledBounds(browser, "b1", accDoc, 3.5);
> +
> +  await testScaledBounds(browser, "p1", accDoc, 2.0, true);
> +  await testScaledBounds(browser, "p2", accDoc, 0.75, true);

I would prefer to use literals instead booleans, for example, 'text-bounds'

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: Fix accessible coordinates in APZ viewports. r=surkov r=yzen

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99ec19154f8a
Fix accessible coordinates in APZ viewports. r=surkov, r=yzen, r=jchen

Comment 12

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 99ec19154f8a (bug 1454081) for browser chrome failures on browser_test_resolution.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5a4487e760e31307c461289627602579a53456

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=99ec19154f8ae42c178030c0581551c7c46d230c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=superseded&selectedJob=174603505

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174603505&repo=mozilla-inbound&lineNumber=1835

18:37:56     INFO -  17 INFO TEST-PASS | accessible/tests/browser/bounds/browser_test_resolution.js | Wrong scaled y of [DOM node id: b1, role: pushbutton, name: 'Hello', address: 0x1c12f764460] - Got 262.5 -
18:37:56     INFO -  Buffered messages finished
18:37:56    ERROR -  18 INFO TEST-UNEXPECTED-FAIL | accessible/tests/browser/bounds/browser_test_resolution.js | Uncaught exception - [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIAccessibleText.getRangeExtents]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://mochitests/content/a11y/accessible/tests/mochitest/layout.js :: getRangeExtents :: line 214"  data: no]
18:37:56     INFO -  Stack trace:
18:37:56     INFO -      getRangeExtents@chrome://mochitests/content/a11y/accessible/tests/mochitest/layout.js:214:3
18:37:56     INFO -      testScaledBounds@chrome://mochitests/content/browser/accessible/tests/browser/bounds/browser_test_resolution.js:17:5
18:37:56     INFO -      async*runTests@chrome://mochitests/content/browser/accessible/tests/browser/bounds/browser_test_resolution.js:44:9
18:37:56     INFO -      async*addAccessibleTask/</<@chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js:284:13
18:37:56     INFO -      async*withNewTab@resource://testing-common/BrowserTestUtils.jsm:104:24
18:37:56     INFO -      async*addAccessibleTask/<@chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js:260:11
18:37:56     INFO -      Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1076:21
18:37:56     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:1067:9
18:37:56     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:967:9
18:37:56     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
18:37:56     INFO -  19 INFO Leaving test bound

Comment 13

[Eitan Isaacson [:eeejay]]

Oops. I forgot that this test is not in the mochitest-a11y suite, so I didn't pretest that on 'try'.

Comment 14

[Eitan Isaacson [:eeejay]]

Skipping it on windows, as per bug 1372296

Comment 15

[Eitan Isaacson [:eeejay]]

Attachment: Fix accessible coordinates in APZ viewports. r=surkov, r=yzen

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9520b795d6f3
Fix accessible coordinates in APZ viewports. r=surkov, r=yzen, r=jchen

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9520b795d6f3