Commit 41e98a96 authored by Jan Krcal's avatar Jan Krcal Committed by Chromium LUCI CQ

Revert "[Profile creation] Add {back,reload} accelerators for navigation"

This reverts commit bedb24da.

Reason for revert: the test is flaky / failing on MSan

Original change's description:
> [Profile creation] Add {back,reload} accelerators for navigation
>
> This CL adds two accelerators into the profile creation flow to improve
> accessibility and usability:
>  - the back action for screens that actually show the back button
>  - the reload action for the sign-in flow that happens over the internet
>    and thus loading of the page can fail.
>
> Bug: 1126913, 1167675
> Change-Id: I3c197dea2859c0136db62249064249e0569a93da
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2627403
> Auto-Submit: Jan Krcal <jkrcal@chromium.org>
> Commit-Queue: David Roger <droger@chromium.org>
> Reviewed-by: David Roger <droger@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#844695}

TBR=droger@chromium.org,jkrcal@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: I50060ea130339fe25df000ecffaf7e474ba285b8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1126913
Bug: 1167675
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2637595Reviewed-by: default avatarJan Krcal <jkrcal@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844751}
parent 3d31c12b
...@@ -74,24 +74,6 @@ class ProfilePickerInteractiveUiTest : public ProfilePickerTestBase { ...@@ -74,24 +74,6 @@ class ProfilePickerInteractiveUiTest : public ProfilePickerTestBase {
widget()->GetNativeWindow(), ui::VKEY_W, control, shift, /*alt=*/false, widget()->GetNativeWindow(), ui::VKEY_W, control, shift, /*alt=*/false,
command)); command));
} }
void SendBackKeyboardCommand() {
// Close window using keyboard.
#if defined(OS_MAC)
// Use Cmd-[ on Mac.
bool alt = false;
bool command = true;
ui::KeyboardCode key = ui::VKEY_OEM_4;
#else
// Use Ctrl-left on other platforms.
bool alt = true;
bool command = false;
ui::KeyboardCode key = ui::VKEY_LEFT;
#endif
ASSERT_TRUE(ui_test_utils::SendKeyPressToWindowSync(
widget()->GetNativeWindow(), key, /*control=*/false,
/*shift=*/false, alt, command));
}
}; };
// Checks that the main picker view can be closed with keyboard shortcut. // Checks that the main picker view can be closed with keyboard shortcut.
...@@ -183,44 +165,3 @@ IN_PROC_BROWSER_TEST_F(ProfilePickerInteractiveUiTest, ...@@ -183,44 +165,3 @@ IN_PROC_BROWSER_TEST_F(ProfilePickerInteractiveUiTest,
SendCloseWindowKeyboardCommand(); SendCloseWindowKeyboardCommand();
WaitForPickerClosed(); WaitForPickerClosed();
} }
// Checks that both the signin web view and the main picker view are able to
// process a back keyboard event.
IN_PROC_BROWSER_TEST_F(ProfilePickerInteractiveUiTest,
NavigateBackWithKeyboard) {
// Simulate walking through the flow starting at the picker so that navigating
// back to the picker makes sense.
ProfilePicker::Show(ProfilePicker::EntryPoint::kProfileMenuManageProfiles);
WaitForLayoutWithoutToolbar();
WaitForFirstPaint(web_contents(), GURL("chrome://profile-picker"));
web_contents()->GetController().LoadURL(
GURL("chrome://profile-picker/new-profile"), content::Referrer(),
ui::PAGE_TRANSITION_AUTO_TOPLEVEL, std::string());
WaitForFirstPaint(web_contents(),
GURL("chrome://profile-picker/new-profile"));
// Simulate a click on the signin button.
base::MockCallback<base::OnceCallback<void(bool)>> switch_finished_callback;
EXPECT_CALL(switch_finished_callback, Run(true));
ProfilePicker::SwitchToSignIn(SK_ColorRED, switch_finished_callback.Get());
// Switch to the signin webview.
WaitForLayoutWithToolbar();
WaitForFirstPaint(web_contents(),
GaiaUrls::GetInstance()->signin_chrome_sync_dice());
// Navigate back with the keyboard.
SendBackKeyboardCommand();
WaitForLayoutWithoutToolbar();
WaitForFirstPaint(web_contents(),
GURL("chrome://profile-picker/new-profile"));
// Navigate again back with the keyboard.
SendBackKeyboardCommand();
WaitForFirstPaint(web_contents(), GURL("chrome://profile-picker"));
// Navigating back once again does nothing.
SendBackKeyboardCommand();
EXPECT_EQ(web_contents()->GetController().GetPendingEntry(), nullptr);
}
...@@ -101,7 +101,7 @@ constexpr base::TimeDelta kExtendedAccountInfoTimeout = ...@@ -101,7 +101,7 @@ constexpr base::TimeDelta kExtendedAccountInfoTimeout =
constexpr int kSupportedAcceleratorCommands[] = { constexpr int kSupportedAcceleratorCommands[] = {
IDC_CLOSE_TAB, IDC_CLOSE_WINDOW, IDC_EXIT, IDC_FULLSCREEN, IDC_CLOSE_TAB, IDC_CLOSE_WINDOW, IDC_EXIT, IDC_FULLSCREEN,
IDC_MINIMIZE_WINDOW, IDC_BACK, IDC_RELOAD}; IDC_MINIMIZE_WINDOW};
void ShowCustomizationBubble(SkColor new_profile_color, Browser* browser) { void ShowCustomizationBubble(SkColor new_profile_color, Browser* browser) {
views::View* anchor_view = BrowserView::GetBrowserViewForBrowser(browser) views::View* anchor_view = BrowserView::GetBrowserViewForBrowser(browser)
...@@ -551,8 +551,7 @@ void ProfilePickerView::SwitchToSyncConfirmation() { ...@@ -551,8 +551,7 @@ void ProfilePickerView::SwitchToSyncConfirmation() {
// the signin screen is no concern any more. // the signin screen is no concern any more.
ShowScreen(new_profile_contents_.get(), ShowScreen(new_profile_contents_.get(),
GURL(chrome::kChromeUISyncConfirmationURL), GURL(chrome::kChromeUISyncConfirmationURL),
/*show_toolbar=*/false, /*show_toolbar=*/false);
/*enable_navigating_back=*/false);
SyncConfirmationUI* sync_confirmation_ui = static_cast<SyncConfirmationUI*>( SyncConfirmationUI* sync_confirmation_ui = static_cast<SyncConfirmationUI*>(
new_profile_contents_->GetWebUI()->GetController()); new_profile_contents_->GetWebUI()->GetController());
...@@ -623,21 +622,6 @@ bool ProfilePickerView::AcceleratorPressed(const ui::Accelerator& accelerator) { ...@@ -623,21 +622,6 @@ bool ProfilePickerView::AcceleratorPressed(const ui::Accelerator& accelerator) {
case IDC_MINIMIZE_WINDOW: case IDC_MINIMIZE_WINDOW:
GetWidget()->Minimize(); GetWidget()->Minimize();
break; break;
case IDC_BACK: {
NavigateBack();
break;
}
// Always reload bypassing cache.
case IDC_RELOAD:
case IDC_RELOAD_BYPASSING_CACHE:
case IDC_RELOAD_CLEARING_CACHE: {
// Sign-in may fail due to connectivity issues, allow reloading.
if (IsSigningIn()) {
new_profile_contents_->GetController().Reload(
content::ReloadType::BYPASSING_CACHE, true);
}
break;
}
default: default:
NOTREACHED() << "Unexpected command_id: " << command_id; NOTREACHED() << "Unexpected command_id: " << command_id;
break; break;
...@@ -731,8 +715,7 @@ void ProfilePickerView::UpdateToolbarColor() { ...@@ -731,8 +715,7 @@ void ProfilePickerView::UpdateToolbarColor() {
void ProfilePickerView::ShowScreen(content::WebContents* contents, void ProfilePickerView::ShowScreen(content::WebContents* contents,
const GURL& url, const GURL& url,
bool show_toolbar, bool show_toolbar) {
bool enable_navigating_back) {
web_view_->SetWebContents(contents); web_view_->SetWebContents(contents);
web_view_->RequestFocus(); web_view_->RequestFocus();
...@@ -740,8 +723,6 @@ void ProfilePickerView::ShowScreen(content::WebContents* contents, ...@@ -740,8 +723,6 @@ void ProfilePickerView::ShowScreen(content::WebContents* contents,
// it easier for tests to detect changing of the screen. // it easier for tests to detect changing of the screen.
toolbar_->SetVisible(show_toolbar); toolbar_->SetVisible(show_toolbar);
enable_navigating_back_ = enable_navigating_back;
if (!url.is_empty()) { if (!url.is_empty()) {
// Make sure to load the url as the last step so that the UI state is // Make sure to load the url as the last step so that the UI state is
// coherent upon the NavigationStateChanged notification. // coherent upon the NavigationStateChanged notification.
...@@ -752,22 +733,18 @@ void ProfilePickerView::ShowScreen(content::WebContents* contents, ...@@ -752,22 +733,18 @@ void ProfilePickerView::ShowScreen(content::WebContents* contents,
} }
void ProfilePickerView::BackButtonPressed(const ui::Event& event) { void ProfilePickerView::BackButtonPressed(const ui::Event& event) {
NavigateBack(); if (!IsSigningIn()) {
}
void ProfilePickerView::NavigateBack() {
if (!enable_navigating_back_)
return; return;
}
if (web_view_->GetWebContents()->GetController().CanGoBack()) { if (new_profile_contents_->GetController().CanGoBack()) {
web_view_->GetWebContents()->GetController().GoBack(); new_profile_contents_->GetController().GoBack();
return; return;
} }
// Move from sign-in back to the previous screen of profile creation. // Move from sign-in back to the previous screen of profile creation.
// Do not load any url because the desired screen is still loaded in // Do not load any url because the desired screen is still loaded in
// `system_profile_contents_`. // `system_profile_contents_`.
if (IsSigningIn())
ShowScreen(system_profile_contents_.get(), GURL(), /*show_toolbar=*/false); ShowScreen(system_profile_contents_.get(), GURL(), /*show_toolbar=*/false);
} }
...@@ -790,7 +767,7 @@ void ProfilePickerView::OnRefreshTokenUpdatedForAccount( ...@@ -790,7 +767,7 @@ void ProfilePickerView::OnRefreshTokenUpdatedForAccount(
// in some cases (such as managed signed-in), there are further delays before // in some cases (such as managed signed-in), there are further delays before
// any follow-up UI is shown. // any follow-up UI is shown.
ShowScreen(new_profile_contents_.get(), GURL(url::kAboutBlankURL), ShowScreen(new_profile_contents_.get(), GURL(url::kAboutBlankURL),
/*show_toolbar=*/true, /*enable_navigating_back=*/false); /*show_toolbar=*/true);
// Set up a timeout for extended account info (which cancels any existing // Set up a timeout for extended account info (which cancels any existing
// timeout closure). // timeout closure).
...@@ -906,7 +883,7 @@ void ProfilePickerView::FinishSignedInCreationFlowForSAML() { ...@@ -906,7 +883,7 @@ void ProfilePickerView::FinishSignedInCreationFlowForSAML() {
// Free up `new_profile_contents_` to be moved to a new browser window. // Free up `new_profile_contents_` to be moved to a new browser window.
new_profile_contents_->SetDelegate(nullptr); new_profile_contents_->SetDelegate(nullptr);
ShowScreen(system_profile_contents_.get(), GURL(url::kAboutBlankURL), ShowScreen(system_profile_contents_.get(), GURL(url::kAboutBlankURL),
/*show_toolbar=*/false, /*enable_navigating_back=*/false); /*show_toolbar=*/false);
FinishSignedInCreationFlowImpl( FinishSignedInCreationFlowImpl(
base::BindOnce(&ContinueSAMLSignin, std::move(new_profile_contents_)), base::BindOnce(&ContinueSAMLSignin, std::move(new_profile_contents_)),
/*enterprise_sync_consent_needed=*/true); /*enterprise_sync_consent_needed=*/true);
......
...@@ -132,11 +132,9 @@ class ProfilePickerView : public views::WidgetDelegateView, ...@@ -132,11 +132,9 @@ class ProfilePickerView : public views::WidgetDelegateView,
// `url` is empty, it only shows `contents` with its currently loaded url. // `url` is empty, it only shows `contents` with its currently loaded url.
void ShowScreen(content::WebContents* contents, void ShowScreen(content::WebContents* contents,
const GURL& url, const GURL& url,
bool show_toolbar, bool show_toolbar);
bool enable_navigating_back = true);
void BackButtonPressed(const ui::Event& event); void BackButtonPressed(const ui::Event& event);
void NavigateBack();
// Checks whether the sign-in flow is in progress. // Checks whether the sign-in flow is in progress.
bool IsSigningIn() const; bool IsSigningIn() const;
...@@ -190,7 +188,6 @@ class ProfilePickerView : public views::WidgetDelegateView, ...@@ -190,7 +188,6 @@ class ProfilePickerView : public views::WidgetDelegateView,
// A mapping between accelerators and command IDs. // A mapping between accelerators and command IDs.
std::map<ui::Accelerator, int> accelerator_table_; std::map<ui::Accelerator, int> accelerator_table_;
bool enable_navigating_back_ = true;
// Handler for unhandled key events from renderer. // Handler for unhandled key events from renderer.
views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_; views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;
......
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