New incognito tab animation wrong with browser-container-fullscreen enabled. |
||||||||||||
Issue descriptionIt's just a blank page animation
,
Oct 24
,
Oct 25
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.
,
Oct 25
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;
}
,
Oct 25
,
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
,
Oct 26
lindsayw@ Can someone from Test verify the non-NTP not-from-tab-grid new-tab animations before we merge request?
,
Oct 26
[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.
,
Oct 26
HYD can verify, cc'ing vbarigela for assignment
,
Oct 26
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!
,
Oct 26
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
,
Oct 26
rakurati@ Thanks for the thorough testing!
,
Oct 26
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
,
Oct 30
Can this fix possibly brake anything else?
,
Oct 31
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.
,
Oct 31
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.
,
Nov 1
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
,
Nov 1
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}
,
Nov 6
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 |
||||||||||||
Comment 1 by linds...@chromium.org
, Oct 17Components: UI>Browser>FullScreen
Labels: -Pri-2 Pri-1
Owner: kkhorimoto@chromium.org
Status: Assigned (was: Untriaged)