Don't fire a FOCUS accessibility event on a View that doesn't have focus.

Don't fire a FOCUS accessibility event on a View that doesn't have focus.

Soon firing a focus event on a View won't have any effect if that View
isn't really the focused view (http://crrev.com/1589623002). This is
important so we can filter out events from inactive windows.

There were a few cases where we were firing a FOCUS accessibility event
on a View when it didn't actually have focus. Replace each of these
with another accessibility event that will have the same end result.

BUG=584505
Committed: https://crrev.com/d688790882cbc634e1b831c2eb99f9ffb3fbe307
Cr-Commit-Position: refs/heads/master@{#375809}

[dmazzoni, 2016-02-12 22:37:32]

Description was changed from

==========
Don't fire a FOCUS accessibility event on a View that doesn't have focus.

We want to distinguish between "this is the View that really has input
focus", and things like "this View was selected (but doesn't have focus)".

Soon firing a focus event on a View won't have any effect if that View
isn't really the focused view (http://crrev.com/1589623002). This is
important to filter out events from inactive windows.

There were a few cases where we were firing a FOCUS accessibility event
on a View when it didn't actually have focus. This change replaces this
with another accessibility event that will have the desired effect
while allowing us to keep track of focus more strongly.

BUG=584505
==========

to

==========
Don't fire a FOCUS accessibility event on a View that doesn't have focus.

Soon firing a focus event on a View won't have any effect if that View
isn't really the focused view (http://crrev.com/1589623002). This is
important so we can filter out events from inactive windows.

There were a few cases where we were firing a FOCUS accessibility event
on a View when it didn't actually have focus. Replace each of these
with another accessibility event that will have the same end result.

BUG=584505
==========

[dmazzoni, 2016-02-12 22:37:32]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org, sky@chromium.org

[sky, 2016-02-14 16:21:52]

https://codereview.chromium.org/1693713004/diff/1/ash/wm/overview/window_sele...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/1693713004/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_item.cc:217: ui::AX_EVENT_SELECTION, true);
If the name of this method is correct, why send selection rather than focus?

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_list_view.cc (right):

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_list_view.cc:195:
selected_view->NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
I believe this function is always called when the selection changes. In which
case shouldn't notification be unconditional in this function?

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/start_pag...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/start_pag...
ui/app_list/views/start_page_view.cc:89:
NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
Do you really only want to notify if selected, or if the selection changes?

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/tile_item...
File ui/app_list/views/tile_item_view.cc (right):

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/tile_item...
ui/app_list/views/tile_item_view.cc:56:
NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
Same comment here.

[dmazzoni, 2016-02-17 00:25:19]

https://codereview.chromium.org/1693713004/diff/1/ash/wm/overview/window_sele...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/1693713004/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_item.cc:217: ui::AX_EVENT_SELECTION, true);
On 2016/02/14 16:21:52, sky wrote:
> If the name of this method is correct, why send selection rather than focus?

The name isn't right, the view isn't actually focused. Renamed the method to
SendAccessibleSelectionEvent.

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_list_view.cc (right):

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_list_view.cc:195:
selected_view->NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
On 2016/02/14 16:21:52, sky wrote:
> I believe this function is always called when the selection changes. In which
> case shouldn't notification be unconditional in this function?

We need to fire the selection on a particular item, though. Nothing to fire it
on if there are no results.

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/start_pag...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/1693713004/diff/1/ui/app_list/views/start_pag...
ui/app_list/views/start_page_view.cc:89:
NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
On 2016/02/14 16:21:52, sky wrote:
> Do you really only want to notify if selected, or if the selection changes?

In this particular case a notification when something is selected seems to work
best we need an explicit notification when you use the arrow keys to choose an
app / result, but we don't if you press a key and that unselects an item because
focus is in the text field and pressing the key is already sending accessibility
events there.

Another way to do this would be to make the relationship between the focused
view and the list of results more explicit (e.g., the focused view's accessible
state could include fields like "active descendant" or "selected children"),
then we could just fire a "something changed" type of event and it'd be possible
for the accessibility client to figure out what happened. I was planning on that
as a possible future improvement, but let me know if you'd prefer something like
that now.

[sky, 2016-02-17 01:06:52]

LGTM

[dmazzoni, 2016-02-17 05:29:34]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-02-17 05:29:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1693713004/#ps40001
(title: "Missed one call to SendFocusAlert")

[commit-bot: I haz the power, 2016-02-17 05:33:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693713004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693713004/40001

[commit-bot: I haz the power, 2016-02-17 06:20:40]

Description was changed from

==========
Don't fire a FOCUS accessibility event on a View that doesn't have focus.

Soon firing a focus event on a View won't have any effect if that View
isn't really the focused view (http://crrev.com/1589623002). This is
important so we can filter out events from inactive windows.

There were a few cases where we were firing a FOCUS accessibility event
on a View when it didn't actually have focus. Replace each of these
with another accessibility event that will have the same end result.

BUG=584505
==========

to

==========
Don't fire a FOCUS accessibility event on a View that doesn't have focus.

Soon firing a focus event on a View won't have any effect if that View
isn't really the focused view (http://crrev.com/1589623002). This is
important so we can filter out events from inactive windows.

There were a few cases where we were firing a FOCUS accessibility event
on a View when it didn't actually have focus. Replace each of these
with another accessibility event that will have the same end result.

BUG=584505
==========

[commit-bot: I haz the power, 2016-02-17 06:20:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-17 06:21:28]

Description was changed from

==========
Don't fire a FOCUS accessibility event on a View that doesn't have focus.

Soon firing a focus event on a View won't have any effect if that View
isn't really the focused view (http://crrev.com/1589623002). This is
important so we can filter out events from inactive windows.

There were a few cases where we were firing a FOCUS accessibility event
on a View when it didn't actually have focus. Replace each of these
with another accessibility event that will have the same end result.

BUG=584505
==========

to

==========
Don't fire a FOCUS accessibility event on a View that doesn't have focus.

Soon firing a focus event on a View won't have any effect if that View
isn't really the focused view (http://crrev.com/1589623002). This is
important so we can filter out events from inactive windows.

There were a few cases where we were firing a FOCUS accessibility event
on a View when it didn't actually have focus. Replace each of these
with another accessibility event that will have the same end result.

BUG=584505

Committed: https://crrev.com/d688790882cbc634e1b831c2eb99f9ffb3fbe307
Cr-Commit-Position: refs/heads/master@{#375809}
==========

[commit-bot: I haz the power, 2016-02-17 06:21:29]

Patchset 3 (id:??) landed as
https://crrev.com/d688790882cbc634e1b831c2eb99f9ffb3fbe307
Cr-Commit-Position: refs/heads/master@{#375809}