1481924 - Add GeckoView accessibility tests for scrolling.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

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

We need to test several scenarios related to scrolling, such as scrolling into view, scrolling forward and back and making sure that bounds and scroll positions are correct.

Comment 1

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

Attachment: 1481924 patch

Comment 2

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

Comment on attachment 8998872 [details] [diff] [review]
1481924 patch

removing the r? to address an issue.

Comment 3

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

Attachment: 1481924 patch v2

Comment 4

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

Comment on attachment 8998879 [details] [diff] [review]
1481924 patch v2

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

::: accessible/jsat/Presentation.jsm
@@ +248,5 @@
>  
> +    const currentAcc = currentContext.accessibleForBounds;
> +    if (Utils.isAliveAndVisible(currentAcc)) {
> +      return [{
> +        eventType: AndroidEvents.WINDOW_CONTENT_CHANGED,

I think this should technically be `WINDOW_STATE_CHANGED`? Viewport is part of the window _state_, rather than _content_.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt
@@ +494,5 @@
> +        })
> +
> +        provider.performAction(View.NO_ID, AccessibilityNodeInfo.ACTION_ACCESSIBILITY_FOCUS, null)
> +        sessionRule.waitUntilCalled(object : EventDelegate {
> +            @AssertCalled(count = 1)

Please specify the relative order of these callbacks, e.g. [1]

[1] https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt#39-55

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 8998879 [details] [diff] [review]
1481924 patch v2

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

I'm not 100% convinced the pixel conversion is right. I don't think it matters :)

::: accessible/jsat/Presentation.jsm
@@ +225,5 @@
>     * @param {Window} aWindow window of viewport that changed.
>     */
> +  viewportScrolled(aWindow) {
> +    const { screenPixelsPerCSSPixel, fullZoom } = aWindow.windowUtils;
> +    const scale = screenPixelsPerCSSPixel / fullZoom;

I think you want |resolution|, not fullZoom here.
And I never saw screenPixelsPerCSSPixel before, but if the name is right, it should be good enough?

The conversion I know is (CSS pixel) * (win.devicePixelRatio) * (wu.resolution)

Comment 6

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

(In reply to Eitan Isaacson [:eeejay] from comment #5)
> Comment on attachment 8998879 [details] [diff] [review]
> 1481924 patch v2
> 
> Review of attachment 8998879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not 100% convinced the pixel conversion is right. I don't think it
> matters :)
> 
> ::: accessible/jsat/Presentation.jsm
> @@ +225,5 @@
> >     * @param {Window} aWindow window of viewport that changed.
> >     */
> > +  viewportScrolled(aWindow) {
> > +    const { screenPixelsPerCSSPixel, fullZoom } = aWindow.windowUtils;
> > +    const scale = screenPixelsPerCSSPixel / fullZoom;
> 
> I think you want |resolution|, not fullZoom here.
> And I never saw screenPixelsPerCSSPixel before, but if the name is right, it
> should be good enough?
> 
> The conversion I know is (CSS pixel) * (win.devicePixelRatio) *
> (wu.resolution)

Yeah I looked up the implementation and devicePixelRatio and screenPixelsPerCSSPixel are the same thing. Ill use resolution, afaik zoom is the reverse in this case.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0f8668fb81
added GevkoView accessibility tests for scrolling. r=eeejay, jchen

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6f0f8668fb81

Comment 9

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

Turns out we need to use TYPE_WINDOW_CONTENT_CHANGED event instead of TYPE_WINDOW_STATE_CHANGED after all because it forces redraw and thus updates bounds for selected accessible objects.

Comment 10

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

Attachment: 1481924 follow up

Comment 11

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

Comment on attachment 9001758 [details] [diff] [review]
1481924 follow up

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

Ah ok.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9466b483cb5
use WINDOW_CONTENT_CHANGED instead of WINDOW_STATE_CHANGED when viewport changes to ensure accessible bounds are re-drawn correctly. r=eeejay, jchen

Comment 13

[Daniel Varga [:dvarga]]

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