Closed
Bug 1481924
Opened 7 years ago
Closed 7 years ago
Add GeckoView accessibility tests for scrolling.
Categories
(Core :: Disability Access APIs, enhancement)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: yzen, Assigned: yzen)
References
Details
(Keywords: access)
Attachments
(2 files, 1 obsolete file)
14.46 KB,
patch
|
eeejay
:
review+
jchen
:
review+
|
Details | Diff | Splinter Review |
6.93 KB,
patch
|
eeejay
:
review+
jchen
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8998872 -
Flags: review?(nchen)
Attachment #8998872 -
Flags: review?(eitan)
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8998872 -
Attachment is obsolete: true
Attachment #8998879 -
Flags: review?(nchen)
Attachment #8998879 -
Flags: review?(eitan)
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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+
Assignee | ||
Comment 6•7 years ago
|
||
(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
Comment 8•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Comment 9•7 years ago
|
||
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 → ---
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #9001758 -
Flags: review?(nchen)
Attachment #9001758 -
Flags: review?(eitan)
Updated•7 years ago
|
Attachment #9001758 -
Flags: review?(eitan) → review+
Comment 11•7 years ago
|
||
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+
Comment 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•