Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
266963002:
Expose an accessible relation between the main window and active alert. (Closed)
|
Created:
6 years, 7 months ago by dmazzoni Modified:
6 years, 7 months ago Reviewers:
David Tseng CC:
chromium-reviews, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, tfarina, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ia2_1-3 Visibility:
Public. |
DescriptionExpose an accessible relation between the main window and active alert.
This will specifically allow screen readers to query the accessible
object for the main window and quickly get a list of an alerts that
are visible - for example infobars, bubbles, and the Chrome menu itself
if it has an alert icon in it.
BUG=369903
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270074
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address feedback' #
Total comments: 12
Patch Set 3 : Now with unit test #
Total comments: 13
Patch Set 4 : Address feedback #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.cc:897: (*targets)[*n_targets] = view->GetNativeViewAccessible(); This will put targets in *targets in chronological order, right? Is that what we want (as opposed to reverse chronological order)? Alternatively, if it is chronological order we want, do we want to start at max(0, alert_target_view_storage_ids_.size() - max_targets) to avoid missing the most recent (and thus presumably most relevant) alerts in favour of older alerts? https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.cc:1460: if (view == view_) Similarly, do we want to bump up this view in the alert_target_view_storage_ids_ list if this method gets called again? https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.cc:1476: alert_target_view_storage_ids_.begin() + i); Why don't we want to increment i in this case? https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... File ui/views/accessibility/native_view_accessibility_win.h (right): https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.h:42: public IDispatchImpl<IAccessible2_2, &IID_IAccessible2_2, Why all the _2s? https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.h:415: // Remove this view from alert_target_view_storage_ids_. Nit: Removes
https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.cc:897: (*targets)[*n_targets] = view->GetNativeViewAccessible(); On 2014/05/07 15:26:30, aboxhall wrote: > This will put targets in *targets in chronological order, right? Is that what we > want (as opposed to reverse chronological order)? Alternatively, if it is > chronological order we want, do we want to start at max(0, > alert_target_view_storage_ids_.size() - max_targets) to avoid missing the most > recent (and thus presumably most relevant) alerts in favour of older alerts? I hadn't thought about the order. I'm not even sure what we want, I think it may be more up to the screen reader to decide what to do. I think NV Access was thinking of announcing something like "There are <n> alerts" if there's more than 1. I can see an argument for window order / view order rather than chronological, too. I'd prefer to keep the code simple until we know what we want. https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.cc:1476: alert_target_view_storage_ids_.begin() + i); On 2014/05/07 15:26:30, aboxhall wrote: > Why don't we want to increment i in this case? We're erasing the ith element, which should shift everything over and then we should check the new ith element (which used to be the i+1th element). Please check my logic. Is there a more foolproof way to do this? I guess if order doesn't matter I could use a set...
Fixed issue with who allocates the memory as discussed offline. Ready for another look. The spec is here, make sure I didn't miss anything: http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_... https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... File ui/views/accessibility/native_view_accessibility_win.h (right): https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.h:42: public IDispatchImpl<IAccessible2_2, &IID_IAccessible2_2, On 2014/05/07 15:26:30, aboxhall wrote: > Why all the _2s? Believe it or not, that's what they call the extension to the latest extension to the IAccessible2 type in the iaccessible2 umbrella spec - it's IAccessible2_2. We also now have IAccessibleTable2, IAccessibleText2, and so on. Pretty ugly! Basically they can't add new interfaces to an existing abstract class in each new spec version without breaking old code, so they have to subclass it with a new abstract class each time. http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/ https://codereview.chromium.org/266963002/diff/1/ui/views/accessibility/nativ... ui/views/accessibility/native_view_accessibility_win.h:415: // Remove this view from alert_target_view_storage_ids_. On 2014/05/07 15:26:30, aboxhall wrote: > Nit: Removes Done.
+dtseng to finish the review
https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:886: return S_FALSE; Is this string defined in any header? https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:901: if (n_targets) If this isn't true, we should return E_INVALIDARG early on right (as well as targets below)? https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:909: if (max_targets > 0 && count > max_targets) The below if clause doesn't do what this comment suggests -- to set max_targets to count when it is 0. https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:911: What happens if max_targets > count? https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.h (right): https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.h:444: Why is this static? Shouldn't the lifetime of this list of ids be tied to this instance?
https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:1496: alert_target_view_storage_ids_.erase( When would you add a view == NULL?
Ready for another look. I added a unit test so we can feel more confident the logic is right. https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:886: return S_FALSE; On 2014/05/08 17:23:55, David Tseng wrote: > Is this string defined in any header? This is a new convention with NV Access, we want to try it and then propose it for IAccessible2 v1.4. https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:901: if (n_targets) On 2014/05/08 17:23:55, David Tseng wrote: > If this isn't true, we should return E_INVALIDARG early on right (as well as > targets below)? Done. https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:909: if (max_targets > 0 && count > max_targets) On 2014/05/08 17:23:55, David Tseng wrote: > The below if clause doesn't do what this comment suggests -- to set max_targets > to count when it is 0. I think this is right, please take another look. I clarified the comment. https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:911: On 2014/05/08 17:23:55, David Tseng wrote: > What happens if max_targets > count? It should return |count| targets. https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:1496: alert_target_view_storage_ids_.erase( On 2014/05/08 17:53:42, David Tseng wrote: > When would you add a view == NULL? We're using ViewStorage, which automatically nulls out views when they're deleted. This is needed because there's a period of time when the view could be null but the NativeViewAccessibilityWin might still have a reference to it so this hasn't been called yet. https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.h (right): https://codereview.chromium.org/266963002/diff/20001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.h:444: On 2014/05/08 17:23:55, David Tseng wrote: > Why is this static? Shouldn't the lifetime of this list of ids be tied to this > instance? Every view has its own instance of this class. There shouldn't be that many alerts. It seemed simpler to keep track of all alerts, globally, and then return the ones associated with a window on-demand. Would a comment make that more clear? If you'd prefer, the alternative would be to map each alert to its root view, then keep track of it there - the downside is we'd either need a special class for the root view, or a field that only applies to that view.
PTAL
lgtm https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:918: if (targets) { Didn't you check this above? https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:1498: alert_target_view_storage_ids_.begin() + i); break? (since you're never duplicating ids) on insertion. https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win_unittest.cc (right): https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win_unittest.cc:32: view_accessible.QueryInterface(service_provider.Receive()); Dup with below? https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win_unittest.cc:33: ASSERT_EQ(view_accessible.QueryInterface(service_provider.Receive()), S_OK); (S_OK, ...); i.e. ASSERT_EQ(expected, actual) https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win_unittest.cc:161: GetIAccessible2InterfaceForView(root_view, root_view_accessible.Receive()); ASSERT_EQ? https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win_unittest.cc:194: CoTaskMemFree(targets); Odd to see this freed after each call; is this correct?
https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:918: if (targets) { On 2014/05/13 01:34:54, David Tseng wrote: > Didn't you check this above? Done. https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win_unittest.cc (right): https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win_unittest.cc:32: view_accessible.QueryInterface(service_provider.Receive()); On 2014/05/13 01:34:54, David Tseng wrote: > Dup with below? Done. https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win_unittest.cc:33: ASSERT_EQ(view_accessible.QueryInterface(service_provider.Receive()), S_OK); On 2014/05/13 01:34:54, David Tseng wrote: > (S_OK, ...); i.e. ASSERT_EQ(expected, actual) Done. https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win_unittest.cc:161: GetIAccessible2InterfaceForView(root_view, root_view_accessible.Receive()); On 2014/05/13 01:34:54, David Tseng wrote: > ASSERT_EQ? It asserts inside this helper function. https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win_unittest.cc:194: CoTaskMemFree(targets); On 2014/05/13 01:34:54, David Tseng wrote: > Odd to see this freed after each call; is this correct? Yep, that's what the spec says. The server allocates it, the caller frees it. I guess the idea is that it's better than requiring multiple calls - one to get the number of targets, and then more to retrieve each target.
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/266963002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 270074
Message was sent while issue was closed.
https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:1498: alert_target_view_storage_ids_.begin() + i); On 2014/05/13 01:34:54, David Tseng wrote: > break? (since you're never duplicating ids) on insertion. FYI (unresolved comment); not really a big deal since likely very few alerts.
Message was sent while issue was closed.
https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/266963002/diff/30001/ui/views/accessibility/n... ui/views/accessibility/native_view_accessibility_win.cc:1498: alert_target_view_storage_ids_.begin() + i); On 2014/05/13 13:47:11, David Tseng wrote: > On 2014/05/13 01:34:54, David Tseng wrote: > > break? (since you're never duplicating ids) on insertion. > > FYI (unresolved comment); not really a big deal since likely very few alerts. Sorry, forgot to respond. I don't want to break because it's a good excuse to clear out any views that were deleted but whose NativeViewAccessibility hasn't been dereferenced yet. If we did a break we'd have to handle NULL separately from (view == view_) Hope that's okay.
Still lgtm. Thanks for clarifying. On Tue, May 13, 2014 at 8:52 AM, <dmazzoni@chromium.org> wrote: > > https://codereview.chromium.org/266963002/diff/30001/ui/ > views/accessibility/native_view_accessibility_win.cc > File ui/views/accessibility/native_view_accessibility_win.cc (right): > > https://codereview.chromium.org/266963002/diff/30001/ui/ > views/accessibility/native_view_accessibility_win.cc#newcode1498 > ui/views/accessibility/native_view_accessibility_win.cc:1498: > alert_target_view_storage_ids_.begin() + i); > On 2014/05/13 13:47:11, David Tseng wrote: > >> On 2014/05/13 01:34:54, David Tseng wrote: >> > break? (since you're never duplicating ids) on insertion. >> > > FYI (unresolved comment); not really a big deal since likely very few >> > alerts. > > Sorry, forgot to respond. I don't want to break because it's a good > excuse to clear out any views that were deleted but whose > NativeViewAccessibility hasn't been dereferenced yet. If we did a break > we'd have to handle NULL separately from (view == view_) > > Hope that's okay. > > https://codereview.chromium.org/266963002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
