Do not activate TrayBubbleView by default

Do not activate TrayBubbleView by default

- Activating a TrayBubbleView brings an Android window to onPause state.
- This CL sets can_set_activate to false and makes TrayBubbleView not
  activated by default.
- If the TrayBubbleView is not activated, it cannot capture key events
  for moving focus or closing the view by keyboard. TrayBubbleView tries
  to register accelerators at the global level to capture those key
  events.
- Activates the TrayBubbleView by default if keyboard navigation
  accessibility feature (e.g. spoken feedback) is enabled.

BUG=726588, 731748
TEST=Follow steps described in the issue

Review-Url: https://codereview.chromium.org/2897553002
Cr-Commit-Position: refs/heads/master@{#481421}
Committed: https://chromium.googlesource.com/chromium/src/+/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c

[yawano, 2017-05-19 03:31:50]

yawano@chromium.org changed reviewers:
+ dmazzoni@chromium.org, stevenjb@chromium.org

[yawano, 2017-05-19 03:31:50]

PTAL.

stevenjb@: All files
dmazzoni@: Accessibility part. This CL changes TrayBubbleView to put focus on
the default item if spoken feedback is enabled.

Thank you!

This CL is going to do a similar thing which we did at
https://crrev.com/2330403002

[yawano, 2017-05-19 04:24:09]

Uploaded new patch set to add comment.

[dmazzoni, 2017-05-19 19:34:41]

Normally we don't change focus behavior just based on
spoken feedback. Is the issue with focusing the default
view, or with activating the popup window, or both?

Is it possible that a user who doesn't have spoken
feedback on could encounter this bug too? How does it
work if someone's trying to only use the keyboard
and not the mouse, but they don't have any accessibility
turned on?

It looks like TrayBubbleView is used outside of just
Chrome OS, which is why you needed the extra plumbing
to be able to call IsSpokenFeedbackEnabled.

We're also coming out with other accessibility features
like Switch Access that would also benefit from better
focus management. 

Both of those suggest that we should maybe have a more
general test to see if focus behavior should be different,
like if any relevant accessibility service is enabled.
Maybe we should call it something like
ShouldEnableExtraKeyboardAccessibility() or something
like that, what do you think?

[yawano, 2017-05-22 08:54:50]

I agree that it's better not to change focus behavior depending on whether user
is using particular accessibility feature or not.

This issue is activating the popup window. We don't want to activate the popup
when it opens.

For keyboard navigation users, this CL doesn't resolve the issue. I've filed one
for keyboard navigation case. https://crbug.com/724365

stevenjb@, oshima@: Would it be possible to move focus without activating a
widget? If there is such way, we should be able to fix this issue in better way.

[yawano, 2017-05-26 04:49:13]

Description was changed from

==========
Do not activate TrayBubbleView by default

BUG=723995
TEST=Follow steps described in the issue
==========

to

==========
Do not activate TrayBubbleView by default

BUG=726588
TEST=Follow steps described in the issue
==========

[yawano, 2017-05-26 04:59:44]

dmazzoni@: I've filed another bug to clarify what I want to do with this CL at
https://crbug.com/726588.
Changed BUG= field in the CL description.

The issue is that if we activate the tray view, an Android application will
pause.
We want to avoid that. We already fixed it for the system tray at
https://crrev.com/2330403002.
This CL is to apply similar thing for other trays as well.

The problem we have here is that, IIUC, we cannot put focus on non-activated
element.
It means that we need to activate it if user is going to navigate with keyboard.

Yes, ShouldEnableExtraKeyboardAccessibility sounds good for me.
It's more general and it works well with other types of accessibility services
too.
I'll rename the method.

[yawano, 2017-05-26 07:11:51]

Description was changed from

==========
Do not activate TrayBubbleView by default

BUG=726588
TEST=Follow steps described in the issue
==========

to

==========
Do not activate TrayBubbleView by default

BUG=726588
TEST=Follow steps described in the issue
==========

[yawano, 2017-05-26 07:13:46]

Please wait to review this CL for now. I found that this CL doesn't work with
keyboard navigation. ui::View::OnKeyReleased cannot be used for this case as the
view is not activated. I'm finding a solution now.

[yawano, 2017-06-07 04:07:01]

This CL is now ready for review again.

stevenjb@, dmazzoni@, PTAL. Thank you!

[stevenjb, 2017-06-08 16:32:45]

At a quick glance it looks like the overridden methods just do the same thing as
the base implementation, am I missing something?

[yawano, 2017-06-09 01:20:48]

On 2017/06/08 16:32:45, stevenjb wrote:
> At a quick glance it looks like the overridden methods just do the same thing
as
> the base implementation, am I missing something?

Which methods are you talking about?

What we want to do in this CL is

- Set can_activate of TrayBubbleView to false by default. Do not activate the
tray bubble view when user opens it.

But we cannot simply flip the bit because keyboard navigation doesn't works if
the view is not activated.
The view doesn't get key events if it's not activated. To fix it, we are
handling it by ourselves.
Accelerators in TrayBubbleView are expected to be registered at the global
level. Those accelerators will work even if the view is not activated.

Also once keyboard focus has moved into the tray bubble view, the view is
activated.
The base implementation of keyboard navigation handling becomes to work. We
unregister our accelerators in that case.

[stevenjb, 2017-06-09 15:34:59]

stevenjb@chromium.org changed reviewers:
+ jamescook@chromium.org, msw@chromium.org

[stevenjb, 2017-06-09 15:35:01]

Apologies, lots going on this week so when I looked at this before it was a bit
too quickly.

I was confused because SystemTray, PaletteTray, WebNotificationTray, and
ImeMenuTray all implement views::TrayBubbleView::Delegate, and all do the same
thing in the new delegate methods, but do not actually share a common class.

This is probably OK as-is, but, there is quite a bit of copy/paste code in these
4 classes already. I'm wondering if we could move some of it to a common base
class or a common delegate (and maybe move that to a member)?

I haven't really been in views code lately and I'm not really the best owner for
this any more, so +msw@, except he is OOO, so also +jamescook@, and take my
feedback with a grain of salt.

[James Cook, 2017-06-09 16:06:13]

Please write a more detailed CL description, including why you are making the
change and a brief summary of what you did.

Options for the copy/pastes:
* Split the TrayBubbleView::Delegate into Delegate and KeyboardDelegate and have
a shared ash::TrayBubbleViewKeyboardDelegate object all these classes use.
* Keep them, since the code might be easier to understand that way

Steven, I think the right solution here is for TrayBubbleView to move out of
ui/views/bubble and into ash/system/tray. Ash is the only user and it would
remove all this delegation. It's not completely trivial, though, due to message
center bubbles (which ought to move into ash also, imho). But that's a problem
for bubble and ash OWNERS. I filed crbug.com/731748 to discuss.

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
ash/system/tray/system_tray.cc:439: // If ChromeVox is enabled, focus the
default item if no item is focused.
Is this comment still needed?

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
ash/system/tray/system_tray.h:21: class KeyEventWatcher;
Still needed?

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
ash/system/tray/system_tray.h:172: void CreateKeyEventWatcher();
Didn't you delete this function?

https://codereview.chromium.org/2897553002/diff/140001/ash/system/web_notific...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2897553002/diff/140001/ash/system/web_notific...
ash/system/web_notification/web_notification_tray.cc:433: bool
WebNotificationTray::ShouldEnableExtraKeyboardAccessibility() {
nit: functions in .cc should match order in header

https://codereview.chromium.org/2897553002/diff/140001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.cc (left):

https://codereview.chromium.org/2897553002/diff/140001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:199: set_can_activate(params_.can_activate);
Is TrayBubbleView::InitParams::can_activate still used anywhere? If not, remove
it.

https://codereview.chromium.org/2897553002/diff/140001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/2897553002/diff/140001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:237: // If TrayBubbleView cannot be
activated, register accelerators to activate
Comment says "activate it" but list contains Escape, which I presume deactivates
it. Clarify comment?

https://codereview.chromium.org/2897553002/diff/140001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:405: } else if (accelerator ==
ui::Accelerator(ui::VKEY_TAB, ui::EF_NONE) ||
nit: no "else" after return

[yawano, 2017-06-12 09:03:38]

Description was changed from

==========
Do not activate TrayBubbleView by default

BUG=726588
TEST=Follow steps described in the issue
==========

to

==========
Do not activate TrayBubbleView by default

- Activating a TrayBubbleView brings an Android window to onPause state.
- This CL sets can_set_activate to false and makes TrayBubbleView not
  activated by default.
- If the TrayBubbleView is not activated, it cannot capture key events
  for moving focus or closing the view by keyboard. TrayBubbleView tries
  to register accelerators at the global level to capture those key
  events.
- Activates the TrayBubbleView by default if keyboard navigation
  accessibility feature (e.g. spoken feedback) is enabled.

BUG=726588
TEST=Follow steps described in the issue
==========

[yawano, 2017-06-12 09:11:04]

I prefer to keep the copy/paste codes in each use case implementations (e.g.
ime_menu_tray). Those codes aren't much. Those are a few tens of lines at most.
The copy/paste codes issue will be resolved once we moved TrayBubbleView into
ash/system/tray.

Addressed the comments, and fixed a few issues I found (e.g. Remove a line from
AcceleratorController which activated the system tray if it's open from
accelerator).

PTAL. Thank you!

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
ash/system/tray/system_tray.cc:439: // If ChromeVox is enabled, focus the
default item if no item is focused.
No longer necessary. Deleted.

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
ash/system/tray/system_tray.h:21: class KeyEventWatcher;
No, SystemTray no longer uses it. Removed.

https://codereview.chromium.org/2897553002/diff/140001/ash/system/tray/system...
ash/system/tray/system_tray.h:172: void CreateKeyEventWatcher();
Yes, I've deleted the function. Thank you for catching this!

https://codereview.chromium.org/2897553002/diff/140001/ash/system/web_notific...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2897553002/diff/140001/ash/system/web_notific...
ash/system/web_notification/web_notification_tray.cc:433: bool
WebNotificationTray::ShouldEnableExtraKeyboardAccessibility() {
On 2017/06/09 16:06:13, James Cook wrote:
> nit: functions in .cc should match order in header

Done.

https://codereview.chromium.org/2897553002/diff/140001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/2897553002/diff/140001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:237: // If TrayBubbleView cannot be
activated, register accelerators to activate
On 2017/06/09 16:06:13, James Cook wrote:
> Comment says "activate it" but list contains Escape, which I presume
deactivates
> it. Clarify comment?

Done.

https://codereview.chromium.org/2897553002/diff/140001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:405: } else if (accelerator ==
ui::Accelerator(ui::VKEY_TAB, ui::EF_NONE) ||
On 2017/06/09 16:06:13, James Cook wrote:
> nit: no "else" after return

Done.

[James Cook, 2017-06-12 16:38:20]

LGTM. Please add crbug.com/731748 to the BUG= line and comment in that bug about
which delegates can be reviewed.

Nice CL!

[stevenjb, 2017-06-12 16:43:14]

As long as we have a plan to remove/reduce the duplicate code, lgtm.

[dmazzoni, 2017-06-12 17:00:47]

The only think I'm slightly worried about
is that we're changing the behavior based on whether
ChromeVox is enabled.

There are other users who rely on the keyboard, too.
We also have another accessibility service in progress,
Switch Access, that's for users who can only press a
few keys.

What happens with this change if someone can't use a
mouse or trackpad but they don't have any accessibility
services enabled?

[yawano, 2017-06-13 01:03:45]

On 2017/06/12 17:00:47, dmazzoni wrote:
> The only think I'm slightly worried about
> is that we're changing the behavior based on whether
> ChromeVox is enabled.
> 
> There are other users who rely on the keyboard, too.
> We also have another accessibility service in progress,
> Switch Access, that's for users who can only press a
> few keys.
> 
> What happens with this change if someone can't use a
> mouse or trackpad but they don't have any accessibility
> services enabled?

User can still navigate by keyboard without enabling any accessibility service.
User can open system tray by Alt+Shift+S. The tray bubble view is not activated
at this time.
But once user presses Tab key, the focus moves to the tray bubble view and user
can navigate on the system tray by pressing Tab or Shift+Tab.
The tray bubble view is activated once the focus moves to the tray bubble view.

The reason we are activating the tray bubble view by default if spoken feedback
is enabled is that ChromeVox won't provide a feedback if we don't activate the
tray.
For ChromeVox, we want to activate the tray bubble view to let accessibility
service know that the bubble view has opened.

Does Switch Access auto scan includes an element which is not activated?
If they don't, we need to activate the tray bubble view by default for switch
access as well.

[yawano, 2017-06-13 06:28:44]

Description was changed from

==========
Do not activate TrayBubbleView by default

- Activating a TrayBubbleView brings an Android window to onPause state.
- This CL sets can_set_activate to false and makes TrayBubbleView not
  activated by default.
- If the TrayBubbleView is not activated, it cannot capture key events
  for moving focus or closing the view by keyboard. TrayBubbleView tries
  to register accelerators at the global level to capture those key
  events.
- Activates the TrayBubbleView by default if keyboard navigation
  accessibility feature (e.g. spoken feedback) is enabled.

BUG=726588
TEST=Follow steps described in the issue
==========

to

==========
Do not activate TrayBubbleView by default

- Activating a TrayBubbleView brings an Android window to onPause state.
- This CL sets can_set_activate to false and makes TrayBubbleView not
  activated by default.
- If the TrayBubbleView is not activated, it cannot capture key events
  for moving focus or closing the view by keyboard. TrayBubbleView tries
  to register accelerators at the global level to capture those key
  events.
- Activates the TrayBubbleView by default if keyboard navigation
  accessibility feature (e.g. spoken feedback) is enabled.

BUG=726588
TEST=Follow steps described in the issue
==========

[yawano, 2017-06-14 05:54:13]

Description was changed from

==========
Do not activate TrayBubbleView by default

- Activating a TrayBubbleView brings an Android window to onPause state.
- This CL sets can_set_activate to false and makes TrayBubbleView not
  activated by default.
- If the TrayBubbleView is not activated, it cannot capture key events
  for moving focus or closing the view by keyboard. TrayBubbleView tries
  to register accelerators at the global level to capture those key
  events.
- Activates the TrayBubbleView by default if keyboard navigation
  accessibility feature (e.g. spoken feedback) is enabled.

BUG=726588
TEST=Follow steps described in the issue
==========

to

==========
Do not activate TrayBubbleView by default

- Activating a TrayBubbleView brings an Android window to onPause state.
- This CL sets can_set_activate to false and makes TrayBubbleView not
  activated by default.
- If the TrayBubbleView is not activated, it cannot capture key events
  for moving focus or closing the view by keyboard. TrayBubbleView tries
  to register accelerators at the global level to capture those key
  events.
- Activates the TrayBubbleView by default if keyboard navigation
  accessibility feature (e.g. spoken feedback) is enabled.

BUG=726588,731748
TEST=Follow steps described in the issue
==========

[yawano, 2017-06-14 05:55:06]

On 2017/06/12 16:38:20, James Cook wrote:
> LGTM. Please add crbug.com/731748 to the BUG= line and comment in that bug
about
> which delegates can be reviewed.
> 
> Nice CL!

Thank you. Added 731748 in BUG= line.

[yawano, 2017-06-15 00:39:53]

On 2017/06/13 01:03:45, yawano wrote:
> On 2017/06/12 17:00:47, dmazzoni wrote:
> > The only think I'm slightly worried about
> > is that we're changing the behavior based on whether
> > ChromeVox is enabled.
> > 
> > There are other users who rely on the keyboard, too.
> > We also have another accessibility service in progress,
> > Switch Access, that's for users who can only press a
> > few keys.
> > 
> > What happens with this change if someone can't use a
> > mouse or trackpad but they don't have any accessibility
> > services enabled?
> 
> User can still navigate by keyboard without enabling any accessibility
service.
> User can open system tray by Alt+Shift+S. The tray bubble view is not
activated
> at this time.
> But once user presses Tab key, the focus moves to the tray bubble view and
user
> can navigate on the system tray by pressing Tab or Shift+Tab.
> The tray bubble view is activated once the focus moves to the tray bubble
view.
> 
> The reason we are activating the tray bubble view by default if spoken
feedback
> is enabled is that ChromeVox won't provide a feedback if we don't activate the
> tray.
> For ChromeVox, we want to activate the tray bubble view to let accessibility
> service know that the bubble view has opened.
> 
> Does Switch Access auto scan includes an element which is not activated?
> If they don't, we need to activate the tray bubble view by default for switch
> access as well.

dmazzoni@: ping.

[dmazzoni, 2017-06-15 06:05:06]

On 2017/06/15 00:39:53, yawano wrote:
> On 2017/06/13 01:03:45, yawano wrote:
> > On 2017/06/12 17:00:47, dmazzoni wrote:
> > > The only think I'm slightly worried about
> > > is that we're changing the behavior based on whether
> > > ChromeVox is enabled.
> > > 
> > > There are other users who rely on the keyboard, too.
> > > We also have another accessibility service in progress,
> > > Switch Access, that's for users who can only press a
> > > few keys.
> > > 
> > > What happens with this change if someone can't use a
> > > mouse or trackpad but they don't have any accessibility
> > > services enabled?
> > 
> > User can still navigate by keyboard without enabling any accessibility
> service.
> > User can open system tray by Alt+Shift+S. The tray bubble view is not
> activated
> > at this time.
> > But once user presses Tab key, the focus moves to the tray bubble view and
> user
> > can navigate on the system tray by pressing Tab or Shift+Tab.
> > The tray bubble view is activated once the focus moves to the tray bubble
> view.

Got it. So it's initially shown but *not* activated, but once you press Tab
and focus moves inside, it does get activated.

> > The reason we are activating the tray bubble view by default if spoken
> feedback
> > is enabled is that ChromeVox won't provide a feedback if we don't activate
the
> > tray.
> > For ChromeVox, we want to activate the tray bubble view to let accessibility
> > service know that the bubble view has opened.

OK, makes sense

> > Does Switch Access auto scan includes an element which is not activated?
> > If they don't, we need to activate the tray bubble view by default for
switch
> > access as well.

Switch Access won't be broken, but it may not be ideal. Switch Access lets you
navigate to anything focusable on the whole screen, so that sounds like it will
work, and that's good. However, it automatically follows when focus changes,
and I suspect it will not follow when the tray bubble view is opened if it
doesn't
get activated. However the user will still be able to navigate to it.

We can revisit this later when Switch Access is ready for launch to see if we
want to improve it, but for now it's fine that it's not going to be completely
broken.

Thanks for the explanations and your hard work on this patch.

lgtm

[yawano, 2017-06-15 06:44:38]

On 2017/06/15 06:05:06, dmazzoni wrote:
> On 2017/06/15 00:39:53, yawano wrote:
> > On 2017/06/13 01:03:45, yawano wrote:
> > > On 2017/06/12 17:00:47, dmazzoni wrote:
> > > > The only think I'm slightly worried about
> > > > is that we're changing the behavior based on whether
> > > > ChromeVox is enabled.
> > > > 
> > > > There are other users who rely on the keyboard, too.
> > > > We also have another accessibility service in progress,
> > > > Switch Access, that's for users who can only press a
> > > > few keys.
> > > > 
> > > > What happens with this change if someone can't use a
> > > > mouse or trackpad but they don't have any accessibility
> > > > services enabled?
> > > 
> > > User can still navigate by keyboard without enabling any accessibility
> > service.
> > > User can open system tray by Alt+Shift+S. The tray bubble view is not
> > activated
> > > at this time.
> > > But once user presses Tab key, the focus moves to the tray bubble view and
> > user
> > > can navigate on the system tray by pressing Tab or Shift+Tab.
> > > The tray bubble view is activated once the focus moves to the tray bubble
> > view.
> 
> Got it. So it's initially shown but *not* activated, but once you press Tab
> and focus moves inside, it does get activated.
> 
> > > The reason we are activating the tray bubble view by default if spoken
> > feedback
> > > is enabled is that ChromeVox won't provide a feedback if we don't activate
> the
> > > tray.
> > > For ChromeVox, we want to activate the tray bubble view to let
accessibility
> > > service know that the bubble view has opened.
> 
> OK, makes sense
> 
> > > Does Switch Access auto scan includes an element which is not activated?
> > > If they don't, we need to activate the tray bubble view by default for
> switch
> > > access as well.
> 
> Switch Access won't be broken, but it may not be ideal. Switch Access lets you
> navigate to anything focusable on the whole screen, so that sounds like it
will
> work, and that's good. However, it automatically follows when focus changes,
> and I suspect it will not follow when the tray bubble view is opened if it
> doesn't
> get activated. However the user will still be able to navigate to it.
> 
> We can revisit this later when Switch Access is ready for launch to see if we
> want to improve it, but for now it's fine that it's not going to be completely
> broken.
> 
> Thanks for the explanations and your hard work on this patch.
> 
> lgtm

Thank you!

[yawano, 2017-06-15 06:45:09]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2017-06-15 06:45:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 08:10:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 08:10:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yawano, 2017-06-16 04:44:06]

The CQ bit was checked by yawano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 04:44:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 06:23:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 06:23:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yawano, 2017-06-20 07:08:22]

The CQ bit was checked by yawano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 07:08:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 07:59:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 07:59:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yawano, 2017-06-20 09:03:05]

The CQ bit was checked by yawano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 09:03:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yawano, 2017-06-20 09:30:48]

The CQ bit was checked by yawano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 09:31:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yawano, 2017-06-20 10:24:53]

The CQ bit was checked by yawano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 10:25:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yawano, 2017-06-20 10:36:18]

Patch set 10 has failed to pass test cases.

PTAL again.

stevenjb@: all files.
dmazzoni@: accessibility part. Especially
SystemTray::ShouldEnableExtraKeyboardAccessibility. I've changed it not to
enable extra keyboard accessibility for persistent system bubble.

Thank you!

[commit-bot: I haz the power, 2017-06-20 11:26:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 11:26:13]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-06-20 16:07:55]

Delta from PS 10 lgtm

[dmazzoni, 2017-06-20 16:32:54]

still lgtm with one suggestion

https://codereview.chromium.org/2897553002/diff/270001/ash/system/tray/system...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2897553002/diff/270001/ash/system/tray/system...
ash/system/tray/system_tray.cc:587: // Do not enable extra keyboard
accessibility for persistent system bubble.
Maybe the comment should mention spoken feedback?

// Do not enable extra keyboard accessibility for persistent system bubbles,
// which are those that are not closed even if the user clicks outside
// of the bubble, e.g. the volume slider - but do enable extra keyboard
// accessibility for them if spoken feedback is enabled.

[yawano, 2017-06-21 01:20:04]

Thank you!

https://codereview.chromium.org/2897553002/diff/270001/ash/system/tray/system...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2897553002/diff/270001/ash/system/tray/system...
ash/system/tray/system_tray.cc:587: // Do not enable extra keyboard
accessibility for persistent system bubble.
On 2017/06/20 16:32:54, dmazzoni wrote:
> Maybe the comment should mention spoken feedback?
> 
> // Do not enable extra keyboard accessibility for persistent system bubbles,
> // which are those that are not closed even if the user clicks outside
> // of the bubble, e.g. the volume slider - but do enable extra keyboard
> // accessibility for them if spoken feedback is enabled.

No, we don't enable extra keyboard accessibility for persistent system bubbles
even if spoken feedback is enabled. Extra keyboard accessibility moves focus to
the first focusable item in persistent system bubble. If we move the focus, it
causes spoken feedback.

My assumption is that for persistent system bubble, user don't want move focus
on to it, e.g. volume slider. Note that user can still move the focus to an
element in volume slider by pressing Tab.

Also this should be same with the current behavior.

[yawano, 2017-06-22 01:13:38]

The CQ bit was checked by yawano@chromium.org

[yawano, 2017-06-22 01:13:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2897553002/#ps270001
(title: "Remove unnecessary code.")

[commit-bot: I haz the power, 2017-06-22 01:13:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 03:07:18]

CQ is committing da patch.

Bot data: {"patchset_id": 270001, "attempt_start_ts": 1498094018943690,
"parent_rev": "67f72caa837f24bbb1595c26363daf09e0200f14", "commit_rev":
"c97f341f1a2f6f359ba1a7c8687aa58af6f7033c"}

[commit-bot: I haz the power, 2017-06-22 03:07:32]

Description was changed from

==========
Do not activate TrayBubbleView by default

- Activating a TrayBubbleView brings an Android window to onPause state.
- This CL sets can_set_activate to false and makes TrayBubbleView not
  activated by default.
- If the TrayBubbleView is not activated, it cannot capture key events
  for moving focus or closing the view by keyboard. TrayBubbleView tries
  to register accelerators at the global level to capture those key
  events.
- Activates the TrayBubbleView by default if keyboard navigation
  accessibility feature (e.g. spoken feedback) is enabled.

BUG=726588,731748
TEST=Follow steps described in the issue
==========

to

==========
Do not activate TrayBubbleView by default

- Activating a TrayBubbleView brings an Android window to onPause state.
- This CL sets can_set_activate to false and makes TrayBubbleView not
  activated by default.
- If the TrayBubbleView is not activated, it cannot capture key events
  for moving focus or closing the view by keyboard. TrayBubbleView tries
  to register accelerators at the global level to capture those key
  events.
- Activates the TrayBubbleView by default if keyboard navigation
  accessibility feature (e.g. spoken feedback) is enabled.

BUG=726588,731748
TEST=Follow steps described in the issue

Review-Url: https://codereview.chromium.org/2897553002
Cr-Commit-Position: refs/heads/master@{#481421}
Committed:
https://chromium.googlesource.com/chromium/src/+/c97f341f1a2f6f359ba1a7c8687a...
==========

[commit-bot: I haz the power, 2017-06-22 03:07:36]

Committed patchset #15 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/c97f341f1a2f6f359ba1a7c8687a...

[vabr (Chromium), 2017-06-22 09:02:35]

A revert of this CL (patchset #15 id:270001) has been created in
https://codereview.chromium.org/2948223002/ by vabr@chromium.org.

The reason for reverting is: Speculative revert. I suspect this broke the
interactive_ui_tests on Linux ChromiumOS MSan Tests
(https://build.chromium.org/p/chromium.memory/builders/Linux%20ChromiumOS%20MS...).
The tests now time out, and the log contains lines with:

Still waiting for the following processes to finish:
	./interactive_ui_tests --brave-new-test-launcher --cfi-diag=0
--gtest_also_run_disabled_tests
--gtest_filter=TestAsNormalAndGuestUser/SpokenFeedbackTest.NavigateSystemTray/0
--single_process --test-launcher-bot-mode
--test-launcher-print-test-stdio=always
--test-launcher-summary-output=/b/s/w/ioS9oZeu/output.json
--user-data-dir=/b/s/w/it1bN0Em/.org.chromium.Chromium.iOqw8e/dOj481Q
.

[vabr (Chromium), 2017-06-22 11:21:16]

On 2017/06/22 09:02:35, vabr (Chromium) wrote:
> A revert of this CL (patchset #15 id:270001) has been created in
> https://codereview.chromium.org/2948223002/ by mailto:vabr@chromium.org.
> 
> The reason for reverting is: Speculative revert. I suspect this broke the
> interactive_ui_tests on Linux ChromiumOS MSan Tests
>
(https://build.chromium.org/p/chromium.memory/builders/Linux%20ChromiumOS%20MS...).
> The tests now time out, and the log contains lines with:
> 
> Still waiting for the following processes to finish:
> 	./interactive_ui_tests --brave-new-test-launcher --cfi-diag=0
> --gtest_also_run_disabled_tests
>
--gtest_filter=TestAsNormalAndGuestUser/SpokenFeedbackTest.NavigateSystemTray/0
> --single_process --test-launcher-bot-mode
> --test-launcher-print-test-stdio=always
> --test-launcher-summary-output=/b/s/w/ioS9oZeu/output.json
> --user-data-dir=/b/s/w/it1bN0Em/.org.chromium.Chromium.iOqw8e/dOj481Q
> .

The revert seems to have fixed the interactive_ui_tests, in
https://build.chromium.org/p/chromium.memory/builders/Linux%20ChromiumOS%20MS...,
so I'm leaving it in.

[yawano, 2017-06-23 10:01:50]

On 2017/06/22 11:21:16, vabr (Chromium) wrote:
> On 2017/06/22 09:02:35, vabr (Chromium) wrote:
> > A revert of this CL (patchset #15 id:270001) has been created in
> > https://codereview.chromium.org/2948223002/ by mailto:vabr@chromium.org.
> > 
> > The reason for reverting is: Speculative revert. I suspect this broke the
> > interactive_ui_tests on Linux ChromiumOS MSan Tests
> >
>
(https://build.chromium.org/p/chromium.memory/builders/Linux%20ChromiumOS%20MS...).
> > The tests now time out, and the log contains lines with:
> > 
> > Still waiting for the following processes to finish:
> > 	./interactive_ui_tests --brave-new-test-launcher --cfi-diag=0
> > --gtest_also_run_disabled_tests
> >
>
--gtest_filter=TestAsNormalAndGuestUser/SpokenFeedbackTest.NavigateSystemTray/0
> > --single_process --test-launcher-bot-mode
> > --test-launcher-print-test-stdio=always
> > --test-launcher-summary-output=/b/s/w/ioS9oZeu/output.json
> > --user-data-dir=/b/s/w/it1bN0Em/.org.chromium.Chromium.iOqw8e/dOj481Q
> > .
> 
> The revert seems to have fixed the interactive_ui_tests, in
>
https://build.chromium.org/p/chromium.memory/builders/Linux%20ChromiumOS%20MS...,
> so I'm leaving it in.

I've investigated the cause and this CL has a bug. I'm going to write a
follow-up CL next week. Thank you!