Make the correct tab reselected when closing a tab. |
|||||||||
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.
,
Nov 3 2016
Rohit, who would be a good owner for Tab Model bugs?
,
May 21 2018
Would be nice if we have consistent behavior with Android and Desktop. Do you know happens there?
,
May 21 2018
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?
,
May 22 2018
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.
,
May 22 2018
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?
,
May 22 2018
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.
,
May 22 2018
Re to comment #7: Sorry for the confusion. On Step 4, I close the tab using Pull To Action gesture.
,
May 23 2018
Tentative CL with some additional comments on what to expect at https://chromium-review.googlesource.com/c/chromium/src/+/1068680
,
May 23 2018
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.
,
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
,
May 24 2018
,
May 29 2018
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
,
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
,
Jul 4
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.
,
Jul 17
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
,
Jul 19
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 |
|||||||||
Comment 1 by eugene...@chromium.org
, Nov 3 2016Components: UI>Browser>TabStrip