Commit c24f4824 authored by John Lee's avatar John Lee Committed by Commit Bot

WebUI Tab Strip: Handle cancelling out of a beforeunload confirm dialog

This CL adds a tracker that observes a tab's WebContents after it has
been swiped to close on the front-end. The tracker's observer listens
for close events (which will naturally remove the TabElement from the
DOM) or for when a user cancels out of a beforeunload dialog (which will
reset the swipe animation).

Bug: 1059080
Change-Id: Id21355fc8d1b8f1001a10106c801fd514cf35ee2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2088410
Commit-Queue: John Lee <johntlee@chromium.org>
Reviewed-by: default avatardpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748904}
parent d498e91b
......@@ -226,16 +226,14 @@ export class TabElement extends CustomElement {
* @private
*/
onClose_(event) {
if (!this.tab_) {
return;
}
assert(this.tab_);
event.stopPropagation();
this.tabsApi_.closeTab(this.tab_.id, CloseTabAction.CLOSE_BUTTON);
}
/** @private */
onSwipe_() {
assert(this.tab_);
this.tabsApi_.closeTab(this.tab_.id, CloseTabAction.SWIPED_TO_CLOSE);
}
......@@ -249,6 +247,10 @@ export class TabElement extends CustomElement {
}
}
resetSwipe() {
this.tabSwiper_.reset();
}
/**
* @param {boolean} isDragging
*/
......@@ -307,8 +309,8 @@ export class TabElement extends CustomElement {
* @return {!Promise}
*/
slideOut() {
if (!this.embedderApi_.isVisible() || this.tab_.pinned) {
// There is no point in animating if the tab strip is hidden.
if (!this.embedderApi_.isVisible() || this.tab_.pinned ||
this.tabSwiper_.wasSwiping()) {
this.remove();
return Promise.resolve();
}
......
......@@ -272,6 +272,8 @@ class TabListElement extends CustomElement {
this.addWebUIListener_('tab-updated', tab => this.onTabUpdated_(tab));
this.addWebUIListener_(
'tab-active-changed', tabId => this.onTabActivated_(tabId));
this.addWebUIListener_(
'tab-close-cancelled', tabId => this.onTabCloseCancelled_(tabId));
this.addWebUIListener_(
'tab-group-state-changed',
(tabId, index, groupId) =>
......@@ -458,6 +460,18 @@ class TabListElement extends CustomElement {
this.activatingTabIdTimestamp_ = Date.now();
}
/**
* @param {number} id
* @private
*/
onTabCloseCancelled_(id) {
const tabElement = this.findTabElement_(id);
if (!tabElement) {
return;
}
tabElement.resetSwipe();
}
/**
* @param {!TabData} tab
* @private
......
......@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
import {isRTL} from 'chrome://resources/js/util.m.js';
/**
* The minimum amount of pixels needed for the user to swipe for the position
* (controlled by transform property) to start animating to 0.
......@@ -81,6 +83,9 @@ export class TabSwiper {
/** @private */
createAnimation_() {
// TODO(crbug.com/1025390): padding-inline-end does not work with
// animations built using JS.
const paddingInlineEnd = isRTL() ? 'paddingLeft' : 'paddingRight';
const animation = new Animation(new KeyframeEffect(
this.element_,
[
......@@ -88,6 +93,7 @@ export class TabSwiper {
// Base.
opacity: 1,
maxWidth: 'var(--tabstrip-tab-width)',
[paddingInlineEnd]: 'var(--tabstrip-tab-spacing)',
transform: `translateY(0)`
},
{
......@@ -100,12 +106,14 @@ export class TabSwiper {
// Start of max-width and opacity animation swiping up.
maxWidth: 'var(--tabstrip-tab-width)',
offset: SWIPE_START_THRESHOLD_PX / SWIPE_FINISH_THRESHOLD_PX,
[paddingInlineEnd]: 'var(--tabstrip-tab-spacing)',
opacity: 1,
},
{
// Fully swiped up.
maxWidth: '0px',
opacity: 0,
[paddingInlineEnd]: 0,
transform: `translateY(-${SWIPE_FINISH_THRESHOLD_PX}px)`
},
],
......@@ -113,7 +121,7 @@ export class TabSwiper {
duration: SWIPE_FINISH_THRESHOLD_PX,
fill: 'both',
}));
animation.currentTime = 0;
animation.cancel();
animation.onfinish = () => {
this.element_.dispatchEvent(new CustomEvent('swipe'));
};
......@@ -161,6 +169,10 @@ export class TabSwiper {
}
const yDiff = this.currentPointerDownEvent_.clientY - event.clientY;
const animationTime = yDiff;
this.animation_.currentTime =
Math.max(0, Math.min(SWIPE_FINISH_THRESHOLD_PX, animationTime));
if (!this.animationInitiated_ &&
Math.abs(yDiff) > TRANSLATE_ANIMATION_THRESHOLD_PX) {
this.animationInitiated_ = true;
......@@ -177,22 +189,31 @@ export class TabSwiper {
return;
}
const yDiff = this.currentPointerDownEvent_.clientY - event.clientY;
const pixelsSwiped =
Math.max(0, Math.min(SWIPE_FINISH_THRESHOLD_PX, yDiff));
const pixelsSwiped = this.animation_.currentTime;
const swipedEnoughToClose = pixelsSwiped > SWIPE_START_THRESHOLD_PX;
const wasHighVelocity = pixelsSwiped /
(event.timeStamp - this.currentPointerDownEvent_.timeStamp) >
SWIPE_VELOCITY_THRESHOLD;
if (pixelsSwiped === SWIPE_FINISH_THRESHOLD_PX || swipedEnoughToClose ||
wasHighVelocity) {
this.element_.dispatchEvent(new CustomEvent('swipe'));
if (pixelsSwiped === SWIPE_FINISH_THRESHOLD_PX) {
// The user has swiped the max amount of pixels to swipe and the animation
// has already completed all its keyframes, so just fire the onfinish
// events on the animation.
this.animation_.finish();
} else if (swipedEnoughToClose || wasHighVelocity) {
this.animation_.play();
} else {
this.animation_.cancel();
this.animation_.currentTime = 0;
}
this.clearPointerEvents_();
}
reset() {
this.animation_.cancel();
}
startObserving() {
this.element_.addEventListener('pointerdown', this.pointerDownListener_);
}
......
......@@ -110,15 +110,13 @@ export class TabsApiProxy {
/**
* @param {number} tabId
* @param {!CloseTabAction} closeTabAction
* @return {!Promise}
*/
closeTab(tabId, closeTabAction) {
return new Promise(resolve => {
chrome.tabs.remove(tabId, resolve);
chrome.metricsPrivate.recordEnumerationValue(
'WebUITabStrip.CloseTabAction', closeTabAction,
Object.keys(CloseTabAction).length);
});
chrome.send(
'closeTab', [tabId, closeTabAction === CloseTabAction.SWIPED_TO_CLOSE]);
chrome.metricsPrivate.recordEnumerationValue(
'WebUITabStrip.CloseTabAction', closeTabAction,
Object.keys(CloseTabAction).length);
}
/**
......
......@@ -4244,6 +4244,8 @@ jumbo_static_library("ui") {
"views/toolbar/webui_tab_counter_button.h",
"webui/tab_strip/chrome_content_browser_client_tab_strip_part.cc",
"webui/tab_strip/chrome_content_browser_client_tab_strip_part.h",
"webui/tab_strip/tab_before_unload_tracker.cc",
"webui/tab_strip/tab_before_unload_tracker.h",
"webui/tab_strip/tab_strip_ui.cc",
"webui/tab_strip/tab_strip_ui.h",
"webui/tab_strip/tab_strip_ui_embedder.h",
......
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/ui/webui/tab_strip/tab_before_unload_tracker.h"
#include <memory>
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_observer.h"
namespace tab_strip_ui {
TabBeforeUnloadTracker::TabBeforeUnloadTracker(
TabCloseCancelledCallback cancelled_callback)
: cancelled_callback_(std::move(cancelled_callback)) {}
TabBeforeUnloadTracker::~TabBeforeUnloadTracker() = default;
void TabBeforeUnloadTracker::Observe(content::WebContents* contents) {
observers_[contents] = std::make_unique<TabObserver>(contents, this);
}
void TabBeforeUnloadTracker::Unobserve(content::WebContents* contents) {
observers_.erase(contents);
}
void TabBeforeUnloadTracker::OnBeforeUnloadDialogCancelled(
content::WebContents* contents) {
cancelled_callback_.Run(contents);
}
class TabBeforeUnloadTracker::TabObserver
: public content::WebContentsObserver {
public:
TabObserver(content::WebContents* contents, TabBeforeUnloadTracker* tracker)
: content::WebContentsObserver(contents), tracker_(tracker) {}
~TabObserver() override = default;
// content::WebContentsObserver
void WebContentsDestroyed() override { tracker_->Unobserve(web_contents()); }
void BeforeUnloadDialogCancelled() override {
tracker_->OnBeforeUnloadDialogCancelled(web_contents());
}
private:
TabBeforeUnloadTracker* tracker_;
};
} // namespace tab_strip_ui
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_UI_WEBUI_TAB_STRIP_TAB_BEFORE_UNLOAD_TRACKER_H_
#define CHROME_BROWSER_UI_WEBUI_TAB_STRIP_TAB_BEFORE_UNLOAD_TRACKER_H_
#include "base/observer_list.h"
#include "content/public/browser/web_contents.h"
namespace tab_strip_ui {
// This class keeps track of WebContents observers that listen for when a tab
// is actually closed or when a user cancels out of a beforeunload confirm
// dialog. The observers are added once a user has swiped on a tab in the
// WebUI tab strip and is needed to make the swiped tab in the tab strip
// visible again if a user cancels out of the close flow.
class TabBeforeUnloadTracker {
public:
using TabCloseCancelledCallback =
base::RepeatingCallback<void(content::WebContents*)>;
explicit TabBeforeUnloadTracker(TabCloseCancelledCallback cancelled_callback);
~TabBeforeUnloadTracker();
void Observe(content::WebContents* contents);
void Unobserve(content::WebContents* contents);
void OnBeforeUnloadDialogCancelled(content::WebContents* contents);
private:
class TabObserver;
std::map<content::WebContents*, std::unique_ptr<TabObserver>> observers_;
TabCloseCancelledCallback cancelled_callback_;
};
} // namespace tab_strip_ui
#endif // CHROME_BROWSER_UI_WEBUI_TAB_STRIP_TAB_BEFORE_UNLOAD_TRACKER_H_
......@@ -164,7 +164,10 @@ TabStripUIHandler::TabStripUIHandler(Browser* browser,
: browser_(browser),
embedder_(embedder),
thumbnail_tracker_(base::Bind(&TabStripUIHandler::HandleThumbnailUpdate,
base::Unretained(this))) {}
base::Unretained(this))),
tab_before_unload_tracker_(
base::Bind(&TabStripUIHandler::OnTabCloseCancelled,
base::Unretained(this))) {}
TabStripUIHandler::~TabStripUIHandler() = default;
void TabStripUIHandler::NotifyLayoutChanged() {
......@@ -372,6 +375,9 @@ void TabStripUIHandler::RegisterMessages() {
web_ui()->RegisterMessageCallback(
"closeContainer", base::Bind(&TabStripUIHandler::HandleCloseContainer,
base::Unretained(this)));
web_ui()->RegisterMessageCallback(
"closeTab",
base::Bind(&TabStripUIHandler::HandleCloseTab, base::Unretained(this)));
web_ui()->RegisterMessageCallback(
"showBackgroundContextMenu",
base::Bind(&TabStripUIHandler::HandleShowBackgroundContextMenu,
......@@ -681,6 +687,27 @@ void TabStripUIHandler::HandleCloseContainer(const base::ListValue* args) {
embedder_->CloseContainer();
}
void TabStripUIHandler::HandleCloseTab(const base::ListValue* args) {
AllowJavascript();
int tab_id = args->GetList()[0].GetInt();
content::WebContents* tab = nullptr;
if (!extensions::ExtensionTabUtil::GetTabById(tab_id, browser_->profile(),
true, &tab)) {
// ID didn't refer to a valid tab.
DVLOG(1) << "Invalid tab ID";
return;
}
bool tab_was_swiped = args->GetList()[1].GetBool();
if (tab_was_swiped) {
// The unload tracker will automatically unobserve the tab when it
// successfully closes.
tab_before_unload_tracker_.Observe(tab);
}
tab->Close();
}
void TabStripUIHandler::HandleShowBackgroundContextMenu(
const base::ListValue* args) {
gfx::PointF point;
......@@ -815,6 +842,12 @@ void TabStripUIHandler::HandleThumbnailUpdate(
base::Value(data_uri));
}
void TabStripUIHandler::OnTabCloseCancelled(content::WebContents* tab) {
tab_before_unload_tracker_.Unobserve(tab);
const int tab_id = extensions::ExtensionTabUtil::GetTabId(tab);
FireWebUIListener("tab-close-cancelled", base::Value(tab_id));
}
// Reports a histogram using the format
// WebUITabStrip.|histogram_fragment|.[tab count bucket].
void TabStripUIHandler::ReportTabDurationHistogram(
......
......@@ -10,6 +10,7 @@
#include "chrome/browser/ui/tabs/tab_change_type.h"
#include "chrome/browser/ui/tabs/tab_group.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "chrome/browser/ui/webui/tab_strip/tab_before_unload_tracker.h"
#include "chrome/browser/ui/webui/tab_strip/thumbnail_tracker.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_ui_message_handler.h"
......@@ -49,6 +50,7 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
void RegisterMessages() override;
private:
FRIEND_TEST_ALL_PREFIXES(TabStripUIHandlerTest, CloseTab);
FRIEND_TEST_ALL_PREFIXES(TabStripUIHandlerTest, GetGroupVisualData);
FRIEND_TEST_ALL_PREFIXES(TabStripUIHandlerTest, GroupTab);
FRIEND_TEST_ALL_PREFIXES(TabStripUIHandlerTest, MoveGroup);
......@@ -66,6 +68,7 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
void HandleGetGroupVisualData(const base::ListValue* args);
void HandleGetThemeColors(const base::ListValue* args);
void HandleCloseContainer(const base::ListValue* args);
void HandleCloseTab(const base::ListValue* args);
void HandleShowBackgroundContextMenu(const base::ListValue* args);
void HandleShowEditDialogForGroup(const base::ListValue* args);
void HandleShowTabContextMenu(const base::ListValue* args);
......@@ -80,6 +83,7 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
void HandleReportTabCreationDuration(const base::ListValue* args);
void HandleThumbnailUpdate(content::WebContents* tab,
ThumbnailTracker::CompressedThumbnailData image);
void OnTabCloseCancelled(content::WebContents* tab);
void ReportTabDurationHistogram(const char* histogram_fragment,
int tab_count,
base::TimeDelta duration);
......@@ -87,6 +91,7 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
Browser* const browser_;
TabStripUIEmbedder* const embedder_;
ThumbnailTracker thumbnail_tracker_;
tab_strip_ui::TabBeforeUnloadTracker tab_before_unload_tracker_;
DISALLOW_COPY_AND_ASSIGN(TabStripUIHandler);
};
......
......@@ -509,3 +509,16 @@ TEST_F(TabStripUIHandlerTest, UngroupTab) {
ASSERT_FALSE(browser()->tab_strip_model()->GetTabGroupForTab(0).has_value());
}
TEST_F(TabStripUIHandlerTest, CloseTab) {
AddTab(browser(), GURL("http://foo"));
AddTab(browser(), GURL("http://bar"));
base::ListValue args;
args.AppendInteger(extensions::ExtensionTabUtil::GetTabId(
browser()->tab_strip_model()->GetWebContentsAt(0)));
args.AppendBoolean(false); // If the tab is closed by swipe.
handler()->HandleCloseTab(&args);
ASSERT_EQ(1, browser()->tab_strip_model()->GetTabCount());
}
......@@ -9,6 +9,8 @@ import {setScrollAnimationEnabledForTesting} from 'chrome://tab-strip/tab_list.j
import {TabStripEmbedderProxy} from 'chrome://tab-strip/tab_strip_embedder_proxy.js';
import {TabsApiProxy} from 'chrome://tab-strip/tabs_api_proxy.js';
import {eventToPromise} from '../test_util.m.js';
import {TestTabStripEmbedderProxy} from './test_tab_strip_embedder_proxy.js';
import {TestTabsApiProxy} from './test_tabs_api_proxy.js';
......
......@@ -25,7 +25,7 @@ suite('TabSwiper', () => {
tabSwiper.startObserving();
});
test('swiping does not progress the animation', () => {
test('SwipingProgressesAnimation', () => {
// Set margin top 0 to avoid offsetting the bounding client rect.
document.body.style.margin = 0;
......@@ -47,27 +47,33 @@ suite('TabSwiper', () => {
let startTop = tabElement.getBoundingClientRect().top;
assertEquals(startTop, 0);
// Swiping did not start the animation.
// Swipe was enough to start animating the position.
pointerState.clientY = startY - (TRANSLATE_ANIMATION_THRESHOLD_PX + 1);
tabElement.dispatchEvent(new PointerEvent('pointermove', pointerState));
assertEquals(tabElStyle.maxWidth, `${tabWidth}px`);
assertEquals(startTop, tabElement.getBoundingClientRect().top);
let top = tabElement.getBoundingClientRect().top;
assertTrue(top < startTop && top > -1 * SWIPE_FINISH_THRESHOLD_PX);
// Swipe was enough to close but did not yet animate.
// Swipe was enough to start animating max width and opacity.
pointerState.clientY = startY - (SWIPE_START_THRESHOLD_PX + 1);
tabElement.dispatchEvent(new PointerEvent('pointermove', pointerState));
assertEquals(tabWidth, parseInt(tabElStyle.maxWidth, 10));
assertEquals('1', tabElStyle.opacity);
// Verify animation still not progressed.
assertTrue(
parseInt(tabElStyle.maxWidth) > 0 &&
parseInt(tabElStyle.maxWidth) < tabWidth);
assertTrue(
parseFloat(tabElStyle.opacity) > 0 &&
parseFloat(tabElStyle.opacity) < 1);
// Swipe was enough to finish animating.
pointerState.clientY = startY - (SWIPE_FINISH_THRESHOLD_PX + 1);
tabElement.dispatchEvent(new PointerEvent('pointermove', pointerState));
assertEquals(tabWidth, parseInt(tabElStyle.maxWidth, 10));
assertEquals('1', tabElStyle.opacity);
assertEquals(startTop, tabElement.getBoundingClientRect().top);
assertEquals(tabElStyle.maxWidth, '0px');
assertEquals(tabElStyle.opacity, '0');
assertEquals(
tabElement.getBoundingClientRect().top, -SWIPE_FINISH_THRESHOLD_PX);
});
test('finishing the swipe animation fires an event', async () => {
test('SwipingPastFinishThresholdFiresEvent', async () => {
const firedEventPromise = eventToPromise('swipe', tabElement);
const startY = 50;
const pointerState = {clientY: startY, pointerId: 1};
......@@ -80,7 +86,25 @@ suite('TabSwiper', () => {
await firedEventPromise;
});
test('swiping and letting go before resets animation', () => {
test('SwipingPastStartThresholdFinishesAnimation', async () => {
const firedEventPromise = eventToPromise('swipe', tabElement);
const tabElStyle = window.getComputedStyle(tabElement);
const startY = 50;
const pointerState = {clientY: 50, pointerId: 1};
tabElement.dispatchEvent(new PointerEvent('pointerdown', pointerState));
pointerState.clientY = startY - (SWIPE_START_THRESHOLD_PX + 1);
pointerState.movementY = 1; /* Any non-0 value here is fine. */
tabElement.dispatchEvent(new PointerEvent('pointermove', pointerState));
tabElement.dispatchEvent(new PointerEvent('pointerup', pointerState));
await firedEventPromise;
assertEquals(tabElStyle.maxWidth, '0px');
assertEquals(tabElStyle.opacity, '0');
});
test('NotCompletingSwipePastThreshold', () => {
tabElement.style.setProperty('--tabstrip-tab-width', '100px');
const tabElStyle = window.getComputedStyle(tabElement);
const startY = 50;
......@@ -97,7 +121,7 @@ suite('TabSwiper', () => {
assertEquals(tabElStyle.opacity, '1');
});
test('swiping fast enough fires an event', async () => {
test('SwipingAtHighVelocityFinishesAnimation', async () => {
const tabElStyle = window.getComputedStyle(tabElement);
const firedEventPromise = eventToPromise('swipe', tabElement);
const startY = 50;
......@@ -112,9 +136,11 @@ suite('TabSwiper', () => {
tabElement.dispatchEvent(new PointerEvent('pointerup', pointerState));
await firedEventPromise;
assertEquals(tabElStyle.maxWidth, '0px');
assertEquals(tabElStyle.opacity, '0');
});
test('pointerdown should reset the animation time', async () => {
test('PointerDownResetsAnimationTime', async () => {
tabElement.style.setProperty('--tabstrip-tab-width', '100px');
const tabElStyle = window.getComputedStyle(tabElement);
const pointerState = {clientY: 50, pointerId: 1};
......
......@@ -66,7 +66,7 @@ suite('Tab', function() {
// After animation completes.
assertEquals('100px', tabElementStyle.paddingRight);
assertEquals('none', tabElementStyle.maxWidth);
assertEquals('matrix(1, 0, 0, 1, 0, 0)', tabElementStyle.transform);
assertEquals('none', tabElementStyle.transform);
});
test('slideIn animations for not the last tab', async () => {
......@@ -87,7 +87,7 @@ suite('Tab', function() {
// After animation completes.
assertEquals('100px', tabElementStyle.paddingRight);
assertEquals('none', tabElementStyle.maxWidth);
assertEquals('matrix(1, 0, 0, 1, 0, 0)', tabElementStyle.transform);
assertEquals('none', tabElementStyle.transform);
});
test('slideIn animations right to left for RTL languages', async () => {
......@@ -108,7 +108,7 @@ suite('Tab', function() {
// After animation completes.
assertEquals('100px', tabElementStyle.paddingLeft);
assertEquals('none', tabElementStyle.maxWidth);
assertEquals('matrix(1, 0, 0, 1, 0, 0)', tabElementStyle.transform);
assertEquals('none', tabElementStyle.transform);
});
test('slideOut animates out the element', async () => {
......
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