"Open in New Tab" does't open in the next tab |
|||||||||
Issue descriptionWith UI Refresh flag enabled What steps will reproduce the problem? (1) Open a WebPage (wikipedia.com) (2) Open a WebPage in a new tab using toolsMenu -> Open New Tab (3) Open a WebPage in a new again in the same way. (4) Go back to the first website (wikipedia.com) (5) Open a link in a new page using long press -> Open in a New Tab What is the expected result? The new page should be opened in the following tab. What happens instead? The page is opened between the tab opened in (2) and the one opened in (3)
,
Jul 2
This was introduced by https://chromium-review.googlesource.com/c/chromium/src/+/1068680. sdefresne@: What do you think? We could add a way to differentiate between tabs opened with "open new tab" and tabs opened with long press -> Open in New Tab.
,
Jul 3
I think "Open new Tab" button and "Open in new Tab" on long press should use different code path. AFAICT, the former one uses -[BrowserViewController openNewTab:] while the latter uses -[BrowserViewController webPageOrderedOpen:referrer:inIncognito:inBackground:appendTo:].
So, the fix is to just change -[BrowserViewController openNewTab:] to pass nil to opener when invoking -[BrowserViewController addSelectedTabWithURL:transition:opener:].
That is do the following change:
git diff
diff --git a/ios/chrome/browser/ui/browser_view_controller.mm b/ios/chrome/browser/ui/browser_view_controller.mm
index f320171fd78d..c839b494f34b 100644
--- a/ios/chrome/browser/ui/browser_view_controller.mm
+++ b/ios/chrome/browser/ui/browser_view_controller.mm
@@ -4666,7 +4666,7 @@ applicationCommandEndpoint:(id<ApplicationCommands>)applicationCommandEndpoint {
}
[self addSelectedTabWithURL:GURL(kChromeUINewTabURL)
transition:ui::PAGE_TRANSITION_TYPED
- opener:currentTab];
+ opener:nil];
}
- (void)printTab {
,
Jul 3
BTW, the new behaviour is what was asked, i.e. do the same thing as desktop where tab opened with the "new tab button" are marked as opened by the current tab, allowing you to go back to the previous tab when they are closed.
,
Jul 3
I now, I have discussed with Mardini about it, we will revert the CL introducing the behavior. Also on desktop you are going back to the previous tab only if you do nothing. If you navigate in the newly opened page or switch tab then come back, closing it doesn't let you go back to its parent.
,
Jul 3
If we want to keep the opener-opened relationship, then a new flag needs to be passed to -addSelectedTabWithURL:transition:opener: to indicate the position of the newly inserted Tab because the logic is to append the Tab or put it just after the opener if WebStateList::INSERT_FORCE_INDEX is not used.
,
Jul 3
Then do not revert the whole CL, just make the change suggested in comment #3.
,
Jul 3
The CL is only adding support for adding an opener when using ToolsMenu->Open New Tab and LongPress -> Search Image. After checking, we also want to remove the behavior for Search Image. What would be the point of keeping the additional argument in the methods if we always pass nil?
,
Jul 3
If we also remove it for Search Image, then yes there is no point in keeping the change. Though the behaviour described in the current bug is the one that was asked in https://bugs.chromium.org/p/chromium/issues/detail?id=661988.
,
Jul 3
We decided that the old behavior was better than this bug. I have created a revert: https://chromium-review.googlesource.com/c/chromium/src/+/1124279
,
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 3
Asking for merge request on M68 as it is also reproducible without UI Refresh flag.
,
Jul 3
This bug requires manual review: Less than 17 days to go before AppStore submit on M68 Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 3
+kariahda@ for merge request approval.
,
Jul 10
Can we get canary verification please.
,
Jul 12
+Tests team for canary verification before merge.
,
Jul 16
Issue verified Version: Chrome Canary 69.0.3493.0 Device: iPad iOS: 12.0 beta Page is opened in the following tab. Not in between the tab opened in (2) and the one opened in (3) from original description. https://drive.google.com/open?id=12X0YRTsVbbcauZTDm-QASdo14r5mvEvZ
,
Jul 16
Approved!
,
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 |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by gambard@chromium.org
, Jul 2