Add GeckoView accessibility tests for scrolling.

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: yzen, Assigned: yzen)

Tracking

(Blocks 1 bug, {access})

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Posted patch 1481924 patch (obsolete) — Splinter Review
Attachment #8998872 - Flags: review?(nchen)
Attachment #8998872 - Flags: review?(eitan)
Comment on attachment 8998872 [details] [diff] [review]
1481924 patch

removing the r? to address an issue.
Attachment #8998872 - Flags: review?(nchen)
Attachment #8998872 - Flags: review?(eitan)
Attachment #8998872 - Attachment is obsolete: true
Attachment #8998879 - Flags: review?(nchen)
Attachment #8998879 - Flags: review?(eitan)
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
Attachment #8998879 - Flags: review?(nchen) → review+
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)
Attachment #8998879 - Flags: review?(eitan) → review+
(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.
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0f8668fb81
added GevkoView accessibility tests for scrolling. r=eeejay, jchen
https://hg.mozilla.org/mozilla-central/rev/6f0f8668fb81
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #9001758 - Flags: review?(nchen)
Attachment #9001758 - Flags: review?(eitan)
Attachment #9001758 - Flags: review?(eitan) → review+
Comment on attachment 9001758 [details] [diff] [review]
1481924 follow up

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

Ah ok.
Attachment #9001758 - Flags: review?(nchen) → review+
Pushed by yura.zenevich@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/d9466b483cb5
Status: REOPENED → RESOLVED
Closed: Last year11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.