New issue
Advanced search Search tips

Issue 871652 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 9
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Flaky-Test: FocusManagerTest.ViewFocusCallbacks



Sign in to add a comment

FocusManagerTest.ViewFocusCallbacks is Flaky

Project Member Reported by Findit, Aug 7

Issue description

Labels: -Sheriff-Chromium
Owner: sky@chromium.org
Status: Assigned (was: Available)
Revert triggered.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 7

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

commit cd86dba05535e8b4fb9b2eafa88cd96be93f18f4
Author: Rakina Zata Amni <rakina@chromium.org>
Date: Tue Aug 07 07:16:00 2018

Disable flaky FocusManagerTest.ViewFocusCallbacks

TBR=msw@chromium.org

Bug:  871652 
Change-Id: Ic5ecabf54b1915a3c29cdf65d4bcc4e400300840
Reviewed-on: https://chromium-review.googlesource.com/1164805
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581159}
[modify] https://crrev.com/cd86dba05535e8b4fb9b2eafa88cd96be93f18f4/ui/views/focus/focus_manager_unittest.cc

Status: Fixed (was: Assigned)
My patch was reverted, so I'm moving this to fixed. Will ensure next landing doesn't reintroduce flake.
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

Sign in to add a comment