Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1055883002:
Fire an alert event to notify accessibility when a permissions bubble gets shown. (Closed)
|
Created:
5 years, 8 months ago by David Tseng Modified:
5 years, 8 months ago CC:
chromium-reviews, dtseng+watch_chromium.org, je_julie(Not used), nkostylev+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, tfarina, nektar+watch_chromium.org, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, oshima+watch_chromium.org, markusheintz_, stevenjb+watch_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd an event to notify accessibility when a permissions bubble gets shown.
BUG=381338
TEST=ctrl+alt+z; navigate to html5demos.com/geo; observe speech is as expected (reads contents of bubble).
Committed: https://crrev.com/36318aa45e02cf0b5cf871bf38bbe47f3431cc8d
Cr-Commit-Position: refs/heads/master@{#323982}
Patch Set 1 #Patch Set 2 : Adjust some output rules for ChromeVox. #
Total comments: 4
Patch Set 3 : #
Total comments: 2
Patch Set 4 : Do not condition upon presence of anchor. #
Total comments: 2
Patch Set 5 : Remove braces. #Patch Set 6 : Fix tests. #
Total comments: 6
Messages
Total messages: 30 (8 generated)
dtseng@chromium.org changed reviewers: + felt@chromium.org
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org, markusheintz@chromium.org
+ markusheintz for c/b/u/w/* + dmazzoni for c/b/r/c/chromevox
I'd prefer to solve this for all bubbles, not just this one https://codereview.chromium.org/1055883002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/1055883002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:198: speak: '$value $name' This should probably say $value OR $name, we can work on that later https://codereview.chromium.org/1055883002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1055883002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:448: bubble_delegate_->NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true); I think you should do this in ui/views/bubble/bubble_delegate.cc instead. Have CreateBubbleWidget return a subclass of Widget instead, and have it fire an alert event on Show().
https://codereview.chromium.org/1055883002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/1055883002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:198: speak: '$value $name' On 2015/04/02 20:29:20, dmazzoni wrote: > This should probably say $value OR $name, we can work on that later Views uses name...web uses value. https://codereview.chromium.org/1055883002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1055883002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:448: bubble_delegate_->NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true); On 2015/04/02 20:29:20, dmazzoni wrote: > I think you should do this in ui/views/bubble/bubble_delegate.cc instead. Have > CreateBubbleWidget return a subclass of Widget instead, and have it fire an > alert event on Show(). Not sure what you mean. I moved the alert notification to HandleVisibilityChanged inside of BubbleDelegate.
lgtm Putting it in BubbleDelegateView::HandleVisibilityChanged sounds great. Thanks.
felt@chromium.org changed reviewers: + msw@chromium.org
+msw since neither markuzheintz_ nor i are reviewers for ui/views/bubble/
Monday bump. Thanks. On 2015/04/04 14:36:32, felt wrote: > +msw since neither markuzheintz_ nor i are reviewers for ui/views/bubble/
https://codereview.chromium.org/1055883002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1055883002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_delegate.cc:305: NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true); Most bubbles should have an anchor, but I suspect that firing this event shouldn't be contingent on the presence of an anchor widget (nor that object's top level widget).
https://codereview.chromium.org/1055883002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1055883002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_delegate.cc:305: NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true); On 2015/04/06 17:30:19, msw wrote: > Most bubbles should have an anchor, but I suspect that firing this event > shouldn't be contingent on the presence of an anchor widget (nor that object's > top level widget). Done.
lgtm with a nit https://codereview.chromium.org/1055883002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1055883002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_delegate.cc:303: if (visible) { nit: you don't need to add these curly braces anymore.
https://codereview.chromium.org/1055883002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1055883002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_delegate.cc:303: if (visible) { On 2015/04/06 17:57:20, msw wrote: > nit: you don't need to add these curly braces anymore. Done.
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1055883002/#ps80001 (title: "Remove braces.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1055883002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/1055883002/#ps100001 (title: "Fix tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1055883002/100001
Now I'm confused and worried that this is wrong... not lgtm. Why isn't this applicable to all bubbles and why doesn't it work like views dialogs? Is this somehow treating this bubble like a JS alert bubble? https://codereview.chromium.org/1055883002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1055883002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:357: state->role = ui::AX_ROLE_ALERT_DIALOG; Why can't this use ui::AX_ROLE_DIALOG like other bubbles? https://codereview.chromium.org/1055883002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1055883002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_delegate.cc:312: if (state.role == ui::AX_ROLE_ALERT_DIALOG) Why did you add this condition? Why can't all bubbles be treated similarly?
https://codereview.chromium.org/1055883002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/1055883002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:357: state->role = ui::AX_ROLE_ALERT_DIALOG; On 2015/04/06 23:01:20, msw wrote: > Why can't this use ui::AX_ROLE_DIALOG like other bubbles? See my other comments. https://codereview.chromium.org/1055883002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1055883002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_delegate.cc:312: if (state.role == ui::AX_ROLE_ALERT_DIALOG) On 2015/04/06 23:01:20, msw wrote: > Why did you add this condition? Why can't all bubbles be treated similarly? Sorry; I should have pinged you on the changes here. SystemTray happens to be a subclass of this class. The semantics of an alert currently are as follows: For ChromeVox, it means compute an utterance by concatenating all of the node's descendant views as if you navigated to each of them linearly. For SystemTray, this is undesirable since we really want it to simply read the name but not all of its descendant views. Had an offline discussion with dmazzoni@. One option, taken here, is to explicitly mark all views that are actually alert dialogs as having role alert dialog and triggering the alert event for alert dialogs only. A different event should be fired for the "other" cases to distinguish it for clients such as ChromeVox. Note that I spent some time exploring other strategies, but due to the strictness of interactive_ui_tests --gtest_filter=*SpokenFeedbackTests*.*, this was the minimal change possible without breaking lots of test assumptions.
Dominic, would you be ok if I submitted patchset 2 considering that we will need to merge this fix? We can explore how to do this right on trunk.
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/36318aa45e02cf0b5cf871bf38bbe47f3431cc8d Cr-Commit-Position: refs/heads/master@{#323982}
Message was sent while issue was closed.
https://codereview.chromium.org/1055883002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1055883002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_delegate.cc:312: if (state.role == ui::AX_ROLE_ALERT_DIALOG) On 2015/04/06 23:34:02, David Tseng wrote: > On 2015/04/06 23:01:20, msw wrote: > > Why did you add this condition? Why can't all bubbles be treated similarly? > > Sorry; I should have pinged you on the changes here. > > SystemTray happens to be a subclass of this class. > > The semantics of an alert currently are as follows: > For ChromeVox, it means compute an utterance by concatenating all of the node's > descendant views as if you navigated to each of them linearly. > > For SystemTray, this is undesirable since we really want it to simply read the > name but not all of its descendant views. > > Had an offline discussion with dmazzoni@. One option, taken here, is to > explicitly mark all views that are actually alert dialogs as having role alert > dialog and triggering the alert event for alert dialogs only. A different event > should be fired for the "other" cases to distinguish it for clients such as > ChromeVox. > > Note that I spent some time exploring other strategies, but due to the > strictness of interactive_ui_tests --gtest_filter=*SpokenFeedbackTests*.*, this > was the minimal change possible without breaking lots of test assumptions. What actually toggles the ChromeVox behavior between "an alert" and a non-alert bubble or dialog? Is it simply the AX_EVENT_ALERT type sent here, the AX_ROLE_ALERT_DIALOG role, or are both required? What plans do you have, if any, to audit other bubbles and opt them into the alert pattern? Can you file a bug with sufficient background/context for someone else to follow up, if an audit is warranted? I'm trying to decide how much of this CL is widely applicable to BubbleDelegateView, or whether it should mostly be limited to PermissionsBubbleDelegateView. In either case, the distinction between "alert dialog" and "dialog" behavior is non-obvious and deserves a comment somewhere.
Message was sent while issue was closed.
I'll go ahead and revert. https://codereview.chromium.org/1055883002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1055883002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_delegate.cc:312: if (state.role == ui::AX_ROLE_ALERT_DIALOG) On 2015/04/07 00:18:22, msw wrote: > On 2015/04/06 23:34:02, David Tseng wrote: > > On 2015/04/06 23:01:20, msw wrote: > > > Why did you add this condition? Why can't all bubbles be treated similarly? > > > > Sorry; I should have pinged you on the changes here. > > > > SystemTray happens to be a subclass of this class. > > > > The semantics of an alert currently are as follows: > > For ChromeVox, it means compute an utterance by concatenating all of the > node's > > descendant views as if you navigated to each of them linearly. > > > > For SystemTray, this is undesirable since we really want it to simply read the > > name but not all of its descendant views. > > > > Had an offline discussion with dmazzoni@. One option, taken here, is to > > explicitly mark all views that are actually alert dialogs as having role alert > > dialog and triggering the alert event for alert dialogs only. A different > event > > should be fired for the "other" cases to distinguish it for clients such as > > ChromeVox. > > > > Note that I spent some time exploring other strategies, but due to the > > strictness of interactive_ui_tests --gtest_filter=*SpokenFeedbackTests*.*, > this > > was the minimal change possible without breaking lots of test assumptions. > > What actually toggles the ChromeVox behavior between "an alert" and a non-alert > bubble or dialog? Is it simply the AX_EVENT_ALERT type sent here, the > AX_ROLE_ALERT_DIALOG role, or are both required? Currently, just the alert event. > > What plans do you have, if any, to audit other bubbles and opt them into the > alert pattern? Can you file a bug with sufficient background/context for someone > else to follow up, if an audit is warranted? Done. crbug.com/474622 > > I'm trying to decide how much of this CL is widely applicable to > BubbleDelegateView, or whether it should mostly be limited to > PermissionsBubbleDelegateView. > > In either case, the distinction between "alert dialog" and "dialog" behavior is > non-obvious and deserves a comment somewhere. The distinction comes from ARIA: http://www.w3.org/TR/wai-aria/roles#alertdialog See bug above for a fuller writeup.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1066973002/ by dtseng@chromium.org. The reason for reverting is: Not lg'ed by OWNER as of latest patchset.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
