Closed Bug 1454081 Opened 6 years ago Closed 6 years ago

Calculate display bounds in Android correctly

Categories

(Core :: Disability Access APIs, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- ?
firefox61 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

(Whiteboard: [geckoview:klar])

Attachments

(1 file, 3 obsolete files)

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.
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.
Attachment #8968277 - Flags: review?(yzenevich)
Attachment #8968277 - Flags: review?(surkov.alexander)
Attachment #8968277 - Flags: review?(nchen)
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.
Attachment #8968277 - Flags: review?(nchen) → review+
Whiteboard: [geckoview:klar]
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?
Attachment #8968277 - Flags: review?(yzenevich) → review+
(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.
(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.
(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.
Addressed Yura and Jim's nits. Added comments.
Attachment #8968720 - Flags: review?(surkov.alexander)
Attachment #8968277 - Attachment is obsolete: true
Attachment #8968277 - Flags: review?(surkov.alexander)
(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 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'
Attachment #8968720 - Flags: review?(surkov.alexander) → review+
Attachment #8968720 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99ec19154f8a
Fix accessible coordinates in APZ viewports. r=surkov, r=yzen, r=jchen
Keywords: checkin-needed
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
Flags: needinfo?(eitan)
Oops. I forgot that this test is not in the mochitest-a11y suite, so I didn't pretest that on 'try'.
Flags: needinfo?(eitan)
Skipping it on windows, as per bug 1372296
Attachment #8969337 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9520b795d6f3
Fix accessible coordinates in APZ viewports. r=surkov, r=yzen, r=jchen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9520b795d6f3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.