Introduce viewport accessible caching in android

RESOLVED FIXED in Firefox 64

Status

()

defect
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(geckoview64 fixed, firefox63 wontfix, firefox64 fixed, firefox65 fixed)

Details

(Whiteboard: [geckoview:p1])

Attachments

(4 attachments)

Assignee

Description

7 months ago
The goal of this caching scheme is to serve these cases:
1. Password managers care about the visible content, specifically if there is a pair of plan and password editboxes.
2. Switch access cares about all the interactive object in view
3. BrailleBack cares about the geometries of the adjacent accessibles.

For those cases we can offer a lightweight cache of all the accessibles in the viewport. For now, we will only provide hierarchy, bounds, className and boolean flags. Because it is lightweight invalidating and repopulating the entire cache is relatively cheap. We will invalidate on every scroll (with a throttled interval), and on initial tree creation, and on any tree mutation. We my add more events that invalidate the cache in the future.

Because the cache is invalidated frequently , this should be good enough for accessible services that are not heavily dependent on events or need low latency, like a screen reader.
Assignee

Comment 1

7 months ago
This protocol is meant to be used by platform wrappers to push bulk data
to the chrome process.

Depends on D9689
Assignee

Comment 2

7 months ago
Defaults now to 'true', when caching is good enough I'll switch it off.

Generally, this would be used for apps/cases when full accessible tree
is needed like UIAutomator.

Depends on D9864
Assignee

Updated

7 months ago
Duplicate of this bug: 1479044
Assignee

Updated

7 months ago
Blocks: android-native-tree-a11y
No longer blocks: 1479037
Assignee

Updated

7 months ago
Blocks: 1499163
Assignee

Updated

7 months ago
Blocks: 1502366

Comment 6

7 months ago
Pushed by eisaacson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a184f7e259c
Add Batch protocol to PDocAccessible. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/70def5ae1d11
Add FULL_ACCESSIBILITY_TREE to GeckoSessionSettings. r=jchen
https://hg.mozilla.org/integration/autoland/rev/89413425620a
Implement native part of viewport caching. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/fe3f1d254a1f
Implement Java part of viewport tree caching. r=jchen
Assignee

Comment 8

6 months ago
Comment on attachment 9020198 [details]
Bug 1502187 - Add Batch protocol to PDocAccessible. r?Jamie!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1479037

User impact if declined: Users may potentially experience slowdowns if they have accessibility enabled (screen reader, password manager, etc)

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1501496, Bug 1479039, Bug 1507026, Bug 1506709

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): This feature (caching) is turned off by default, so this should not impact users directly. A followup uplift will turn this feature on by default and we can monitor it in nightly for a bit longer.

String changes made/needed:
Attachment #9020198 - Flags: approval-mozilla-beta?
Assignee

Comment 9

6 months ago
Comment on attachment 9020199 [details]
Bug 1502187 - Add FULL_ACCESSIBILITY_TREE to GeckoSessionSettings. r?jchen!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1479037

User impact if declined: See previous patch request

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1501496, Bug 1479039, Bug 1507026, Bug 1506709

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): See previous patch request

String changes made/needed:
Attachment #9020199 - Flags: approval-mozilla-beta?
Assignee

Comment 10

6 months ago
Comment on attachment 9020200 [details]
Bug 1502187 - Implement native part of viewport caching. r?Jamie!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1479037

User impact if declined: See previous

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1501496, Bug 1479039, Bug 1507026, Bug 1506709

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): See previous

String changes made/needed:
Attachment #9020200 - Flags: approval-mozilla-beta?
Assignee

Comment 11

6 months ago
Comment on attachment 9020201 [details]
Bug 1502187 - Implement Java part of viewport tree caching. r?jchen!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1479037

User impact if declined: See previous

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1501496, Bug 1479039, Bug 1507026, Bug 1506709

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): See previous

String changes made/needed:
Attachment #9020201 - Flags: approval-mozilla-beta?
I'm rather uneasy about taking this set of changes late in the beta cycle.  Is there a viable alternative such as backing out the regressing change?  How many fennec users enable accessibility?
Flags: needinfo?(eitan)
Assignee

Comment 13

6 months ago
About 1/4 of our users have a11y enabled, if I am reading telemetry correctly[1]. The absolute majority of those users have a11y enabled because of some anti-malware app (or malware), or a password manager. From their perspective Fennec has a noticeable performance regression in 64. Only a tiny percentage of the a11y users are screen reader, or other assistive technology, users. I am almost certain that this, and the other uplifts I asked for, would have no impact outside of Android/GeckoView.

If we were to back out the patches that introduced this regression (bug 1479037), it would be equally disruptive if not more. It would mean reintroducing a few thousand lines of code.

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2018-11-12&include_spill=0&keys=__none__!__none__!__none__&max_channel_version=beta%252F64&measure=A11Y_INSTANTIATED_FLAG&min_channel_version=null&processType=*&product=Fennec&sanitize=1&sort_keys=submissions&start_date=2018-10-22&table=0&trim=1&use_submission_date=0
Flags: needinfo?(eitan)
64=affected and [geckoview:p1] because this bug blocks Focus 8.0.
Whiteboard: [geckoview:p1]
Comment on attachment 9020201 [details]
Bug 1502187 - Implement Java part of viewport tree caching. r?jchen!

a11y changes for 64.0b12
Attachment #9020201 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9020198 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9020199 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9020200 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-geckoview64=fixed
You need to log in before you can comment on or make changes to this bug.