Commit 4b3acdaf authored by edchin's avatar edchin Committed by Commit Bot

[ios] Fix simultaneous touch bug in recent tabs in tab grid

Previously, simultaneously tapping on a recent tab and "enable sync" in
tab grid's recent tab panel would leave the UI in an unusable state.

The cause of the bug is that both touches are processed, and then both
actions attempt to present view controllers.

Simultaneous touch is a grey area, in which the user's intent is unclear
and therefore, the best approach to handling the touches is also unclear.
One approach is to disable one UI element when another UI element is
tapped first. I think that should be a last resort because it makes the
UI feel less responsive, and it is difficult to keep track of all the UI
states of each button and tappable region.

Instead, this CL takes the approach of allowing the taps, processing the
first tap, then the second. Then the second tap attempts to present a view
controller. It is reasonable to ignore this second tap's attempt because
another view controller has already been presented.

This CL fixes these simultaneous touches from recent tabs:
1) a recently closed tab and "enable sync"
2) a tab from another device and "enable sync"
3) "show full history" and "enable sync"

Note that this CL only applies to the sync settings being shown first,
because in practice it seems that the sync settings button tap registers
first before a table view tap. If the other way also happens in practice,
we can add further code in the future.

Also note that I thought about adding tests. But it is not possible to
add an egtest that tests simultaneous taps. And a unittest is too contrived
in this scenario.

TEST: from recent tabs (in tab grid or from menu), simultaneously tap a
recently closed tab, a tab from another device, or "show full history"
with "enable sync".

Bug: 961050
Change-Id: Id388eaeb80173e3ba2f9800a571e990b9d75181b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1750190
Commit-Queue: edchin <edchin@chromium.org>
Reviewed-by: default avatarSergio Collazos <sczs@chromium.org>
Reviewed-by: default avatarRohit Rao <rohitrao@chromium.org>
Reviewed-by: default avataredchin <edchin@chromium.org>
Auto-Submit: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686700}
parent 44174464
......@@ -628,7 +628,15 @@ const int kRecentlyClosedTabsSectionIndex = 0;
break;
case ItemTypeShowFullHistory:
[tableView deselectRowAtIndexPath:indexPath animated:NO];
[self.presentationDelegate showHistoryFromRecentTabs];
// Tapping "show full history" attempts to dismiss recent tabs to show the
// history UI. It is reasonable to ignore this if a modal UI is already
// showing above recent tabs. This can happen when a user simultaneously
// taps "show full history" and "enable sync". The sync settings UI
// appears first and we should not dismiss it to display history.
if (!self.presentedViewController) {
[self.presentationDelegate showHistoryFromRecentTabs];
}
break;
case ItemTypeOtherDevicesSyncOff:
case ItemTypeOtherDevicesNoSessions:
......@@ -861,6 +869,13 @@ const int kRecentlyClosedTabsSectionIndex = 0;
- (void)openTabWithContentOfDistantTab:
(synced_sessions::DistantTab const*)distantTab {
// It is reasonable to ignore this request if a modal UI is already showing
// above recent tabs. This can happen when a user simultaneously taps a
// distant tab and "enable sync". The sync settings UI appears first and we
// should not dismiss it to show a distant tab.
if (self.presentedViewController)
return;
sync_sessions::OpenTabsUIDelegate* openTabs =
SessionSyncServiceFactory::GetForBrowserState(self.browserState)
->GetOpenTabsUIDelegate();
......@@ -896,6 +911,13 @@ const int kRecentlyClosedTabsSectionIndex = 0;
- (void)openTabWithTabRestoreEntry:
(const sessions::TabRestoreService::Entry*)entry {
// It is reasonable to ignore this request if a modal UI is already showing
// above recent tabs. This can happen when a user simultaneously taps a
// recently closed tab and "enable sync". The sync settings UI appears first
// and we should not dismiss it to restore a recently closed tab.
if (self.presentedViewController)
return;
// Only TAB type is handled.
DCHECK_EQ(entry->type, sessions::TabRestoreService::TAB);
base::RecordAction(
......
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