Commit 848a06eb authored by Sven Zheng's avatar Sven Zheng Committed by Commit Bot

Fix dialog gets randomly closed in tests

When tests run in parrallel, the dialog get randomly closed with reason
lost focus. For bubble dialog, when they get deactivate they will close
themselves:
https://cs.chromium.org/chromium/src/ui/views/bubble/bubble_dialog_delegate_view.cc?l=237&rcl=c32f919eb9638bd8b1030a460c016c6b75a1c286
There's also test trying to tackle the problem:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc?l=345&rcl=2e48cb8072cdcf0a4da0c3e04e196a100838ebcc
This cl will fix it by block close the dialog temporarily.

Why it wasn't an issue when pixel test not enabled?
It's because there's no wait/async in ShowAndVerifyUi(). But with pixeltest, there are several async
calls and wait for callback, which give the browser chance to close the dialog.
Eg the async in snapshot:
https://cs.chromium.org/chromium/src/ui/snapshot/snapshot_aura.cc?l=51&rcl=07fe9cc024d6c20514423412c9c3aead11d9767c

When running in 36 launcher jobs, this issue contributes ~2% flakiness.
I've also observed that in very rare cases, dialog can get closed with ESC pressed. But I didn't
figure out which test mis-sending the ESC key press to the system. So the block close will block
all reasons instead of lost focus only.

Bug: 958242
Change-Id: I51037dd7070fc4c35195d0e07c408763906dcdd9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1889949
Commit-Queue: Sven Zheng <svenzheng@chromium.org>
Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711110}
parent 411235fc
...@@ -22,6 +22,7 @@ ...@@ -22,6 +22,7 @@
#endif #endif
#if defined(TOOLKIT_VIEWS) #if defined(TOOLKIT_VIEWS)
#include "base/callback_helpers.h"
#include "base/strings/strcat.h" #include "base/strings/strcat.h"
#include "ui/compositor/test/draw_waiter_for_test.h" #include "ui/compositor/test/draw_waiter_for_test.h"
#include "ui/display/display.h" #include "ui/display/display.h"
...@@ -100,6 +101,10 @@ bool TestBrowserDialog::VerifyUi() { ...@@ -100,6 +101,10 @@ bool TestBrowserDialog::VerifyUi() {
// TODO(https://crbug.com/958242) support Mac for pixel tests. // TODO(https://crbug.com/958242) support Mac for pixel tests.
#if !defined(OS_MACOSX) #if !defined(OS_MACOSX)
if (pixel_diff_) { if (pixel_diff_) {
dialog_widget->SetBlockCloseForTesting(true);
base::ScopedClosureRunner unblock_close(
base::BindOnce(&views::Widget::SetBlockCloseForTesting,
base::Unretained(dialog_widget), false));
// Wait for painting complete. // Wait for painting complete.
auto* compositor = dialog_widget->GetCompositor(); auto* compositor = dialog_widget->GetCompositor();
ui::DrawWaiterForTest::WaitForCompositingEnded(compositor); ui::DrawWaiterForTest::WaitForCompositingEnded(compositor);
......
...@@ -598,7 +598,9 @@ void Widget::CloseWithReason(ClosedReason closed_reason) { ...@@ -598,7 +598,9 @@ void Widget::CloseWithReason(ClosedReason closed_reason) {
// invoking Close again. // invoking Close again.
return; return;
} }
if (block_close_) {
return;
}
if (non_client_view_ && !non_client_view_->CanClose()) if (non_client_view_ && !non_client_view_->CanClose())
return; return;
......
...@@ -547,6 +547,14 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate, ...@@ -547,6 +547,14 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
// discouraged and only supported for backwards-compatibility with Close(). // discouraged and only supported for backwards-compatibility with Close().
void CloseWithReason(ClosedReason closed_reason); void CloseWithReason(ClosedReason closed_reason);
// A UI test which tries to asynchronously examine a widget (e.g. the pixel
// tests) will fail if the widget is closed before that. This can happen
// easily with widgets that close on focus loss coupled with tests being run
// in parallel, since one test's widget can be closed by the appearance of
// another test's. This method can be used to temporarily disable
// Widget::Close() for such asynchronous cases.
void SetBlockCloseForTesting(bool block_close) { block_close_ = block_close; }
// TODO(beng): Move off public API. // TODO(beng): Move off public API.
// Closes the widget immediately. Compare to |Close|. This will destroy the // Closes the widget immediately. Compare to |Close|. This will destroy the
// window handle associated with this Widget, so should not be called from // window handle associated with this Widget, so should not be called from
...@@ -1097,6 +1105,9 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate, ...@@ -1097,6 +1105,9 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
// disabled. // disabled.
bool movement_disabled_ = false; bool movement_disabled_ = false;
// Block the widget from closing.
bool block_close_ = false;
ScopedObserver<ui::NativeTheme, ui::NativeThemeObserver> observer_manager_{ ScopedObserver<ui::NativeTheme, ui::NativeThemeObserver> observer_manager_{
this}; this};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment