New issue
Advanced search Search tips

Issue 661988 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug



Sign in to add a comment

Make the correct tab reselected when closing a tab.

Project Member Reported by stkhapugin@chromium.org, Nov 3 2016

Issue description

See TODO in tab_model.mm:

    // Also, any tab opened at the end of the TabStrip with a "TYPED"
    // transition inherit group as well. This covers the cases where the user
    // creates a New Tab (e.g. Ctrl+T, or clicks the New Tab button), or types
    // in the address bar and presses Alt+Enter. This allows for opening a new
    // Tab to quickly look up something. When this Tab is closed, the old one
    // is re-selected, not the next-adjacent.
 
Cc: -pinkerton@chromium.org
Components: UI>Browser>TabStrip
App Version (from "Chrome Settings > About Chrome"): 
iOS Version: 
Device: 

Steps to reproduce: 
1.) Create 2 tabs
2.) Go to the first tab
3.) Open a new tab from tools menu
4.) Close newly open tab

Observed behavior: 
Chrome displays Tab #2

Expected behavior: 
Chrome should display tab #1 (the previous Tab)


Owner: rohitrao@chromium.org
Status: Assigned (was: Untriaged)
Rohit, who would be a good owner for Tab Model bugs?

Comment 3 by pkl@chromium.org, May 21 2018

Cc: eugene...@chromium.org justincohen@chromium.org
Would be nice if we have consistent behavior with Android and Desktop. Do you know happens there?
Cc: rohitrao@chromium.org
Owner: sdefresne@chromium.org
Seems like a bug in WebStateListOrderController::DetermineNewActiveIndex. Sylvain, you are listed as the only owner of web_state_list directory. Can you take a look?
This is a TODO that dates back from TabModel implementation. AFAICT, when I dived in the code, this TODO has been there since the file tab_model.mm was introduced. I created the bug because when I introduced WebStateList, we already had a PRESUBMIT in place that required a crbug for each and every TODO.

I think the feature was never coded in TabModel because Chrome on iOS did not support physical keyboard at the time (thus there was no way to open a new tab with Alt+T or to open a new tab by typing in the omnibox and pressing Alt+Enter). Now that this is supported on Chrome on iOS, we may decide to implement this or just close the bug as WNF (my recommendation).

Note that this would only affect iPads, as you have the tab switcher presented when closing a tab on iPhones, and thus there is no automatically selected tabs (BTW, the same thing happens on Android, the tab switcher does not automatically select another tab, and using back background the app instead of closing the tab if it was open via "new tab" button).

Again, my recommendation would be to close as WNF and to remove the TODO. I did not do it when introducing WebStateList as I did not want to introduce functional changes at the same time as I was refactoring the code.
Cc: pinkerton@chromium.org sdefresne@chromium.org
Owner: mard...@chromium.org
Steps To Reproduce from comment #1 are iPhone-specific, and personally I do feel like existing behavior is a bug.

Mardini, Pink, what do you think about this issue?
Even after re-reading the comment #1, I don't see how the "wrong" tab be selected given that to close a tab you need to open the tab switcher and there are no "selected" tab in the tab switcher.

Here is what I get when I do reproduce the steps in comment #1:  https://drive.google.com/file/d/1FVcAM-U0PTZJbFx6YgaD1mCQaBL4KU5w/view?usp=sharing

There is no selected tabs here.

If however we decide to use the current tab as opener when opening any tab with the menu item, then UI just need to pass TabModel's currentTab as the parentTab when calling TabModel -insertTabWithURL:referrer:transition:opener:openedByDOM:atIndex:inBackground:. And the UI needs to move the tab strip to highlight the currentTab when a tab is closed.

AFAICT, the tab strip does not move to highlight the active web state even if the opener is correctly set (it is set when you use "open in new tab" on a link). To reproduce, just replace the step 3. by "long press a link and select open in new tab" and you would get the same behaviour on iPhone.
Re to comment #7: Sorry for the confusion. On Step 4, I close the tab using Pull To Action gesture.
Tentative CL with some additional comments on what to expect at https://chromium-review.googlesource.com/c/chromium/src/+/1068680
Owner: sdefresne@chromium.org
I agree with comment #6. Thank you, Eugene and Sylvain.

Just a minor correction to the description of the CL: 
"Pass the current tab as the opener when opening a new tab.
This allow selection the previous current tab when the
tab is closed via "pull-to-close" action."

It should also allow selection of the previous tab when the tab is closed via long-press on tab switcher icon ==> Close Tab (when UI Refresh Phase 1 flag is enabled)

See screenshot. 

Sylvain: I am assigning the bug to you since you submitted a CL for it.
IMG_9033.PNG
1022 KB View Download
Project Member

Comment 11 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6bdafce48e5663385945c942cb5fbbf5e4edc3f7

commit 6bdafce48e5663385945c942cb5fbbf5e4edc3f7
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Thu May 24 08:43:33 2018

Pass current tab as opener when opening new tab

Pass the current tab as the opener when opening a new tab.
This allow selection the previous current tab when the
tab is closed via "pull-to-close" action or via the new
"close" action in the UI refresh.

The current tab is passed when tab is opened via ([*]
denotes that this behaviour is added by this CL):
- open tab in background
- "New Tab" menu action [*]
- captive portal detection [*]
- search by image [*]

Bug:  661988 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Iecf41a8c0b3cfe465b3ff95a1061c97739b4ed79
Reviewed-on: https://chromium-review.googlesource.com/1068680
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561427}
[modify] https://crrev.com/6bdafce48e5663385945c942cb5fbbf5e4edc3f7/ios/chrome/app/application_delegate/url_opener_unittest.mm
[modify] https://crrev.com/6bdafce48e5663385945c942cb5fbbf5e4edc3f7/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/6bdafce48e5663385945c942cb5fbbf5e4edc3f7/ios/chrome/browser/ui/browser_view_controller.h
[modify] https://crrev.com/6bdafce48e5663385945c942cb5fbbf5e4edc3f7/ios/chrome/browser/ui/browser_view_controller.mm

Status: Fixed (was: Assigned)
Status: Verified (was: Fixed)
Issue verified 
Version: Chrome Canary 69.0.3444.0
Device: iPhone 6
iOS: 11.2.6

Correct tab is reselected when closing previous tab
https://drive.google.com/open?id=1KPWT0drjt5K0dS5MhQmUxuoANC-V3oHx
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cf2911fabed50b4b13f648f3ff7f53c641c895c

commit 5cf2911fabed50b4b13f648f3ff7f53c641c895c
Author: Gauthier Ambard <gambard@chromium.org>
Date: Tue Jul 03 11:52:34 2018

Revert "Pass current tab as opener when opening new tab"

This reverts commit 6bdafce48e5663385945c942cb5fbbf5e4edc3f7.

Reason for revert: This CL is breaking the child ordering of tabs creating with Long Press on a link -> Open in a New Tab. They are now opened after the last page opened with ToolsMenu -> Open New Tab. See  crbug.com/859508 

Original change's description:
> Pass current tab as opener when opening new tab
>
> Pass the current tab as the opener when opening a new tab.
> This allow selection the previous current tab when the
> tab is closed via "pull-to-close" action or via the new
> "close" action in the UI refresh.
>
> The current tab is passed when tab is opened via ([*]
> denotes that this behaviour is added by this CL):
> - open tab in background
> - "New Tab" menu action [*]
> - captive portal detection [*]
> - search by image [*]
>
> Bug:  661988 
> Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
> Change-Id: Iecf41a8c0b3cfe465b3ff95a1061c97739b4ed79
> Reviewed-on: https://chromium-review.googlesource.com/1068680
> Reviewed-by: Eugene But <eugenebut@chromium.org>
> Reviewed-by: Mark Cogan <marq@chromium.org>
> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561427}

TBR=marq@chromium.org,sdefresne@chromium.org,eugenebut@chromium.org

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

Bug:  661988 ,  859508 
Change-Id: I14e6f2350bad216381224aaf3319018e1c1ffdf1
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/1124279
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572173}
[modify] https://crrev.com/5cf2911fabed50b4b13f648f3ff7f53c641c895c/ios/chrome/app/application_delegate/url_opener_unittest.mm
[modify] https://crrev.com/5cf2911fabed50b4b13f648f3ff7f53c641c895c/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/5cf2911fabed50b4b13f648f3ff7f53c641c895c/ios/chrome/browser/ui/browser_view_controller.h
[modify] https://crrev.com/5cf2911fabed50b4b13f648f3ff7f53c641c895c/ios/chrome/browser/ui/browser_view_controller.mm

I have created issue 859817 to track the behavior we currently want to be consistent with desktop as the revert is also removing the fix.
Project Member

Comment 16 by bugdroid1@chromium.org, Jul 17

Labels: merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/562c0480c7a0cc0ec19ae55a5e4d6de4fa6846da

commit 562c0480c7a0cc0ec19ae55a5e4d6de4fa6846da
Author: Gauthier Ambard <gambard@chromium.org>
Date: Tue Jul 17 06:59:07 2018

Revert "Pass current tab as opener when opening new tab"

This reverts commit 6bdafce48e5663385945c942cb5fbbf5e4edc3f7.

Reason for revert: This CL is breaking the child ordering of tabs creating with Long Press on a link -> Open in a New Tab. They are now opened after the last page opened with ToolsMenu -> Open New Tab. See  crbug.com/859508 

Original change's description:
> Pass current tab as opener when opening new tab
>
> Pass the current tab as the opener when opening a new tab.
> This allow selection the previous current tab when the
> tab is closed via "pull-to-close" action or via the new
> "close" action in the UI refresh.
>
> The current tab is passed when tab is opened via ([*]
> denotes that this behaviour is added by this CL):
> - open tab in background
> - "New Tab" menu action [*]
> - captive portal detection [*]
> - search by image [*]
>
> Bug:  661988 
> Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
> Change-Id: Iecf41a8c0b3cfe465b3ff95a1061c97739b4ed79
> Reviewed-on: https://chromium-review.googlesource.com/1068680
> Reviewed-by: Eugene But <eugenebut@chromium.org>
> Reviewed-by: Mark Cogan <marq@chromium.org>
> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561427}

TBR=marq@chromium.org,sdefresne@chromium.org,eugenebut@chromium.org

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

Bug:  661988 ,  859508 
Change-Id: I14e6f2350bad216381224aaf3319018e1c1ffdf1
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/1124279
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#572173}(cherry picked from commit 5cf2911fabed50b4b13f648f3ff7f53c641c895c)
Reviewed-on: https://chromium-review.googlesource.com/1139953
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#691}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/562c0480c7a0cc0ec19ae55a5e4d6de4fa6846da/ios/chrome/app/application_delegate/url_opener_unittest.mm
[modify] https://crrev.com/562c0480c7a0cc0ec19ae55a5e4d6de4fa6846da/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/562c0480c7a0cc0ec19ae55a5e4d6de4fa6846da/ios/chrome/browser/ui/browser_view_controller.h
[modify] https://crrev.com/562c0480c7a0cc0ec19ae55a5e4d6de4fa6846da/ios/chrome/browser/ui/browser_view_controller.mm

Issue verified 
Version: 
Chrome Beta 68.0.3440.70
Chrome Canary 69.0.3496.0
Device: iPhone 8
iOS: 11.4

Revert "Pass current tab as opener when opening new tab"

Beta
https://drive.google.com/open?id=1J3u_9SIzOGZklpC7LXWNQv29rMoRP2g0

Canary
https://drive.google.com/open?id=1-PJPufUJlMO9n3nqR6JTJiKTUgd8YgvO

Sign in to add a comment