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

Issue 755974 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

NewTab animation is not smooth while a phone call is in progress.

Project Member Reported by srikanthg@chromium.org, Aug 16 2017

Issue description

App Version: 62.0.3187.0 canary
iOS Version: 11.0, 10.3.3
Device: iPhone7 plus, iPhone6s
URL: NTP

Steps to reproduce: [From: justincohen@]

  1. Make a phone call from the device
  2. Background the call in progress
  3. Cold start Google Chrome canary
  4. Open a new tab

Observed results: Observe that NewTab animation is not smooth. Bookmarks and RecentTabs icons jumps a little bit while opening up new tab. Also note tapping on tools menu is also not correct.

Expected results: NewTab animation should be correct.

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: NA
Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA
Bug reproducible on current stable build (App Version, iOS Version): M60 Yes
Bug reproducible on the current beta channel build (App Version, iOS Version): M61 Yes

Link to video/image: https://drive.google.com/file/d/0B-xmXLQhjeKuQkdiQ2R4WDkwTDQ/view 
 
Justin: this is the same behavior as the one in stable.
I think this is because the native view is first displayed in 320x412 view then resized. I don't think it is possible to fix it before clean.
WDYT?
Labels: ReleaseBlock-Stable M-62
Owner: justincohen@chromium.org
Status: Assigned (was: Untriaged)
Cc: marq@chromium.org rohitrao@chromium.org
Owner: kkhorimoto@chromium.org
I think it's more complicated than that.  I think the -StatusBarHeight hack in ui_util.mm is no longer valid on iOS11, and a lot of the assumptions made about what happens during a phone call are therefor wrong.

I don't know what the right solution is, in that case.  

For example, this 'fixes' the problem, but I don't know what the real solution is.

kkhorimoto@ can you lend a hand here, I think you know more about BVC layout solutions, perhaps?

diff --git a/ios/chrome/browser/ui/browser_view_controller.mm b/ios/chrome/browser/ui/browser_view_controller.mm
index 183de5d9c4a4..5ed67ce2e22c 100644
--- a/ios/chrome/browser/ui/browser_view_controller.mm
+++ b/ios/chrome/browser/ui/browser_view_controller.mm
@@ -2303,10 +2303,7 @@ bubblePresenterForFeature:(const base::Feature&)feature
 }

 - (UIImageView*)pageFullScreenOpenCloseAnimationView {
-  CGRect viewBounds, remainder;
-  CGRectDivide(self.view.bounds, &remainder, &viewBounds, StatusBarHeight(),
-               CGRectMinYEdge);
-  return [[UIImageView alloc] initWithFrame:viewBounds];
+  return [[UIImageView alloc] initWithFrame:self.contentArea.frame];
 }

 - (UIImageView*)pageOpenCloseAnimationView {
diff --git a/ios/chrome/browser/ui/ui_util.mm b/ios/chrome/browser/ui/ui_util.mm
index 527abe026097..8b3f523292fb 100644
--- a/ios/chrome/browser/ui/ui_util.mm
+++ b/ios/chrome/browser/ui/ui_util.mm
@@ -56,7 +56,7 @@ CGFloat StatusBarHeight() {
   // Checking [UIApplication sharedApplication].statusBarFrame will return the
   // wrong offset when the application is started while in a phone call, so
   // simply return 20 here.
-  return 20;
+  return [UIApplication sharedApplication].statusBarFrame.size.height;
 }

 CGFloat AlignValueToPixel(CGFloat value) {
Cc: kkhorimoto@chromium.org
Owner: justincohen@chromium.org
I'm unfamiliar with this code; the only BVC layout stuff I've worked with is the header height/native controller stuff.  This seems to be specific to UIKit behavior, and I don't know enough about the new tab animations to tell if changing StatusBarHeight() is the right choice here.

Comment 5 by fi...@chromium.org, Sep 4 2017

Labels: zine-triaged
justincohen@ please what's the status on this blocker? We aim to fix our blockers as early in the cycle as possible.
Owner: kkhorimoto@chromium.org
I think the fix for this is going to be very similar to the fix needed for iPhone X status bar handling.  Flipping to kkhorimoto since this won't be something I can realistically fix before going on leave.
Owner: rohitrao@chromium.org
Passing this along to Rohit since I'm working on Bijoi in Paris next week and will be prepping for most of this week.  I can take a look when I get back from Paris if this is still a problem, but again, I'm not familiar with this code.
From: https://www.hackingwithswift.com/articles/12/how-to-update-your-app-design-for-iphone-x

"The status bar is now no longer fixed at 20 points in height, so double check you haven’t hard-coded that figure anywhere. If your app hides the status bar to reclaim space, Apple are asking you to politely reconsider your choice because let’s face it: there wasn’t much you could put up in that notch anyway."

Also note: prefersHomeIndicatorAutoHidden

Labels: -Pri-3 Pri-1
rohitrao@ are you working on this issue? Please update the bug accordingly
Owner: pkl@chromium.org
Peter, this is closely related to all of the iPhoneX statusbar bugs, so it should go to whoever is going to work on those.
Labels: Hotlist-iPhoneX

Comment 14 by pkl@chromium.org, Sep 15 2017

Labels: -Hotlist-iPhoneX
The bug is not specific to iPhone X (original report about iPhone 6/7) and exists in earlier version (at least M60/61). I don't think the root cause is clear, so it's OK to keep the Hotlist-iPhoneX label for now.

However, the priority may need to be re-evaluated.
Cc: pkl@chromium.org
Owner: justincohen@chromium.org
I think I have a fix for this specific bug.  The iPhoneX side of this bug has a much larger scope.

Project Member

Comment 16 by bugdroid1@chromium.org, Sep 15 2017

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

commit 13b7c43252e0c2590ef92e0a27eaa186c7f6b235
Author: Justin Cohen <justincohen@google.com>
Date: Fri Sep 15 12:40:09 2017

[ios] Fix layout issues when starting phone in voice call.

UIKit seems to be insetting our root view controller by 20 points when
in a phone call, but setting our main_view_controller to
self.window.bounds is stomping on that. It's seems that pre-iOS11
this inset would still happen later.  This issue appears when compiling
against the iOS11 SDK, even when running on iOS10 or iOS9 devices, it
seems.

Bug:  755974 
Change-Id: I25b6e3848ae506b5531b5eb28348ac5328a6c779
Reviewed-on: https://chromium-review.googlesource.com/668046
Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502230}
[modify] https://crrev.com/13b7c43252e0c2590ef92e0a27eaa186c7f6b235/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/13b7c43252e0c2590ef92e0a27eaa186c7f6b235/ios/chrome/browser/ui/main/main_coordinator.mm

Status: Fixed (was: Assigned)
I landed a fix unrelated to iPhone X, although later fixes will come for that and are related to crrev/c/668046, this issue is now resolved.
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-62; 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-62 label, otherwise remove Merge-TBD label. Thanks.
Labels: Merge-Request-62
Project Member

Comment 20 by sheriffbot@chromium.org, Sep 15 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: Less than 28 days to go before AppStore submit on M62
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Verified (was: Fixed)
Verified on M63.0.3219.0 canary, dev
Device: iPhone7 plus
iOS: 11 GM seed.
 New tab animation looks good while call is in progress.
Labels: -Hotlist-Merge-Review -Merge-TBD -Merge-Review-62 Merge-Approved-62
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 19 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65537ac4515483363116fa484dfaee95607bb36f

commit 65537ac4515483363116fa484dfaee95607bb36f
Author: Justin Cohen <justincohen@google.com>
Date: Tue Sep 19 18:03:46 2017

[ios] Fix layout issues when starting phone in voice call.

UIKit seems to be insetting our root view controller by 20 points when
in a phone call, but setting our main_view_controller to
self.window.bounds is stomping on that. It's seems that pre-iOS11
this inset would still happen later.  This issue appears when compiling
against the iOS11 SDK, even when running on iOS10 or iOS9 devices, it
seems.

TBR=justincohen@google.com

(cherry picked from commit 13b7c43252e0c2590ef92e0a27eaa186c7f6b235)

Bug:  755974 
Change-Id: I25b6e3848ae506b5531b5eb28348ac5328a6c779
Reviewed-on: https://chromium-review.googlesource.com/668046
Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#502230}
Reviewed-on: https://chromium-review.googlesource.com/673423
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#324}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/65537ac4515483363116fa484dfaee95607bb36f/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/65537ac4515483363116fa484dfaee95607bb36f/ios/chrome/browser/ui/main/main_coordinator.mm

Verified the issue on 62.0.3202.35 beta tested on iPhone7+(11.0.1)
New tab animation looks good while call is in progress.

Sign in to add a comment