New issue
Advanced search Search tips

Issue 731748 link

Starred by 2 users
Status: Assigned
Owner:
xiy...@chromium.org
Cc:
msw@chromium.org
yawano@chromium.org
steve...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Move TrayBubbleView out of ui/views/bubble into ash

Project Member Reported by jamescook@chromium.org, Jun 9 2017 
It is only created in ash. Moving it there would allow a bunch of delegates to be removed, and CL's like this would not have to add more:

https://codereview.chromium.org/2897553002/

This isn't 100% trivial, since the declaration in used in ui/message_center/views, but some of that code might want to move also.

OWNERS, WDYT?

Comment 1 by zork@chromium.org, Jun 9 2017

Owner: xiy...@chromium.org
Status: Available (was: Untriaged)

Comment 2 by steve...@chromium.org, Jun 9 2017 

This would probably require a dependency issue/CL to move ui/message_center/views into ash/, which would only be valid if we don't use the message center view on any other platform. Otherwise it would involve moving the system tray specific implementation into src/ash?

Ultimately I agree that is the correct solution; the SystemTray and TrayBubbleView seem like they should be Ash specific in the New Order.

Comment 3 by jamescook@chromium.org, Jun 14 2017 

This would allow some TrayBubbleView::Delegate methods to be removed, specifically:

    virtual void RegisterAccelerators(
         const std::vector<ui::Accelerator>& accelerators,
         TrayBubbleView* tray_bubble_view) = 0;
    virtual void UnregisterAllAccelerators(
         TrayBubbleView* tray_bubble_view) = 0;
    virtual bool ShouldEnableExtraKeyboardAccessibility() = 0;

See https://codereview.chromium.org/2897553002/
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c

commit c97f341f1a2f6f359ba1a7c8687aa58af6f7033c
Author: yawano <yawano@chromium.org>
Date: Thu Jun 22 03:07:12 2017

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}

[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/ime_menu/ime_menu_tray.h
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/palette/palette_tray.cc
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/palette/palette_tray.h
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/tray/system_tray.cc
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/tray/system_tray.h
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/tray/system_tray_bubble.cc
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/tray/system_tray_bubble.h
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/web_notification/web_notification_tray.cc
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ash/system/web_notification/web_notification_tray.h
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ui/message_center/views/message_bubble_base.cc
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ui/views/bubble/tray_bubble_view.cc
[modify] https://crrev.com/c97f341f1a2f6f359ba1a7c8687aa58af6f7033c/ui/views/bubble/tray_bubble_view.h
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0cafb5aa2544670f4313e102e3ff12225dc5dafa

commit 0cafb5aa2544670f4313e102e3ff12225dc5dafa
Author: vabr <vabr@chromium.org>
Date: Thu Jun 22 09:03:27 2017

Revert of Do not activate TrayBubbleView by default (patchset #15 id:270001 of https://codereview.chromium.org/2897553002/ )

Reason for revert:
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%20MSan%20Tests/builds/1276). 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

Original issue's description:
> 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

TBR=stevenjb@chromium.org,dmazzoni@chromium.org,msw@chromium.org,jamescook@chromium.org,yawano@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=726588,731748

Review-Url: https://codereview.chromium.org/2948223002
Cr-Commit-Position: refs/heads/master@{#481485}

[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/ime_menu/ime_menu_tray.h
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/palette/palette_tray.cc
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/palette/palette_tray.h
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/tray/system_tray.cc
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/tray/system_tray.h
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/tray/system_tray_bubble.cc
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/tray/system_tray_bubble.h
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/web_notification/web_notification_tray.cc
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ash/system/web_notification/web_notification_tray.h
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ui/message_center/views/message_bubble_base.cc
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ui/views/bubble/tray_bubble_view.cc
[modify] https://crrev.com/0cafb5aa2544670f4313e102e3ff12225dc5dafa/ui/views/bubble/tray_bubble_view.h
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66c305b497c7f8e2125a05f544fd346862f0bb8e

commit 66c305b497c7f8e2125a05f544fd346862f0bb8e
Author: yawano <yawano@chromium.org>
Date: Tue Jun 27 00:37:14 2017

Reland with a fix: 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.

TBR=dmazzoni@chromium.org, jamescook@chromium.org
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

patch from issue 2897553002 at patchset 270001 (http://crrev.com/2897553002#ps270001)

Review-Url: https://codereview.chromium.org/2958693002
Cr-Commit-Position: refs/heads/master@{#482490}

[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/ime_menu/ime_menu_tray.h
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/palette/palette_tray.cc
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/palette/palette_tray.h
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/tray/system_tray.cc
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/tray/system_tray.h
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/tray/system_tray_bubble.cc
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/tray/system_tray_bubble.h
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/web_notification/web_notification_tray.cc
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ash/system/web_notification/web_notification_tray.h
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ui/message_center/views/message_bubble_base.cc
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ui/views/bubble/tray_bubble_view.cc
[modify] https://crrev.com/66c305b497c7f8e2125a05f544fd346862f0bb8e/ui/views/bubble/tray_bubble_view.h

Comment 7 by yoshiki@chromium.org, Aug 7 2017 

It look this patch doesn't get the focus from Android app by pressing Tab-key.

Repro step:
1. Activate an Android app window
2. Open the message center
3. Press Tab key
Expected: The message center gets the focus
Actual: The focus moves within the Android app window. The message center never gets the focus.

Comment 8 by yawano@chromium.org, Aug 7 2017 

It may be the same issue with  issue 740055 
I'll check Android application as well when I work on the issue.
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6bef6b21c56e44850e3241d92d282bda1117705e

commit 6bef6b21c56e44850e3241d92d282bda1117705e
Author: Yuki Awano <yawano@chromium.org>
Date: Wed Sep 06 02:53:21 2017

Capture key events by pre target handler for tray bubble view

Captures key events by pre target handler for activating tray bubble
view when user tries to interact the tray with keyboard.

      icon and open it. Press Tab key. Confirm that focus moves on the
      system tray. Confirm the same thing for Android window as well.
      ash_unittests::SystemTrayTest.KeyboardNavigationWithOtherWindow
      exo_unittests::ShellSurfaceTest.KeyboardNavigationWithSystemTray

Bug:  740055 , 731748
Test: Open a chrome packaged app (e.g. Files app). Click system tray
Change-Id: I2251f1cdaf7bda3ab8b472c671e9f3089086302e
Reviewed-on: https://chromium-review.googlesource.com/603547
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Commit-Queue: Yuki Awano <yawano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499859}
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/ime_menu/ime_menu_tray.h
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/palette/palette_tray.cc
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/palette/palette_tray.h
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/tray/system_tray.cc
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/tray/system_tray.h
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/tray/system_tray_unittest.cc
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/web_notification/web_notification_tray.cc
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/system/web_notification/web_notification_tray.h
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/test/ash_test_views_delegate.cc
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ash/test/ash_test_views_delegate.h
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/components/exo/shell_surface_unittest.cc
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ui/views/bubble/tray_bubble_view.cc
[modify] https://crrev.com/6bef6b21c56e44850e3241d92d282bda1117705e/ui/views/bubble/tray_bubble_view.h
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 11 2017

Labels: merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c9202986574e296a7dcfe42d0d2dc9ca9fff067

commit 1c9202986574e296a7dcfe42d0d2dc9ca9fff067
Author: Yuki Awano <yawano@chromium.org>
Date: Mon Sep 11 01:16:12 2017

Capture key events by pre target handler for tray bubble view

Captures key events by pre target handler for activating tray bubble
view when user tries to interact the tray with keyboard.

      icon and open it. Press Tab key. Confirm that focus moves on the
      system tray. Confirm the same thing for Android window as well.
      ash_unittests::SystemTrayTest.KeyboardNavigationWithOtherWindow
      exo_unittests::ShellSurfaceTest.KeyboardNavigationWithSystemTray

Bug:  740055 , 731748
Test: Open a chrome packaged app (e.g. Files app). Click system tray
Change-Id: I2251f1cdaf7bda3ab8b472c671e9f3089086302e
Reviewed-on: https://chromium-review.googlesource.com/603547
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Commit-Queue: Yuki Awano <yawano@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499859}(cherry picked from commit 6bef6b21c56e44850e3241d92d282bda1117705e)
Reviewed-on: https://chromium-review.googlesource.com/657541
Reviewed-by: Yuki Awano <yawano@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#115}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/ime_menu/ime_menu_tray.h
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/palette/palette_tray.cc
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/palette/palette_tray.h
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/tray/system_tray.cc
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/tray/system_tray.h
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/tray/system_tray_unittest.cc
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/web_notification/web_notification_tray.cc
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/system/web_notification/web_notification_tray.h
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/test/ash_test_views_delegate.cc
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ash/test/ash_test_views_delegate.h
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/components/exo/shell_surface_unittest.cc
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ui/views/bubble/tray_bubble_view.cc
[modify] https://crrev.com/1c9202986574e296a7dcfe42d0d2dc9ca9fff067/ui/views/bubble/tray_bubble_view.h
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 11 2017

Labels: merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7003113b1eab183b159c1459d2ddb40c9e1d1640

commit 7003113b1eab183b159c1459d2ddb40c9e1d1640
Author: Yuki Awano <yawano@chromium.org>
Date: Mon Sep 11 01:41:29 2017

Capture key events by pre target handler for tray bubble view

Captures key events by pre target handler for activating tray bubble
view when user tries to interact the tray with keyboard.

      icon and open it. Press Tab key. Confirm that focus moves on the
      system tray. Confirm the same thing for Android window as well.
      ash_unittests::SystemTrayTest.KeyboardNavigationWithOtherWindow
      exo_unittests::ShellSurfaceTest.KeyboardNavigationWithSystemTray

TBR=yawano@chromium.org

(cherry picked from commit 6bef6b21c56e44850e3241d92d282bda1117705e)

Bug:  740055 , 731748
Test: Open a chrome packaged app (e.g. Files app). Click system tray
Change-Id: I2251f1cdaf7bda3ab8b472c671e9f3089086302e
Reviewed-on: https://chromium-review.googlesource.com/603547
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Commit-Queue: Yuki Awano <yawano@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499859}
Reviewed-on: https://chromium-review.googlesource.com/659457
Reviewed-by: Yuki Awano <yawano@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1155}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/ime_menu/ime_menu_tray.cc
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/ime_menu/ime_menu_tray.h
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/palette/palette_tray.cc
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/palette/palette_tray.h
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/tray/system_tray.cc
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/tray/system_tray.h
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/tray/system_tray_unittest.cc
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/web_notification/web_notification_tray.cc
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/system/web_notification/web_notification_tray.h
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/test/ash_test_views_delegate.cc
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ash/test/ash_test_views_delegate.h
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/components/exo/shell_surface_unittest.cc
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ui/views/bubble/tray_bubble_view.cc
[modify] https://crrev.com/7003113b1eab183b159c1459d2ddb40c9e1d1640/ui/views/bubble/tray_bubble_view.h

Comment 12 by benhenry@chromium.org, Aug 1

Status: Assigned (was: Available)
►

Sign in to add a comment