Commit a9319110 authored by Abigail Klein's avatar Abigail Klein Committed by Commit Bot

[Live Caption] Only allow the caption bubble to activate when it is

visible.

On Windows, there is a bug where the caption bubble is focused after
the tab model changes, even if the caption bubble had previously been
deactivated. When the tab model changes, it causes focus to advance to
the next focusable view and activate that view's widget. The fix for the
caption bubble is to ensure that the caption bubble cannot be activated
by the focus manager unless the caption bubble is visible.

The stack trace of this behavior is:
Browser::OnTabStripModelChanged
Browser::OnTabDetached
BrowserView::OnTabDetached
views::WebView::SetWebContents
views::WebView::UpdateCrashedOverlayView
views::View::SetFocusBehavior
views::View::AdvanceFocusIfNecessary
views::FocusManager::AdvanceFocusIfNecessary
views::FocusManager::AdvanceFocus
views::FocusManager::SetFocusedViewWithReason
views::Widget::Activate
...leading to...
captions::CaptionBubble::OnWidgetActivationChanged

One outstanding question is whether the focus manager should ever
activate a widget that is not visible. This will be explored in a
follow-up.

Bug: 1055150, 1123635
Change-Id: I9b5eaee54a5f78592c320b18282a726f9e0980eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2558721
Commit-Queue: Abigail Klein <abigailbklein@google.com>
Reviewed-by: default avatarElly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830782}
parent a909becf
......@@ -339,6 +339,8 @@ void CaptionBubble::Init() {
SkColorSetA(gfx::kGoogleGrey900, kCaptionBubbleAlpha);
set_color(caption_bubble_color_);
set_close_on_deactivate(false);
// The caption bubble starts out hidden and unable to be activated.
SetCanActivate(false);
auto label = std::make_unique<views::Label>();
label->SetMultiLine(true);
......@@ -681,6 +683,9 @@ void CaptionBubble::UpdateBubbleVisibility() {
void CaptionBubble::OnWidgetVisibilityChanged(views::Widget* widget,
bool visible) {
DCHECK_EQ(widget, GetWidget());
// The caption bubble can only be activated when it is visible. Nothing else,
// including the focus manager, can activate the caption bubble.
SetCanActivate(visible);
// Ensure that the widget is only activated when it is visible.
// TODO(crbug.com/1144201): Investigate whether Hide() should always
// deactivate widgets, and if so, remove this.
......
......@@ -104,6 +104,10 @@ class CaptionBubbleControllerViewsTest : public InProcessBrowserTest {
return controller_ && controller_->IsWidgetVisibleForTesting();
}
bool CanWidgetActivate() {
return GetCaptionWidget() && GetCaptionWidget()->CanActivate();
}
bool IsWidgetActive() {
return GetCaptionWidget() && GetCaptionWidget()->IsActive();
}
......@@ -1034,12 +1038,15 @@ IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest,
IN_PROC_BROWSER_TEST_F(CaptionBubbleControllerViewsTest,
BubbleDeactivatedWhenHidden) {
EXPECT_FALSE(IsWidgetVisible());
EXPECT_FALSE(CanWidgetActivate());
EXPECT_FALSE(IsWidgetActive());
OnPartialTranscription("Cows can detect odors up to 6 miles away.");
EXPECT_TRUE(IsWidgetVisible());
EXPECT_TRUE(CanWidgetActivate());
EXPECT_TRUE(IsWidgetActive());
ClickButton(GetCloseButton());
EXPECT_FALSE(IsWidgetVisible());
EXPECT_FALSE(CanWidgetActivate());
EXPECT_FALSE(IsWidgetActive());
}
#endif
......
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