New issue
Advanced search Search tips

Issue 918762 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jan 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Tab switcher animation to/from NTP uses too-big snapshot.

Project Member Reported by edchin@google.com, Jan 3

Issue description

App Version (from "Chrome Settings > About Chrome"): TOT, 73.0.3660.0
iOS Version: 12.1
Device: iPad Air 2 simulator

Steps to reproduce: 
1) Open NTP
2) Go to tab switcher
3) Go back to BVC.

Observed behavior: 
4) The snapshot grows too big in the animation. 

Expected behavior: 
4) The tab animation should grow to the exact size of the tab. 

Frequency: 
5/5

Video:
https://drive.google.com/open?id=17AmnVXnSZG110OeV-N_i7_DYu0QohmNH

Additional comments:
I've not tested on M72.

 
Labels: -Restrict-View-Google -ReleaseBlock-Beta ReleaseBlock-Stable
RBB -> RBS, remove RVG.
Status: Started (was: Assigned)
Summary: Tab switcher animation to/from NTP uses too-big snapshot. (was: Tab switcher animation grows too big when animating into tab. )
Note that the animation is the correct size (the toolbar and tab strip, which are are part of the animation, are sized correctly). The issue is that the 'middle section' snapshot is incorrectly sized/positioned when an NTP is being shown.

This is actually happening in both directions; the snapshot used when going from NTP to grid is also incorrectly sized.
Labels: M-72
Repros in M72.
Owner: edchin@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 8

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

commit dedd6bb2cb84c479ddbfbc64ac0b93f6a39feaaa
Author: edchin <edchin@chromium.org>
Date: Tue Jan 08 18:42:19 2019

[ios] Fix iPad snapshot frame

The snapshot frame for iPad NTP is different from iPhone. The snapshot
taken was the wrong size. This CL fixes the frame for the iPad NTP.

Bug:  918762 
Change-Id: I7afca5ef2dc1899f2d7516ffb90f41051c0bac83
Reviewed-on: https://chromium-review.googlesource.com/c/1396925
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620813}
[modify] https://crrev.com/dedd6bb2cb84c479ddbfbc64ac0b93f6a39feaaa/ios/chrome/browser/ui/browser_view_controller.mm

Will confirm in tomorrow's canary, then request merge to M-72. 
Labels: Merge-Request-72
Status: Fixed (was: Started)
Confirmed fixed. Requesting merge to M72. 
The worst that can happen is that the snapshot size continues to be wrong. There's little risk here. This is important to fix because it fixes a bad UI experience. 

Project Member

Comment 8 by sheriffbot@chromium.org, Jan 10

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: Less than 15 days to go before AppStore submit on M72
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Review -Merge-Review-72 Merge-Approved-72
Approved, please merge asap.
Project Member

Comment 10 by sheriffbot@chromium.org, Jan 14

Cc: kariahda@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Merge to M72 completed.
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 14

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d8689865c93f0a3945fda19c12b7e1b77f92f60

commit 7d8689865c93f0a3945fda19c12b7e1b77f92f60
Author: edchin <edchin@chromium.org>
Date: Mon Jan 14 18:17:25 2019

[ios] Fix iPad snapshot frame

The snapshot frame for iPad NTP is different from iPhone. The snapshot
taken was the wrong size. This CL fixes the frame for the iPad NTP.

Bug:  918762 
Change-Id: I7afca5ef2dc1899f2d7516ffb90f41051c0bac83
Reviewed-on: https://chromium-review.googlesource.com/c/1396925
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: edchin <edchin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#620813}(cherry picked from commit dedd6bb2cb84c479ddbfbc64ac0b93f6a39feaaa)
Reviewed-on: https://chromium-review.googlesource.com/c/1409802
Cr-Commit-Position: refs/branch-heads/3626@{#668}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/7d8689865c93f0a3945fda19c12b7e1b77f92f60/ios/chrome/browser/ui/browser_view_controller.mm

Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/7d8689865c93f0a3945fda19c12b7e1b77f92f60

Commit: 7d8689865c93f0a3945fda19c12b7e1b77f92f60
Author: edchin@chromium.org
Commiter: edchin@chromium.org
Date: 2019-01-14 18:17:25 +0000 UTC

[ios] Fix iPad snapshot frame

The snapshot frame for iPad NTP is different from iPhone. The snapshot
taken was the wrong size. This CL fixes the frame for the iPad NTP.

Bug:  918762 
Change-Id: I7afca5ef2dc1899f2d7516ffb90f41051c0bac83
Reviewed-on: https://chromium-review.googlesource.com/c/1396925
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: edchin <edchin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#620813}(cherry picked from commit dedd6bb2cb84c479ddbfbc64ac0b93f6a39feaaa)
Reviewed-on: https://chromium-review.googlesource.com/c/1409802
Cr-Commit-Position: refs/branch-heads/3626@{#668}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Comment 14 by vbarig...@chromium.org, Jan 16 (6 days ago)

Status: Verified (was: Fixed)
Verified on chrome beta version 72.0.3626.59 on iPad 2018 with iOS 11.4.1, 12.1.1, following the steps mentioned in comment #0.  Animation of NTP elements looks good.

Sign in to add a comment