New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 865408 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

NTP fakebox to focused omnibox transition is jumpy in compact x regular

Project Member Reported by pschaffner@chromium.org, Jul 19

Issue description

1. open an NTP
2. focus the omnibox without scrolling (note the transition is OK when the NTP is scrolled and the fakebox is pinned to the top of the screen)
 
Owner: gambard@chromium.org
Status: Assigned (was: Untriaged)
Cc: -justincohen@chromium.org gambard@chromium.org
Owner: justincohen@chromium.org
Summary: NTP fakebox to focused omnibox transition is jumpy in compact x regular (was: NTP fakebox to focused omnibox transition is jumpy in CxR)
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 22

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

commit a00d30759a041703602e5a36b52ebede9a74919e
Author: Justin Cohen <justincohen@google.com>
Date: Sun Jul 22 01:28:39 2018

[ios] Clean up NTP focus animation.

Previously the NTP 'scroll up' animation and the omnibox 'focus' animation happened
concurrently when tapping on the NTP fakebox.  Instead, chain these animations, so
the NTP fakebox 'scroll up' animations completes and then focuses the real omnibox.

Also changes how the 'scroll up' animation works to use a CADisplayLink, so each
tick of the scroll updates the fakebox shape and hint label size.

See: https://drive.google.com/open?id=11dbkXTl0V0n5WODLiBYezpdkH-eUvxKO

Bug:  865408 , 865822
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I85d8d193178602a434e4f0007513c38be69e9fec
Reviewed-on: https://chromium-review.googlesource.com/1142946
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577086}
[modify] https://crrev.com/a00d30759a041703602e5a36b52ebede9a74919e/ios/chrome/browser/prerender/prerender_egtest.mm
[modify] https://crrev.com/a00d30759a041703602e5a36b52ebede9a74919e/ios/chrome/browser/ui/content_suggestions/content_suggestions_header_synchronizer.mm
[modify] https://crrev.com/a00d30759a041703602e5a36b52ebede9a74919e/ios/chrome/browser/ui/content_suggestions/content_suggestions_header_view_controller.mm
[modify] https://crrev.com/a00d30759a041703602e5a36b52ebede9a74919e/ios/chrome/browser/ui/content_suggestions/ntp_home_egtest.mm
[modify] https://crrev.com/a00d30759a041703602e5a36b52ebede9a74919e/ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm
[modify] https://crrev.com/a00d30759a041703602e5a36b52ebede9a74919e/ios/chrome/browser/ui/toolbar/toolbar_egtest.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 22

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/6e47d6511179012592c3f724c8734fe684be6285

commit 6e47d6511179012592c3f724c8734fe684be6285
Author: Justin Cohen <justincohen@google.com>
Date: Sun Jul 22 01:39:22 2018

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 23

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

commit 51af7905fe5918a644cd5953c6c87b1ad7089b49
Author: Justin Cohen <justincohen@google.com>
Date: Mon Jul 23 10:47:14 2018

[ios] Revert fakebox animation change for pre-UI refresh.

Bug:  865408 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I43ace7047750f8754cf6882d35fd598c3fffda3a
Reviewed-on: https://chromium-review.googlesource.com/1146209
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577139}
[modify] https://crrev.com/51af7905fe5918a644cd5953c6c87b1ad7089b49/ios/chrome/browser/ui/content_suggestions/content_suggestions_header_view_controller.mm

Cc: mard...@chromium.org pinkerton@chromium.org ghendel@chromium.org
Status: Fixed (was: Assigned)
Marking as fixed per pschaffner@ spec to chain the animations, but I'm not convinced it looks any better.

mardini@/pinkerton@ghendel@ can you chime in here?  What do you think? 
Labels: Merge-Request-69
Project Member

Comment 9 by sheriffbot@chromium.org, Jul 24

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
The revert narrows the scope of this change to ui-refresh only, so I'll cherry-pick 1142946 and 1146209 together.
Let's get canary verification please.
Cc: martijnb@chromium.org
Status: Assigned (was: Fixed)
+ Martijn to weigh in

Thanks for looping me in, Justin. I agree with you. The chained animation looks too discrete and sequential. It is also a bit asymmetric with the reverse animation, right? (when you hit cancel and animate from omnibox back to fakebox). 

I can't remember how it used to look like before you chained them but I think the current animation should be made smoother and more fluid. 

Martijn, do you have a suggestion on how to improve this? Maybe you can have a quick chat with Justin before he heads out.
Status: Fixed (was: Assigned)
There are other animation changes in here I'd like to verify and get into the branch.  We can continue to tweak there, I hope, with a followup bug.
Status: Verified (was: Fixed)
Omnibox transition animation is looking so far. Further refinements will be tracked here http://crbug/866836

Verified the animation on M70.0.3502.0 canary
iPhoneSE, iPhoneX, iPhone7Plus
iOS11.4.1, 12.0 beta#4, 10.3.3
This fix might have created  issue 867458 .
Wrong bug link in comment#14.
Further refinements will be tracked here http://crbug/867455
Labels: -Hotlist-Merge-Review -Merge-Review-69 Merge-Approved-69
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 25

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18818301e2511c9a956bad5096018d4e536848ac

commit 18818301e2511c9a956bad5096018d4e536848ac
Author: Justin Cohen <justincohen@google.com>
Date: Wed Jul 25 17:40:12 2018

[ios] Clean up NTP focus animation.

Previously the NTP 'scroll up' animation and the omnibox 'focus' animation happened
concurrently when tapping on the NTP fakebox.  Instead, chain these animations, so
the NTP fakebox 'scroll up' animations completes and then focuses the real omnibox.

Also changes how the 'scroll up' animation works to use a CADisplayLink, so each
tick of the scroll updates the fakebox shape and hint label size.

See: https://drive.google.com/open?id=11dbkXTl0V0n5WODLiBYezpdkH-eUvxKO

Bug:  865408 , 865822
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I85d8d193178602a434e4f0007513c38be69e9fec
Reviewed-on: https://chromium-review.googlesource.com/1142946
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#577086}(cherry picked from commit a00d30759a041703602e5a36b52ebede9a74919e)
Reviewed-on: https://chromium-review.googlesource.com/1150342
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#77}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/18818301e2511c9a956bad5096018d4e536848ac/ios/chrome/browser/prerender/prerender_egtest.mm
[modify] https://crrev.com/18818301e2511c9a956bad5096018d4e536848ac/ios/chrome/browser/ui/content_suggestions/content_suggestions_header_synchronizer.mm
[modify] https://crrev.com/18818301e2511c9a956bad5096018d4e536848ac/ios/chrome/browser/ui/content_suggestions/content_suggestions_header_view_controller.mm
[modify] https://crrev.com/18818301e2511c9a956bad5096018d4e536848ac/ios/chrome/browser/ui/content_suggestions/ntp_home_egtest.mm
[modify] https://crrev.com/18818301e2511c9a956bad5096018d4e536848ac/ios/chrome/browser/ui/qr_scanner/qr_scanner_view_controller_egtest.mm
[modify] https://crrev.com/18818301e2511c9a956bad5096018d4e536848ac/ios/chrome/browser/ui/toolbar/toolbar_egtest.mm

Project Member

Comment 19 by bugdroid1@chromium.org, Jul 25

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

commit 16d194db4c9a5b94c62c14e26b3810f80f2824b4
Author: Justin Cohen <justincohen@google.com>
Date: Wed Jul 25 17:41:26 2018

[ios] Revert fakebox animation change for pre-UI refresh.

Bug:  865408 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I43ace7047750f8754cf6882d35fd598c3fffda3a
Reviewed-on: https://chromium-review.googlesource.com/1146209
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#577139}(cherry picked from commit 51af7905fe5918a644cd5953c6c87b1ad7089b49)
Reviewed-on: https://chromium-review.googlesource.com/1150343
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#78}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/16d194db4c9a5b94c62c14e26b3810f80f2824b4/ios/chrome/browser/ui/content_suggestions/content_suggestions_header_view_controller.mm

Verified the issue on the build 69.0.3497.22 beta tested on iPhone(iOS 11.4).
Tapping on fake omnibox iss smooth with out any flickering anb jumpy, looks good

Sign in to add a comment