New issue
Advanced search Search tips

Issue 773647 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Toolbar of stackview needs to adapt to the different height of toolbars.

Project Member Reported by jif@chromium.org, Oct 11 2017

Issue description

Tight now, the StackView considers the height of the toolbar to be constant.
This is a problem on the iPhone X, where the height of the toolbar changes with the orientation.

To reproduce:
(1) On iPhone X, enable the "Safe area compatible toolbar" experimental flag.
(2) Relaunch chrome.
(3) Visit a website (e.g. wikipedia).
(4) Enter the Stack View. Notice that the StackView's toolbar has the correct height.
(5) Leave the StackView.
(6) Change orientation.
(7) Enter the Stack View. Notice that the StackView's toolbar has an incorrect height.

 
Screen Shot 2017-10-11 at 13.50.37.png
301 KB View Download

Comment 1 by jif@chromium.org, Oct 11 2017

A good start would be to add in stack_view_controller.mm the following:

- (void)viewSafeAreaInsetsDidChange {
  [super viewSafeAreaInsetsDidChange];
  CGRect newToolbarFrame = [_toolbarController view].frame;
  newToolbarFrame.size.height = [_toolbarController preferredToolbarHeightWhenAlignedToTopOfScreen];
  [[_toolbarController view] setFrame:newToolbarFrame];
}

Comment 2 by sczs@chromium.org, Oct 11 2017

Cc: jif@chromium.org
Owner: sczs@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by sczs@chromium.org, Oct 11 2017

Labels: Hotlist-iOS11 Hotlist-iPhoneX M-63

Comment 4 by cma...@chromium.org, Oct 19 2017

Sergio are you working on this?

Comment 5 by sczs@chromium.org, Oct 19 2017

I will start today and report on any advances I make. 
Thanks for the reminder Estelle, I know this is high priority so I'll keep you updated.

Comment 6 by jif@chromium.org, Oct 23 2017

Owner: jif@chromium.org
Status: Started (was: Assigned)
Partly fixed by https://chromium-review.googlesource.com/c/chromium/src/+/733163

Comment 7 by jif@chromium.org, Oct 24 2017

Labels: -Pri-3 ReleaseBlock-Stable Pri-1

Comment 8 by jif@chromium.org, Oct 25 2017

Status: Fixed (was: Started)
Fix mentionned in #6 has landed.
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-63; 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-63 label, otherwise remove Merge-TBD label. Thanks.
Cc: -jif@chromium.org justincohen@chromium.org
Owner: pkl@chromium.org
Assigning to you Peter. Do we have plan to merge this again?
Status: Verified (was: Fixed)
Verified on 64.0.3267.0 in iPhoneX simulator 

In Landscape the height of the tool bar looks good in stack view

Link to screenshot:
https://drive.google.com/a/google.com/file/d/1E2IgXfbzAVr0cEafrVEAAyUcvTLxZ3Of/view?usp=sharing 
Labels: -Merge-TBD
pkl@ I guess your answer to comment 10 was No so I am removing merge-TBD here.

Sign in to add a comment