Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1015013002:
[android] Accessibility for card unmask dialog. (Closed)
|
Created:
5 years, 9 months ago by please use gerrit instead Modified:
5 years, 9 months ago CC:
browser-components-watch_chromium.org, chromium-reviews, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[android] Accessibility for card unmask dialog.
Announce "Verifying card". Announce the error message and focus the text
field for recoverable errors.
BUG=466699
Committed: https://crrev.com/507fcc4a9830463639c3da2999d211ee844c5452
Cr-Commit-Position: refs/heads/master@{#321478}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Do not focus CVC input field on error. #
Total comments: 4
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Compat and fix announcement cancel #
Total comments: 2
Patch Set 5 : Share logic #
Total comments: 2
Patch Set 6 : Simpler logic. #
Messages
Total messages: 38 (9 generated)
rouslan@chromium.org changed reviewers: + aurimas@chromium.org
Aurimas, PTAL.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1015013002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/1015013002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:314: mCardUnmaskInput.requestFocus(); huh? I don't think we want this
https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:174: mVerificationView.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); Did you mean to use TYPE_WINDOW_CONTENT_CHANGED? TYPE_WINDOW_STATE_CHANGED seems to be used when opening a new popup. https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:313: mErrorMessage.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); TYPE_WINDOW_CONTENT_CHANGED? https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:314: mCardUnmaskInput.requestFocus(); On 2015/03/18 at 00:51:56, Evan Stade wrote: > huh? I don't think we want this Should we only focus for when you have an error? https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:315: mCardUnmaskInput.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED); Why do we need this? https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:340: mNoRetryErrorMessage.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); TYPE_WINDOW_CONTENT_CHANGED?
rouslan@chromium.org changed reviewers: + dmazzoni@chromium.org
Aurimas & Evan, PTAL. There're no code changes, only replies to your comments. Dominic, do you think TYPE_WINDOW_STATE_CHANGED is OK to use here? https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:174: mVerificationView.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > Did you mean to use TYPE_WINDOW_CONTENT_CHANGED? TYPE_WINDOW_STATE_CHANGED seems > to be used when opening a new popup. TYPE_WINDOW_CONTENT_CHANGED does not announce anything in TalkBack. Only TYPE_WINDOW_STATE_CHANGED works. https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:313: mErrorMessage.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > TYPE_WINDOW_CONTENT_CHANGED? Ditto https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:314: mCardUnmaskInput.requestFocus(); On 2015/03/18 00:51:56, Evan Stade wrote: > huh? I don't think we want this From bug report http://crbug.com/466699 "On verify error, we should announce the error message and then focus on the input field." https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:314: mCardUnmaskInput.requestFocus(); On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > On 2015/03/18 at 00:51:56, Evan Stade wrote: > > huh? I don't think we want this > > Should we only focus for when you have an error? Without the error, the dialog goes away, so there's nothing to focus. https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:315: mCardUnmaskInput.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED); On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > Why do we need this? This will announce "Focusing text input field" and will draw a dashed green border around the focused input field. Without this, TalkBack will announce only "Showing number keyboard" and there will be no dashed green border. https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:340: mNoRetryErrorMessage.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > TYPE_WINDOW_CONTENT_CHANGED? Ditto
https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:314: mCardUnmaskInput.requestFocus(); On 2015/03/18 20:35:29, Rouslan Solomakhin wrote: > On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > > On 2015/03/18 at 00:51:56, Evan Stade wrote: > > > huh? I don't think we want this > > > > Should we only focus for when you have an error? > > Without the error, the dialog goes away, so there's nothing to focus. This is not true, setInputError(null) is called elsewhere besides just onVerificationResult. I don't even agree we want this if there is an error. Perhaps if talkbalk is on, but definitely not for the user who doesn't need a11y assistance. Also, we don't always know which field had the mistake (cvc could be correct while expiration date is wrong).
Evan, PTAL Patch Set 2. https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:314: mCardUnmaskInput.requestFocus(); On 2015/03/18 20:47:17, Evan Stade wrote: > On 2015/03/18 20:35:29, Rouslan Solomakhin wrote: > > On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > > > On 2015/03/18 at 00:51:56, Evan Stade wrote: > > > > huh? I don't think we want this > > > > > > Should we only focus for when you have an error? > > > > Without the error, the dialog goes away, so there's nothing to focus. > > This is not true, setInputError(null) is called elsewhere besides just > onVerificationResult. > > I don't even agree we want this if there is an error. Perhaps if talkbalk is on, > but definitely not for the user who doesn't need a11y assistance. Also, we don't > always know which field had the mistake (cvc could be correct while expiration > date is wrong). Makes sense. I removed the two lines for focusing the CVC input field.
https://chromiumcodereview.appspot.com/1015013002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:174: mVerificationView.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); I still don't think TYPE_WINDOW_STATE_CHANGED is correct. From the documentation "Represents the event of opening a {@link android.widget.PopupWindow}, {@link android.view.Menu}, {@link android.app.Dialog}, etc.". We are not opening any of these, we are simply making a new view visible. There is bound to be another way we can do this. dmazzoni@ might know? https://chromiumcodereview.appspot.com/1015013002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:313: mErrorMessage.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); Should we only send this event when message != null?
Evan & Aurimas, PTAL Patch Set 3. I've moved announcement about focus change right after the actual focus change. https://chromiumcodereview.appspot.com/1015013002/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:174: mVerificationView.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); On 2015/03/18 21:42:45, aurimas wrote: > I still don't think TYPE_WINDOW_STATE_CHANGED is correct. From the documentation > "Represents the event of opening a {@link android.widget.PopupWindow}, {@link > android.view.Menu}, {@link android.app.Dialog}, etc.". We are not opening any of > these, we are simply making a new view visible. There is bound to be another way > we can do this. dmazzoni@ might know? Changed to announceForAccesibility(String). https://chromiumcodereview.appspot.com/1015013002/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:313: mErrorMessage.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); On 2015/03/18 21:42:45, aurimas wrote: > Should we only send this event when message != null? Sending "null" is a no-op, so no need to check for it.
https://chromiumcodereview.appspot.com/1015013002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:174: mVerificationView.announceForAccessibility(mVerificationView.getText()); This API exists in 16+ only. Might have to create something in ApiCompatibilityUtils that only calls if for the right api versions and does nothing for pre 16.
PTAL https://chromiumcodereview.appspot.com/1015013002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:174: mVerificationView.announceForAccessibility(mVerificationView.getText()); On 2015/03/18 23:27:07, aurimas wrote: > This API exists in 16+ only. Might have to create something in > ApiCompatibilityUtils that only calls if for the right api versions and does > nothing for pre 16. Done.
On 2015/03/19 00:37:41, Rouslan Solomakhin wrote: > PTAL ... Patch Set 4.
lgtm
lgtm https://chromiumcodereview.appspot.com/1015013002/diff/60001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/60001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:261: if (mShouldRequestExpirationDate) { share this logic with the ternary above, mShouldRequestExpirationDate ? mMonthInput : mCardUnma skInput
rouslan@chromium.org changed reviewers: - dmazzoni@chromium.org
Sending to cq. https://chromiumcodereview.appspot.com/1015013002/diff/60001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/60001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:261: if (mShouldRequestExpirationDate) { On 2015/03/19 00:48:47, Evan Stade wrote: > share this logic with the ternary above, > > mShouldRequestExpirationDate ? mMonthInput : mCardUnma > skInput Done.
On 2015/03/19 at 01:08:44, rouslan wrote: > Sending to cq. Might need base/android owner.
rouslan@chromium.org changed reviewers: + rmcilroy@chromium.org
rmcilroy, OWNERS PTAL: base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java
rouslan@chromium.org changed reviewers: - rmcilroy@chromium.org
Sending to cq.
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:174: mVerificationView.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); On 2015/03/18 20:35:29, Rouslan Solomakhin wrote: > On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > > Did you mean to use TYPE_WINDOW_CONTENT_CHANGED? TYPE_WINDOW_STATE_CHANGED > seems > > to be used when opening a new popup. > > TYPE_WINDOW_CONTENT_CHANGED does not announce anything in TalkBack. Only > TYPE_WINDOW_STATE_CHANGED works. TYPE_WINDOW_STATE_CHANGED is supposed to be used to announce that a new window appeared. If you just want to announce something else, use TYPE_ANNOUNCEMENT or: http://developer.android.com/reference/android/view/View.html#announceForAcce... https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:315: mCardUnmaskInput.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED); On 2015/03/18 20:35:29, Rouslan Solomakhin wrote: > On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > > Why do we need this? > > This will announce "Focusing text input field" and will draw a dashed green > border around the focused input field. Without this, TalkBack will announce only > "Showing number keyboard" and there will be no dashed green border. This seems a little odd to me, though. Is it already focused but not announcing? Typically this event should be fired automatically when a view gets focus. If you have to call it yourself (and it's not a virtual view) that smells funny.
base/android lgtm (although please take a look at Dominic's comment).
Dominic, PTAL my question inline. https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:174: mVerificationView.sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED); On 2015/03/19 06:17:10, dmazzoni wrote: > On 2015/03/18 20:35:29, Rouslan Solomakhin wrote: > > On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > > > Did you mean to use TYPE_WINDOW_CONTENT_CHANGED? TYPE_WINDOW_STATE_CHANGED > > seems > > > to be used when opening a new popup. > > > > TYPE_WINDOW_CONTENT_CHANGED does not announce anything in TalkBack. Only > > TYPE_WINDOW_STATE_CHANGED works. > > TYPE_WINDOW_STATE_CHANGED is supposed to be used to announce that a new window > appeared. > > If you just want to announce something else, use TYPE_ANNOUNCEMENT or: > http://developer.android.com/reference/android/view/View.html#announceForAcce... I Use announceForAccessibility now. https://chromiumcodereview.appspot.com/1015013002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:315: mCardUnmaskInput.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED); On 2015/03/19 06:17:10, dmazzoni wrote: > On 2015/03/18 20:35:29, Rouslan Solomakhin wrote: > > On 2015/03/18 19:15:13, aurimas (ooo until Mar 17) wrote: > > > Why do we need this? > > > > This will announce "Focusing text input field" and will draw a dashed green > > border around the focused input field. Without this, TalkBack will announce > only > > "Showing number keyboard" and there will be no dashed green border. > > This seems a little odd to me, though. Is it already focused but not announcing? > > Typically this event should be fired automatically when a view gets focus. If > you have to call it yourself (and it's not a virtual view) that smells funny. This is because the focusing is accomplished via imm.showSoftInput(). Should requestFocus be used instead?
On 2015/03/19 14:11:26, Rouslan Solomakhin wrote: > Should requestFocus be used instead? Actually, that has to be correct, because showSoftInput() is not correct for the case when a physical keyboard is connected to the device. Let me see if using requestFocus() alone will maintain the functionality and add accessibility...
https://chromiumcodereview.appspot.com/1015013002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://chromiumcodereview.appspot.com/1015013002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:264: mCardUnmaskInput.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED); what I meant was View view = mShouldRequestExpirationDate ? a : b; imm.showSoftInput(view, ...); view.sendA11yEvent(...
On 2015/03/19 14:21:55, Rouslan Solomakhin wrote: > On 2015/03/19 14:11:26, Rouslan Solomakhin wrote: > > Should requestFocus be used instead? > > Actually, that has to be correct, because showSoftInput() is not correct for the > case when a physical keyboard is connected to the device. Let me see if using > requestFocus() alone will maintain the functionality and add accessibility... Contrary to my expectations, requestFocus() does not show the keyboard, does not announce focusing the input field, and does not draw a green border around the input field. So, we have to continue using showSoftInput() and AccessibilityEvent.TYPE_VIEW_FOCUSED. Dominic, please let me know whether that's OK with you.
https://codereview.chromium.org/1015013002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/1015013002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:264: mCardUnmaskInput.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED); On 2015/03/19 15:48:43, Evan Stade wrote: > what I meant was > > View view = mShouldRequestExpirationDate ? a : b; > imm.showSoftInput(view, ...); > view.sendA11yEvent(... Done in Patch Set 6.
lgtm
On 2015/03/19 15:53:04, Rouslan Solomakhin wrote: > On 2015/03/19 14:21:55, Rouslan Solomakhin wrote: > > On 2015/03/19 14:11:26, Rouslan Solomakhin wrote: > > > Should requestFocus be used instead? > > > > Actually, that has to be correct, because showSoftInput() is not correct for > the > > case when a physical keyboard is connected to the device. Let me see if using > > requestFocus() alone will maintain the functionality and add accessibility... > > Contrary to my expectations, requestFocus() does not show the keyboard, does not > announce focusing the input field, and does not draw a green border around the > input field. So, we have to continue using showSoftInput() and > AccessibilityEvent.TYPE_VIEW_FOCUSED. > > Dominic, please let me know whether that's OK with you. That's fine.
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1015013002/#ps100001 (title: "Simpler logic.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015013002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/507fcc4a9830463639c3da2999d211ee844c5452 Cr-Commit-Position: refs/heads/master@{#321478} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
