Fake omnibox doesn’t load smooth on cold start with full screen flags enabled |
|||||||||||||
Issue descriptionApp Version: 71.0.3578.39 Beta iOS Version: 10.3.3, 11.4.1, 12.0.1, 12.1 beta Device: iPhone only Prerequisite: 1. Enable ‘#out-of-web-fullscreen’ flag from chrome://flags 2. Enable ‘#browser-container-fullscreen’ flag from chrome://flags Steps to reproduce: 1. Launch chrome app and set the above mentioned flags 2. Open New tab page 3. Cold start the app Observed results: Notice that fake omnibox shrinks and then displays in correct size before loading NTP. Expected results: NTP should load smoothly Note: This issue is identified after the fix of issue: 896234 Number of times you were able to reproduce: 5/5 Bug reproducible after clean install: Yes Bug reproducible after clearing cache and cookies: Yes Bug reproducible on Chrome Mobile on Android: Not tested Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA Bug reproducible on current stable build (App Version, iOS Version): NA on M70 (Feature available from M71) Bug reproducible on the current beta channel build (App Version, iOS Version): Yes on M71 Beta Link to Video: https://drive.google.com/file/d/1F4tkg6o85k6M8q_PcglluYULNNTO--Lt/view?usp=sharing
,
Nov 7
gambard@ PTAL
,
Nov 14
I have troube reproducing on the latest beta (71.0.3578.54). Could you check if this is still happening?
,
Nov 14
Actually I can reproduce but only on iPhone X.
,
Nov 14
You can reproduce this on M72 as well, but you have to disable kBrowserContainerContainsNTP.
,
Nov 14
gambard@ I am able to reproduce it in other iPhone devices as well. Please refer to recorded video for iPhone8(iOS 11.4.1) Link to video: https://drive.google.com/file/d/1QHOGELpoOtBEV4AERgrAW6P3jrJ5IdYQ/view?usp=sharing
,
Nov 14
Reply to Comment #5- As Justin mentioned, issue is reproducible on disabling the flag 'kBrowserContainerContainsNTP' tested in 72.0.3610.0 Canary Link to video: https://drive.google.com/file/d/1lKPl5bZwiSPIoijxuZCD4Zuw2HZwnCeJ/view?usp=sharing
,
Nov 15
Also it looks like, its more consistently reproduced if you have a unread reading list entry or any of the Most Visited tile with title spans into two rows. gambard@ may be can you add few more sites to your most visited row (with titles that could span across two rows) and then try to repro?
,
Nov 16
Turns out that I am able to reproduce on Canary but not on a locally built version. I have a potential fix under review.
,
Nov 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d480d852d012ef72817eeae8165c6c472a6e9285 commit d480d852d012ef72817eeae8165c6c472a6e9285 Author: Gauthier Ambard <gambard@chromium.org> Date: Tue Nov 20 17:31:46 2018 [iOS] Change the doodle top margin calculation This CL changes the way the top margin of the doodle is computed when the fullscreen flags are enabled and the NTP isn't contained directly in BVC. It uses the StatusBarHeight() instead of the passed topMargin which is wrong on cold start. Bug: 902259 Test: M71 fix, test all types of new tab animation and ntp layout. Change-Id: I33af04db418664c6fd033477329353d645379f4c Reviewed-on: https://chromium-review.googlesource.com/c/1337498 Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Cr-Commit-Position: refs/heads/master@{#609737} [modify] https://crrev.com/d480d852d012ef72817eeae8165c6c472a6e9285/ios/chrome/browser/ui/content_suggestions/BUILD.gn [modify] https://crrev.com/d480d852d012ef72817eeae8165c6c472a6e9285/ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm [modify] https://crrev.com/d480d852d012ef72817eeae8165c6c472a6e9285/ios/chrome/browser/ui/util/ui_util.mm
,
Nov 21
Fixed in Canary. Could we have testing on it today so we can ask for merge request? In addition to this issue, this fix might impact the animation when opening the new tab. Please check if they are correct. Justin@: How do you feel about merging this to M71?
,
Nov 21
[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.
,
Nov 21
Verified in Build Version: 72.0.3617.0 Canary Devices: iPhone X(iOS 12.0.1), iPhone 8(iOS 11.4.1) Followed steps from comment#0. Fake Omnibox loads smooth on cold start. Also tested different scenarios on 'open new tab' animation and no noticeable issues identified, looks good.
,
Nov 21
Thanks!
,
Nov 21
This bug requires manual review: Less than 9 days to go before AppStore submit on M71 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
,
Nov 21
Approved pending justincohen@'s approval.
,
Nov 21
,
Nov 21
Approved.
,
Nov 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1ff6a083ea118e23e2b88f1f2bd4be3e8cc739df Commit: 1ff6a083ea118e23e2b88f1f2bd4be3e8cc739df Author: gambard@chromium.org Commiter: gambard@chromium.org Date: 2018-11-22 08:24:33 +0000 UTC [iOS] Change the doodle top margin calculation This CL changes the way the top margin of the doodle is computed when the fullscreen flags are enabled and the NTP isn't contained directly in BVC. It uses the StatusBarHeight() instead of the passed topMargin which is wrong on cold start. Cherry-pick of: crrev.com/c/1337498 Bug: 902259 Test: M71 fix, test all types of new tab animation and ntp layout. Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: Ieb72ff63a5e8a6bc84ca6a51ec3163b47d021367 Reviewed-on: https://chromium-review.googlesource.com/c/1346307 Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#795} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Nov 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1ff6a083ea118e23e2b88f1f2bd4be3e8cc739df commit 1ff6a083ea118e23e2b88f1f2bd4be3e8cc739df Author: Gauthier Ambard <gambard@chromium.org> Date: Thu Nov 22 08:24:33 2018 [iOS] Change the doodle top margin calculation This CL changes the way the top margin of the doodle is computed when the fullscreen flags are enabled and the NTP isn't contained directly in BVC. It uses the StatusBarHeight() instead of the passed topMargin which is wrong on cold start. Cherry-pick of: crrev.com/c/1337498 Bug: 902259 Test: M71 fix, test all types of new tab animation and ntp layout. Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: Ieb72ff63a5e8a6bc84ca6a51ec3163b47d021367 Reviewed-on: https://chromium-review.googlesource.com/c/1346307 Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#795} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/1ff6a083ea118e23e2b88f1f2bd4be3e8cc739df/ios/chrome/browser/ui/content_suggestions/BUILD.gn [modify] https://crrev.com/1ff6a083ea118e23e2b88f1f2bd4be3e8cc739df/ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm [modify] https://crrev.com/1ff6a083ea118e23e2b88f1f2bd4be3e8cc739df/ios/chrome/browser/ui/ui_util.mm
,
Nov 26
Verified in Build Version: 71.0.3578.72 Beta Devices: iPhone 8(iOS 11.4.1), iPhone X(iOS 11.4.1), iPhone 8plus(iOS 12.1.1 beta #3), iPhone 6s plus(iOS 10.3.3), iPhone SE(iOS 12.0.1), iPad 2018(iOS 11.4.1) Followed steps from comment#0. Fake Omnibox loads smooth on cold start. Also tested different scenarios on 'open new tab' animation and no noticeable issues identified, looks good. Link to video: https://drive.google.com/file/d/1PZaxFd0Hnrh1qf7MjEyp4Am-fgcjazFv/view?usp=sharing
,
Dec 3
Fix has already been merged. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by twelling...@chromium.org
, Nov 6