New issue
Advanced search Search tips

Issue 867654 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 841020



Sign in to add a comment

Closing shortcut viewer doesn't activate underlying browser window

Project Member Reported by jamescook@chromium.org, Jul 25

Issue description

linux-chromeos, ToT

* Open a browser window
* Ctrl-Alt-/ to open shortcut viewer
* Ctrl-W to close shortcut viewer

Browser window doesn't activate. Ctrl-W doesn't work to close, Ctrl-L doesn't work to focus omnibox, etc.

msw, sky suggested I pass this to you. :-P

 
Blocking: 841020
Owner: sky@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 7

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

commit d00fd14396903eb767d86d9fbb97c7dbec8b58d5
Author: Scott Violet <sky@chromium.org>
Date: Tue Aug 07 03:13:24 2018

window-service: ignore requests to focus null

Here's my comment as to why this is ignored:

  // The client is asking to remove focus from a window. This is typically a
  // side effect of the window becoming, or about to become, an unfocusable
  // Window (for example, the Window is hiding). Windows becoming unfocusable is
  // handled locally. Assume the request is for such a scenario and return
  // false to ignore the change.
  //
  // To process null requests conflicts with top-level activation changes. For
  // example, the typical sequence when a window is hidden is to first remove
  // focus, and then hide the window. FocusController keys off window hiding to
  // move activation. If this code where to set focus to null, FocusController
  // would not see the window hiding (because the active window was set to null)
  // and not automatically activate the next window.
  //
  // Another possibility for this code is to handle null as a signal to move
  // focus to the active window (if there is one). I'm going with the simpler
  // approach for now.

BUG= 867654 
TEST=covered by tests

Change-Id: Iee8efd8895284acca4603ff22bde19adf16d8a7f
Reviewed-on: https://chromium-review.googlesource.com/1164409
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581113}
[modify] https://crrev.com/d00fd14396903eb767d86d9fbb97c7dbec8b58d5/ash/main.cc
[modify] https://crrev.com/d00fd14396903eb767d86d9fbb97c7dbec8b58d5/ash/shell/content/client/shell_main_delegate.cc
[modify] https://crrev.com/d00fd14396903eb767d86d9fbb97c7dbec8b58d5/services/ui/ws2/focus_handler.cc
[modify] https://crrev.com/d00fd14396903eb767d86d9fbb97c7dbec8b58d5/services/ui/ws2/focus_handler_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 7

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

commit f792e7d88b0265bf47a7707272ecdfb571cd9679
Author: Rune Lillesveen <futhark@chromium.org>
Date: Tue Aug 07 06:35:39 2018

Revert "window-service: ignore requests to focus null"

This reverts commit d00fd14396903eb767d86d9fbb97c7dbec8b58d5.

Reason for revert: Caused test flakiness  https://crbug.com/871652 

Original change's description:
> window-service: ignore requests to focus null
> 
> Here's my comment as to why this is ignored:
> 
>   // The client is asking to remove focus from a window. This is typically a
>   // side effect of the window becoming, or about to become, an unfocusable
>   // Window (for example, the Window is hiding). Windows becoming unfocusable is
>   // handled locally. Assume the request is for such a scenario and return
>   // false to ignore the change.
>   //
>   // To process null requests conflicts with top-level activation changes. For
>   // example, the typical sequence when a window is hidden is to first remove
>   // focus, and then hide the window. FocusController keys off window hiding to
>   // move activation. If this code where to set focus to null, FocusController
>   // would not see the window hiding (because the active window was set to null)
>   // and not automatically activate the next window.
>   //
>   // Another possibility for this code is to handle null as a signal to move
>   // focus to the active window (if there is one). I'm going with the simpler
>   // approach for now.
> 
> BUG= 867654 
> TEST=covered by tests
> 
> Change-Id: Iee8efd8895284acca4603ff22bde19adf16d8a7f
> Reviewed-on: https://chromium-review.googlesource.com/1164409
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Commit-Queue: Scott Violet <sky@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#581113}

TBR=sky@chromium.org,msw@chromium.org

Change-Id: I7710ea88b1dcb53d5ad299ff1295ecec31ce3f87
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  867654 
Reviewed-on: https://chromium-review.googlesource.com/1164782
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581148}
[modify] https://crrev.com/f792e7d88b0265bf47a7707272ecdfb571cd9679/ash/main.cc
[modify] https://crrev.com/f792e7d88b0265bf47a7707272ecdfb571cd9679/ash/shell/content/client/shell_main_delegate.cc
[modify] https://crrev.com/f792e7d88b0265bf47a7707272ecdfb571cd9679/services/ui/ws2/focus_handler.cc
[modify] https://crrev.com/f792e7d88b0265bf47a7707272ecdfb571cd9679/services/ui/ws2/focus_handler_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 9

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

commit a985282e71b5bb45048538d83e3e0591da5f123c
Author: Scott Violet <sky@chromium.org>
Date: Thu Aug 09 22:29:32 2018

extensions: only close the popup on activation changes if visible

As the widget is shown only once the webcontents has loaded it is possible
to close the popup before it's shown. Ignore activation changes until the
popup is shown.

BUG= 867654 
TEST=covered by tests in mash

Change-Id: I74956b137a119f2b745a52698cb9dee6543b85e4
Reviewed-on: https://chromium-review.googlesource.com/1169525
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581924}
[modify] https://crrev.com/a985282e71b5bb45048538d83e3e0591da5f123c/chrome/browser/ui/views/extensions/extension_popup.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 9

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

commit bef3de4ccd2c52deb936d39dd3a31d86394c5906
Author: Scott Violet <sky@chromium.org>
Date: Thu Aug 09 23:11:26 2018

reland: window-service: ignore requests to focus null

This differs from the first version in that SetFocus(null) now returns
true. As you suggested in the last patch, returning false caused the
client to attempt to revert, which lead to some oddities. So, it seems
safer to return true.

views_mus_unittests no longer fails with this version, and
https://chromium-review.googlesource.com/c/chromium/src/+/1169525 fixs the
issue with the browser_test NavigatingExtensionPopupBrowserTest.Webpage.

Here's my comment as to why this is ignored:

  // The client is asking to remove focus from a window. This is typically a
  // side effect of the window becoming, or about to become, an unfocusable
  // Window (for example, the Window is hiding). Windows becoming unfocusable is
  // handled locally. Assume the request is for such a scenario and return
  // true. Returning false means the client will attempt to revert to the
  // previously focused window, which may cause unexpected activation changes.
  //
  // To process null requests conflicts with top-level activation changes. For
  // example, the typical sequence when a window is hidden is to first remove
  // focus, and then hide the window. FocusController keys off window hiding to
  // move activation. If this code were to set focus to null, FocusController
  // would not see the window hiding (because the active window was set to null)
  // and not automatically activate the next window.
  //
  // Another possibility for this code is to handle null as a signal to move
  // focus to the active window (if there is one). I'm going with the simpler
  // approach for now.

BUG= 867654 ,  871652 
TEST=covered by tests

Change-Id: Iff2ddb31c2b99b977bca8e56fb653b6ce4eb6813
Reviewed-on: https://chromium-review.googlesource.com/1165692
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581951}
[modify] https://crrev.com/bef3de4ccd2c52deb936d39dd3a31d86394c5906/ash/main.cc
[modify] https://crrev.com/bef3de4ccd2c52deb936d39dd3a31d86394c5906/ash/shell/content/client/shell_main_delegate.cc
[modify] https://crrev.com/bef3de4ccd2c52deb936d39dd3a31d86394c5906/services/ui/ws2/focus_handler.cc
[modify] https://crrev.com/bef3de4ccd2c52deb936d39dd3a31d86394c5906/services/ui/ws2/focus_handler_unittest.cc
[modify] https://crrev.com/bef3de4ccd2c52deb936d39dd3a31d86394c5906/ui/views/focus/focus_manager_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment