New issue
Advanced search Search tips

Issue 895963 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Oct 26
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug



Sign in to add a comment

New incognito tab animation wrong with browser-container-fullscreen enabled.

Project Member Reported by justincohen@chromium.org, Oct 16

Issue description

It's just a blank page animation
 
Cc: -kkhorimoto@chromium.org
Components: UI>Browser>FullScreen
Labels: -Pri-2 Pri-1
Owner: kkhorimoto@chromium.org
Status: Assigned (was: Untriaged)
Status: Started (was: Assigned)
Cc: kkhorimoto@chromium.org
Owner: justincohen@chromium.org
Status: Assigned (was: Started)
Okay, I've tracked down the cause of this bug, but I'm not sure how to fix it.  Here are my findings:

1) The snapshot for the new incognito tab is nil because we return the webState->GetView().superview when the fullscreen-out-of-web experiment is enabled.  If we update this to return webState->GetView() when BVC.inNewTabAnimation == YES, we get a non-nil snapshot.
2) After performing that update, we get a non-nil snapshot, but the IncognitoView's subviews aren't updated yet, even after [[self viewForTab:tab] layoutIfNeeded] in BVC's |-animateNewTab:inForegroundWithCompletion:|.  The IncognitoView is resized properly, but the subviews aren't arranged yet.  Adding "[nativeView setNeedsLayout]; [nativeView layoutIfNeeded];" to CRWWebControllerContainerView seems to trigger the layout of these subviews, but it's unclear if that's actually what we want.  Probably okay for a cherry-pickable solution though.
3) If we follow the above steps, we produce a snapshot that's *almost* right, although the frame is not completely correct, as there's a small jump at the end.

Sending this back to you, Justin, since you're probably the most knowledgeable about NTP/IncognitoView.  You can reassign to me or we can chat about it tomorrow if necessary. 
What about something like this?... could probably be cleaned up more, but should work?

diff --git a/ios/chrome/browser/ui/browser_view_controller.mm b/ios/chrome/browser/ui/browser_view_controller.mm
index 65df0e116616..183fbebed710 100644
--- a/ios/chrome/browser/ui/browser_view_controller.mm
+++ b/ios/chrome/browser/ui/browser_view_controller.mm
@@ -5037,6 +5037,10 @@ applicationCommandEndpoint:(id<ApplicationCommands>)applicationCommandEndpoint {
                                ->UpdateSnapshot(/*with_overlays=*/true,
                                                 /*visible_frame_only=*/true);
     newPage = pageScreenshot;
+    if (base::FeatureList::IsEnabled(web::features::kOutOfWebFullscreen)) {
+      newPage = [self viewForTab:tab];
+      newPage.userInteractionEnabled = NO;
+    }
     offset =
         pageScreenshot.frame.size.height - pageScreenshot.image.size.height;
   }


Project Member

Comment 6 by bugdroid1@chromium.org, Oct 25

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

commit 2a201b52ac9d8c22c389f5e2f3a22d227aebe6ae
Author: Justin Cohen <justincohen@google.com>
Date: Thu Oct 25 21:45:39 2018

[ios] Fix new incognito animation with kBrowserContainerFullscreen enabled.

Bug:  895963 
Change-Id: Ifd765373e79bf7c1b86842d601c44a300173decf
Reviewed-on: https://chromium-review.googlesource.com/c/1297931
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602866}
[modify] https://crrev.com/2a201b52ac9d8c22c389f5e2f3a22d227aebe6ae/ios/chrome/browser/ui/browser_view_controller.mm

Cc: linds...@chromium.org
Status: Fixed (was: Started)
lindsayw@ Can someone from Test verify the non-NTP not-from-tab-grid new-tab animations  before we merge request?
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-71; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-71 label, otherwise remove Merge-TBD label. Thanks.
Cc: vbarig...@chromium.org
HYD can verify, cc'ing vbarigela for assignment
vbarigela@ This change affects more than just the incognito new tab animation from the toolbar.  With browser-container-fullscreen and out-of-web features enabled, can you review all the new tab animations (including tapping on a web link that opens a new page).

This does *not* affect new tab animations initiated from the tab grid, or opening an NTP (non-incognito).

Thanks!
Status: Verified (was: Fixed)
Verified in 72.0.3592.0 Canary in iPhone 8plus(iOS 12 beta 5), iPhone 8(iOS 11.4.1), iPad Air(iOS 12 beta 5)

In chrome://flags enable:
    - Browser Container Fullscreen
    - Fullscreen implementation out of web
    - "Fullscreen Viewport Adjustment Mode" to "Smooth Scrolling"

Tested around the following scenarios in both normal and incognito tabs and functionality looks good
1. Tab animation initiated from menu, tool bar and tab grid
2. Open links from context menu in to new tab/incognito tab 
3. Tab animation from todays view and 3D Force touch
4. Tab animation/full screen behavior on changing device orientation
5. Pull to refresh actions

All scenarios looks good. Will keep testing this feature and report if any issues are identified.

Link to Video:

Before Fix:
https://drive.google.com/file/d/12ACxTPFBlH34tDlU9bbGpzooXqLZ0k42/view?usp=sharing

After Fix:
https://drive.google.com/file/d/1B87p7hDIvSYEjfrTi8IXLSY2x9iZ9QDM/view?usp=sharing
 
Labels: -Merge-TBD Merge-Request-71
rakurati@ Thanks for the thorough testing!
Project Member

Comment 13 by sheriffbot@chromium.org, Oct 26

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Can this fix possibly brake anything else?
What is 'else' in your question?

The change affects any new tab animation that is not opening an NTP, and not from the tab switcher.  See comment #10 for why we asked Test to do a more thorough sweep.
Labels: -Merge-Review-71 Merge-Approved-71
By "else" I mean anything not related to the fix so in this case anything not related to incognito tab animation. C10 will suffice. Thank you, approved.

I ask this question because sometimes a fix could possibly affect other things and if known, this is good information to have before a merge.
Project Member

Comment 17 by bugdroid1@chromium.org, Nov 1

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d8762aa87df90141c7da7df0369b9664c0551a29

commit d8762aa87df90141c7da7df0369b9664c0551a29
Author: Justin Cohen <justincohen@google.com>
Date: Thu Nov 01 16:27:15 2018

[ios] Fix new incognito animation with kBrowserContainerFullscreen enabled.

TBR=justincohen@google.com

(cherry picked from commit 2a201b52ac9d8c22c389f5e2f3a22d227aebe6ae)

Bug:  895963 
Change-Id: Ifd765373e79bf7c1b86842d601c44a300173decf
Reviewed-on: https://chromium-review.googlesource.com/c/1297931
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602866}
Reviewed-on: https://chromium-review.googlesource.com/c/1312342
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#446}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/d8762aa87df90141c7da7df0369b9664c0551a29/ios/chrome/browser/ui/browser_view_controller.mm

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/d8762aa87df90141c7da7df0369b9664c0551a29

Commit: d8762aa87df90141c7da7df0369b9664c0551a29
Author: justincohen@google.com
Commiter: justincohen@chromium.org
Date: 2018-11-01 16:27:15 +0000 UTC

[ios] Fix new incognito animation with kBrowserContainerFullscreen enabled.

TBR=justincohen@google.com

(cherry picked from commit 2a201b52ac9d8c22c389f5e2f3a22d227aebe6ae)

Bug:  895963 
Change-Id: Ifd765373e79bf7c1b86842d601c44a300173decf
Reviewed-on: https://chromium-review.googlesource.com/c/1297931
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602866}
Reviewed-on: https://chromium-review.googlesource.com/c/1312342
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#446}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Verified in  71.0.3578.39 Beta in iPhone 8plus(iOS 11.4.1), iPhoneX(iOS 12.1 Beta), iPhone 6 plus (iOS 10.3.3), iPad 2018(iOS 11.4.1)

New incognito tab animation looks good.

Note: Also tested around over scroll actions, open web links in new tab. functionality looks good

Link to video:
https://drive.google.com/file/d/1_tkcN7LmIoS_3OmKRQ6_hQSy6rlxJ1UC/view?usp=sharing

Sign in to add a comment