Skip to content

GitLab

T
tangled
Commit 5ebd4db9 authored by Fergal Daly's avatar Fergal Daly Committed by Commit Bot
Browse files
Options

Revert "(Reland) top-chrome-slide: Disable the behavior when Chromevox is enabled." 

This reverts commit 12e96721.

Reason for revert: Reland also causes flakes

https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVyygELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCKTAWNocm9taXVtLmNocm9taXVtb3MvbGludXgtY2hyb21lb3MtcmVsLzE0OTYzL3NpbmdsZV9wcm9jZXNzX21hc2hfYnJvd3Nlcl90ZXN0cy9WRzl3UTI5dWRISnZiSE5UYkdsa1pVTnZiblJ5YjJ4c1pYSlVaWE4wTGxSbGMzUlViMmRuYkdWRGFISnZiV1ZXYjNnPQwLEhNNYXN0ZXJGbGFrZUFuYWx5c2lzGAEM

Original change's description:
> (Reland) top-chrome-slide: Disable the behavior when Chromevox is enabled.
> 
> Users of Chromevox may need to touch explore the webpage, and for
> things to remain consistent, we should disable hiding top-chrome
> with gesture scrolls as long as Chromevox is enabled.
> 
> R=​sky@chromium.org
> BUG=857331, 896514
> TEST=Added a new browser test.
> 
> Reviewed-on: https://chromium-review.googlesource.com/c/1284289
> Reviewed-by: Scott Violet <sky@chromium.org>
> Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#600408}
> Change-Id: I388bc5056f956bea2edffc7127dcab533824e56b
> Reviewed-on: https://chromium-review.googlesource.com/c/1287510
> Cr-Commit-Position: refs/heads/master@{#600966}

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

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 857331, 896514
Change-Id: I62e0f5202c2b5c408dfeb6555d4ca378038be267
Reviewed-on: https://chromium-review.googlesource.com/c/1293091Reviewed-by: default avatarFergal Daly <fergal@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601466}
parent 12e095b7
Hide whitespace changes
Inline Side-by-side
Showing with 5 additions and 133 deletions
chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.cc
View file @ 5ebd4db9
......@@ -37,13 +37,6 @@ bool IsTabletModeEnabled() {
TabletModeClient::Get()->tablet_mode_enabled();
}
bool IsSpokenFeedbackEnabled() {
chromeos::AccessibilityManager* accessibility_manager =
chromeos::AccessibilityManager::Get();
return accessibility_manager &&
accessibility_manager->IsSpokenFeedbackEnabled();
}
// Based on the current status of |contents|, returns the browser top controls
// shown state constraints, which specifies if the top controls are allowed to
// be only shown, or either shown or hidden.
......@@ -282,16 +275,7 @@ TopControlsSlideControllerChromeOS::TopControlsSlideControllerChromeOS(
browser_view_->browser()->tab_strip_model()->AddObserver(this);
chromeos::AccessibilityManager* accessibility_manager =
chromeos::AccessibilityManager::Get();
if (accessibility_manager) {
accessibility_status_subscription_ =
accessibility_manager->RegisterCallback(base::BindRepeating(
&TopControlsSlideControllerChromeOS::OnAccessibilityStatusChanged,
base::Unretained(this)));
}
OnEnabledStateChanged(CanEnable(base::nullopt));
OnEnabledStateChanged(IsTabletModeEnabled() && !browser_view->IsFullscreen());
}
TopControlsSlideControllerChromeOS::~TopControlsSlideControllerChromeOS() {
......@@ -358,13 +342,14 @@ void TopControlsSlideControllerChromeOS::SetShownRatio(
defer_disabling_ = false;
// Don't just set |is_enabled_| to false. Make sure it's a correct value.
OnEnabledStateChanged(CanEnable(base::nullopt));
OnEnabledStateChanged(IsTabletModeEnabled() &&
!browser_view_->IsFullscreen());
}
}
void TopControlsSlideControllerChromeOS::OnBrowserFullscreenStateWillChange(
bool new_fullscreen_state) {
OnEnabledStateChanged(CanEnable(new_fullscreen_state));
OnEnabledStateChanged(IsTabletModeEnabled() && !new_fullscreen_state);
}
bool TopControlsSlideControllerChromeOS::DoBrowserControlsShrinkRendererSize(
......@@ -406,7 +391,7 @@ bool TopControlsSlideControllerChromeOS::IsTopControlsGestureScrollInProgress()
void TopControlsSlideControllerChromeOS::OnTabletModeToggled(
bool tablet_mode_enabled) {
OnEnabledStateChanged(CanEnable(base::nullopt));
OnEnabledStateChanged(tablet_mode_enabled && !browser_view_->IsFullscreen());
}
void TopControlsSlideControllerChromeOS::TabInsertedAt(
......@@ -492,24 +477,6 @@ void TopControlsSlideControllerChromeOS::Observe(
UpdateBrowserControlsStateShown(active_contents, true /* animate */);
}
bool TopControlsSlideControllerChromeOS::CanEnable(
base::Optional<bool> fullscreen_state) const {
return IsTabletModeEnabled() &&
!(fullscreen_state ? *fullscreen_state
: browser_view_->IsFullscreen()) &&
!IsSpokenFeedbackEnabled();
}
void TopControlsSlideControllerChromeOS::OnAccessibilityStatusChanged(
const chromeos::AccessibilityStatusEventDetails& event_details) {
if (event_details.notification_type !=
chromeos::ACCESSIBILITY_TOGGLE_SPOKEN_FEEDBACK) {
return;
}
OnEnabledStateChanged(CanEnable(base::nullopt));
}
void TopControlsSlideControllerChromeOS::OnEnabledStateChanged(bool new_state) {
if (new_state == is_enabled_)
return;
......
chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.h
View file @ 5ebd4db9
......@@ -9,8 +9,6 @@
#include "base/containers/flat_map.h"
#include "base/macros.h"
#include "base/optional.h"
#include "chrome/browser/chromeos/accessibility/accessibility_manager.h"
#include "chrome/browser/ui/ash/tablet_mode_client_observer.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "chrome/browser/ui/views/frame/top_controls_slide_controller.h"
......@@ -84,21 +82,6 @@ class TopControlsSlideControllerChromeOS
const content::NotificationDetails& details) override;
private:
// Returns true if this feature can be turned on. If |fullscreen_state| is
// supplied, it will be used in calculating the result, otherwise the current
// fullscreen state will be queried from BrowserView. This is needed since
// BrowserView informs us with fullscreen state changes before they happen
// (See OnBrowserFullscreenStateWillChange()) so that we can disable the
// sliding behavior *before* immersive mode is entered.
bool CanEnable(base::Optional<bool> fullscreen_state) const;
// Called back from the AccessibilityManager so that we're updated by the
// status of Chromevox, which when enabled, sliding the top-controls should
// be disabled. This is important for users who want to touch explore and need
// this to be consistent.
void OnAccessibilityStatusChanged(
const chromeos::AccessibilityStatusEventDetails& event_details);
void OnEnabledStateChanged(bool new_state);
// Refreshes the status of the browser top controls.
......@@ -165,9 +148,6 @@ class TopControlsSlideControllerChromeOS
content::NotificationRegistrar registrar_;
std::unique_ptr<chromeos::AccessibilityStatusSubscription>
accessibility_status_subscription_;
DISALLOW_COPY_AND_ASSIGN(TopControlsSlideControllerChromeOS);
};
......
chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos_browsertest.cc
View file @ 5ebd4db9
......@@ -18,7 +18,6 @@
#include "base/strings/safe_sprintf.h"
#include "base/test/scoped_feature_list.h"
#include "cc/base/math_util.h"
#include "chrome/browser/chromeos/accessibility/accessibility_manager.h"
#include "chrome/browser/ui/ash/tablet_mode_client.h"
#include "chrome/browser/ui/browser_commands.h"
#include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
......@@ -1009,78 +1008,4 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
CheckBrowserLayout(browser_view(), TopChromeShownState::kFullyShown);
}
// Waits for a compositor frame to be drawn and committed on the given
// web_contents.
class CompositorFrameWaiter : content::WebContentsObserver {
public:
explicit CompositorFrameWaiter(content::WebContents* contents)
: WebContentsObserver(contents) {}
~CompositorFrameWaiter() override = default;
void Wait() { run_loop_.Run(); }
// content::WebContentsObserver:
void DidCommitAndDrawCompositorFrame() override { run_loop_.Quit(); }
private:
base::RunLoop run_loop_;
DISALLOW_COPY_AND_ASSIGN(CompositorFrameWaiter);
};
IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest, TestToggleChromeVox) {
ToggleTabletMode();
ASSERT_TRUE(GetTabletModeEnabled());
EXPECT_TRUE(top_controls_slide_controller()->IsEnabled());
EXPECT_FLOAT_EQ(top_controls_slide_controller()->GetShownRatio(), 1.f);
OpenUrlAtIndex(embedded_test_server()->GetURL("/top_controls_scroll.html"),
0);
content::WebContents* active_contents =
browser_view()->GetActiveWebContents();
PageStateUpdateWaiter page_state_update_waiter(active_contents);
page_state_update_waiter.Wait();
EXPECT_TRUE(
browser_view()->DoBrowserControlsShrinkRendererSize(active_contents));
aura::Window* browser_window = browser()->window()->GetNativeWindow();
ui::test::EventGenerator event_generator(browser_window->GetRootWindow(),
browser_window);
const gfx::Point start_point = event_generator.current_location();
const gfx::Point end_point = start_point + gfx::Vector2d(0, -100);
GenerateGestureFlingScrollSequence(&event_generator, start_point, end_point);
TopControlsShownRatioWaiter waiter(top_controls_slide_controller());
waiter.WaitForRatio(0.f);
EXPECT_FLOAT_EQ(top_controls_slide_controller()->GetShownRatio(), 0);
CheckBrowserLayout(browser_view(), TopChromeShownState::kFullyHidden);
// Enable Chromevox (spoken feedback) and expect that top-chrome will be fully
// shown, and sliding top-chrome is no longer enabled.
chromeos::AccessibilityManager::Get()->EnableSpokenFeedback(true);
EXPECT_TRUE(chromeos::AccessibilityManager::Get()->IsSpokenFeedbackEnabled());
waiter.WaitForRatio(1.f);
EXPECT_FLOAT_EQ(top_controls_slide_controller()->GetShownRatio(), 1.f);
EXPECT_FALSE(top_controls_slide_controller()->IsEnabled());
EXPECT_EQ(browser_view()->GetTopControlsHeight(), 0);
EXPECT_FALSE(
browser_view()->DoBrowserControlsShrinkRendererSize(active_contents));
// Now disable Chromevox, and expect it's now possible to hide top-chrome with
// gesture scrolling.
CompositorFrameWaiter compositor_frame_waiter(active_contents);
chromeos::AccessibilityManager::Get()->EnableSpokenFeedback(false);
compositor_frame_waiter.Wait();
EXPECT_FALSE(
chromeos::AccessibilityManager::Get()->IsSpokenFeedbackEnabled());
EXPECT_FLOAT_EQ(top_controls_slide_controller()->GetShownRatio(), 1.f);
EXPECT_TRUE(top_controls_slide_controller()->IsEnabled());
CheckBrowserLayout(browser_view(), TopChromeShownState::kFullyShown);
base::RunLoop().RunUntilIdle();
// Scroll down, top-chrome should hide.
GenerateGestureFlingScrollSequence(&event_generator, start_point, end_point);
waiter.WaitForRatio(0.f);
EXPECT_FLOAT_EQ(top_controls_slide_controller()->GetShownRatio(), 0);
CheckBrowserLayout(browser_view(), TopChromeShownState::kFullyHidden);
}
} // namespace
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