Closed Bug 1473998 Opened Last year Closed Last year

Use EditText class names on editable accessibility nodes

Categories

(GeckoView :: General, defect, P1)

defect

Tracking

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

Details

(Whiteboard: [geckoview:klar:p1])

Attachments

(1 file, 4 obsolete files)

We currently provide a role name, a label, and the contents of an input value in the 'text' field of the node. This makes it hard to properly support text editing.

TalkBack expects the following:

1. If the className is 'android.widget.EditText', TalkBack will utter "Edit Box", in addition a text editing submenu will be added to TalkBack's local menu when the element is focused.
2. TalkBack will use the node's 'text' field for the actual text value of the entry.
3. Talkback will use a hint inserted into the 'extras' property bag as the label for the entry.
This patch does a couple of other things:
- Remove checks for API less than 16.
- Changed the scroll events to be followed by window content changed events. That is the correct way to update the bounds on an accessible, not send more a11y focus events.
Attachment #8990469 - Flags: review?(yzenevich)
Attachment #8990469 - Flags: review?(nchen)
Assigning to Eitan since he posted a patch.

I assume we will want to uplift this fix to GV 62 Beta for the initial Focus+GV release.
Assignee: nobody → eitan
Priority: -- → P1
Comment on attachment 8990469 [details] [diff] [review]
Use EditText entry in accessible's classNames and use the hint for the label. r?yzen r?jchen

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

r+ for Java bits.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt
@@ +3,5 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  package org.mozilla.geckoview.test
>  
> +import android.os.Bundle

Move to before `import android.support.test.filters.MediumTest`

@@ +61,5 @@
>          }
>      }
>  
> +    private fun createURL(html: String): String {
> +        return "data:text/html;base64," + Base64.getEncoder().encodeToString(html.toByteArray(Charset.forName("utf-8")))

Use `GeckoSession.createDataUri`

@@ +163,5 @@
> +            @AssertCalled(count = 1)
> +            override fun onAccessibilityFocused(event: AccessibilityEvent) {
> +                val nodeId = getSourceId(event)
> +                val node = provider.createAccessibilityNodeInfo(nodeId)
> +                android.util.Log.i("eeejay", node.toString())

Remove
Attachment #8990469 - Flags: review?(nchen) → review+
Attachment #8990469 - Attachment is obsolete: true
Attachment #8990469 - Flags: review?(yzenevich)
Attachment #8990469 - Flags: review?(yzenevich)
Comment on attachment 8990800 [details] [diff] [review]
Use EditText entry in accessible's classNames and use the hint for the label. r?yzen r?jchen

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

Tanks, just a couple of nits.

::: accessible/jsat/Presentation.jsm
@@ +46,5 @@
>      }
>  
>      let androidEvents = [];
>  
> +    let isExploreByTouch = (aReason == Ci.nsIAccessiblePivot.REASON_POINT);

nit: let -> const and no reason for ()

@@ +55,5 @@
>        androidEvents.push({eventType: AndroidEvents.VIEW_HOVER_EXIT, text: []});
>      }
>  
>      if (aReason === Ci.nsIAccessiblePivot.REASON_TEXT) {
> +      let adjustedText = context.textAndAdjustedOffsets;

nit: const

@@ +143,5 @@
>     */
>    textSelectionChanged(aText, aStart, aEnd, aOldStart, aOldEnd, aIsFromUserInput) {
>      let androidEvents = [];
>  
> +    if (!aIsFromUserInput) {

Could you flip this to be if (aIsFromUserInput) ... ?

@@ +287,5 @@
>    }
>  
>    _infoFromContext(aContext) {
>      let state = Utils.getState(aContext.accessible);
> +    let info = {

nit: const
Attachment #8990800 - Flags: review+
Attachment #8991092 - Attachment is obsolete: true
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9049c5ac63
Use EditText entry in accessible's classNames and use the hint for the label. r=yzen r=jchen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae9049c5ac63
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Backed out for gv-junit failures on test.AccessibilityTest.testTextEntryNode

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=f017a427c3cbccf397c9bf960a7b84ae5e801cb9&filter-searchStr=Android%204.3%20API16%2B%20debug%20test-android-em-4.3-arm7-api-16%2Fdebug-geckoview-junit-1%20(gv-junit1)&selectedJob=187479943

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187479943&repo=mozilla-inbound&lineNumber=1470

Backout link: https://hg.mozilla.org/mozilla-central/rev/3edc9c3ae818490ed36b8bfc8ffdfc9e222b41db

[task 2018-07-10T22:47:39.436Z] 22:47:39     INFO -  org.mozilla.geckoview.test |
[task 2018-07-10T22:47:39.436Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=testTextEntryNode
[task 2018-07-10T22:47:39.437Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: -2
[task 2018-07-10T22:47:39.437Z] 22:47:39  WARNING -  TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.AccessibilityTest.testTextEntryNode | status -2
[task 2018-07-10T22:47:39.438Z] 22:47:39     INFO -  TEST-INFO took 10835ms
[task 2018-07-10T22:47:39.438Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
[task 2018-07-10T22:47:39.439Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=7
[task 2018-07-10T22:47:39.439Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.ContentDelegateTest
[task 2018-07-10T22:47:39.440Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream=
[task 2018-07-10T22:47:39.440Z] 22:47:39     INFO -  org.mozilla.geckoview.test | org.mozilla.geckoview.test.ContentDelegateTest:
[task 2018-07-10T22:47:39.440Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=88
[task 2018-07-10T22:47:39.441Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=saveAndRestoreState
[task 2018-07-10T22:47:39.441Z] 22:47:39     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: 1
Status: RESOLVED → REOPENED
Flags: needinfo?(eitan)
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
Attachment #8991097 - Attachment is obsolete: true
Flags: needinfo?(eitan)
Oops, forgot to account for older API.
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d1b39a642f
Use EditText entry in accessible's classNames and use the hint for the label. r=yzen r=jchen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f3d1b39a642f
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment on attachment 8990469 [details] [diff] [review]
Use EditText entry in accessible's classNames and use the hint for the label. r?yzen r?jchen

Already r+ed more recent one
Attachment #8990469 - Flags: review?(yzenevich) → review+
Eitan, do you consider this bug to be a blocker for the initial Focus+GV release? If so, we should uplift this fix to 62 beta.
Flags: needinfo?(eitan)
Comment on attachment 8991159 [details] [diff] [review]
Use EditText entry in accessible's classNames and use the hint for the label. r=yzen

Approval Request Comment
[Feature/Bug causing the regression]: WebView parity
[User impact if declined]: Users will not get basic text editing functions in TalkBack
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: 
[Is the change risky?]: Slightly, but only for TalkBack users
[Why is the change risky/not risky?]:
[String changes made/needed]:
Flags: needinfo?(eitan)
Attachment #8991159 - Flags: approval-mozilla-beta?
Comment on attachment 8991159 [details] [diff] [review]
Use EditText entry in accessible's classNames and use the hint for the label. r=yzen

Geckoview-only fix for a11y, let's uplift for beta 9.
Attachment #8991159 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
[geckoview:klar:p1] because this bug is a Focus+GV blocker.
Whiteboard: [geckoview:klar:p1]
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.