New issue
Advanced search Search tips

Issue 859508 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug
Q2



Sign in to add a comment

"Open in New Tab" does't open in the next tab

Project Member Reported by gambard@chromium.org, Jul 2

Issue description

With 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)
 
The bug is caused by the fact that the "last tab opened" of a tab is counting the tabs opened with ToolsMenu->OpenNewTab.
Cc: sdefresne@chromium.org
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.
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 {




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.


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.
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.
Then do not revert the whole CL, just make the change suggested in comment #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?
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.
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
Project Member

Comment 11 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

Labels: ReleaseBlock-Stable M-68 Merge-Request-68
Status: Fixed (was: Assigned)
Asking for merge request on M68 as it is also reproducible without UI Refresh flag.
Project Member

Comment 13 by sheriffbot@chromium.org, Jul 3

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
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
Cc: kariahda@chromium.org
+kariahda@ for merge request approval.
Can we get canary verification please.
Cc: linds...@chromium.org srikanthg@chromium.org
+Tests team for canary verification before merge.
Status: Verified (was: Fixed)
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
Labels: -Hotlist-Merge-Review -Merge-Review-68 Merge-Approved-68
Approved!
Project Member

Comment 19 by bugdroid1@chromium.org, Jul 17

Labels: -merge-approved-68 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

Sign in to add a comment