NTP fakebox to focused omnibox transition is jumpy in compact x regular |
|||||||||||
Issue description1. 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)
,
Jul 19
,
Jul 20
,
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
,
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
,
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
,
Jul 23
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?
,
Jul 24
,
Jul 24
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
,
Jul 24
The revert narrows the scope of this change to ui-refresh only, so I'll cherry-pick 1142946 and 1146209 together.
,
Jul 24
Let's get canary verification please.
,
Jul 25
+ 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.
,
Jul 25
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.
,
Jul 25
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
,
Jul 25
This fix might have created issue 867458 .
,
Jul 25
Wrong bug link in comment#14. Further refinements will be tracked here http://crbug/867455
,
Jul 25
,
Jul 25
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
,
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
,
Aug 1
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 |
|||||||||||
Comment 1 by kkhorimoto@chromium.org
, Jul 19Status: Assigned (was: Untriaged)