New issue
Advanced search Search tips

Issue 792503 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Clear text omnibox button is displayed incorrectly

Project Member Reported by srikanthg@chromium.org, Dec 6 2017

Issue description

App Version: 64.0.3282.12 beta
iOS Version: 11.2, 10.3.3
Device: iPhoneX, iPhone7 plus, iPhone6s
URL: any

Steps to reproduce:
  1. Launch Google Chrome
  2. Open a new tab and navigate to www.google.com
  3. In the same page navigate to yahoo.com
  4. Tap on back arrow
  5. Tap on omnibox to focus

Observed results: Observe that Clear text button is displayed towads the center of the omnibix

Expected results: Clear text button should be displayed next to Dismiss button

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): M63 NO
Bug reproducible on the current beta channel build (App Version, iOS Version): M64 YES

Link to video/image: https://drive.google.com/file/d/1IVG_S7qFqMsS9L8xhID--jBYnH1TKQLG/view 
 
Cc: rohitrao@chromium.org
Labels: M-64 ReleaseBlock-Stable
Owner: stkhapugin@chromium.org
Status: Assigned (was: Untriaged)
Status: Started (was: Assigned)
The issue is that LocationBarView has the correct frame, the OmniboxTextfieldIOS has the correct constraints, but somehow no amount of setNeedsLayout/layoutIfNeeded to both of these views triggers an autolayout pass. 
So my best guess is that LocationBarView never allows autolayout to make a pass on its subviews because
 LBV's frame is never directly set; instead it's set as a side effect of setBounds on LBV's layer.

Setting its frame directly seems to help. 
See: https://chromium-review.googlesource.com/c/chromium/src/+/813973


Alternatively, we can revert these commits on M64 branch:
https://chromium-review.googlesource.com/c/chromium/src/+/718106
https://chromium-review.googlesource.com/c/chromium/src/+/758850
https://chromium-review.googlesource.com/c/chromium/src/+/759783
https://chromium-review.googlesource.com/c/chromium/src/+/766367 << This is where autolayout was added


And probably if we do that we should also revert
https://chromium-review.googlesource.com/c/chromium/src/+/758362


I tried to understand why autolayout doesn't work in this case. There seems to not be a lot of docs on
this topic, but here are some relevant quotes from CoreAnimation programming guide:

---
For layer-backed views, it is recommended that you manipulate the view, rather than its layer, whenever
possible. In iOS, views are just a thin wrapper around layer objects, so any manipulations you make to
the layer usually work just fine. But there are cases in both iOS and OS X where manipulating the layer
instead of the view might not yield the desired results. Wherever possible, this document points out 
those pitfalls and tries to provide ways to help you work around them.
---
If you want to use Core Animation classes to initiate animations, you must issue all of your Core 
Animation calls from inside a view-based animation block.
---
If you are using constraint-based layout rules to manage the position of your views, you must remove 
any constraints that might interfere with an animation as part of configuring that animation. 
Constraints affect any changes you make to the position or size of a view. They also affect the 
relationships between the view and its child views. If you are animating changes to any of those items, 
you can remove the constraints, make the change, and then apply whatever new constraints are needed.
---

So in - [WTC animateMaterialOmnibox], we manipulate the view's layer; we don't do it inside of an
animation block; and even though we don't have any interfering constraints per the last citation, I
think this extract demonstrates how little the view.layer's bounds change is related to autolayout.
Cc: gambard@chromium.org sczs@chromium.org stkhapugin@chromium.org
 Issue 789471  has been merged into this issue.

Comment 6 by sczs@chromium.org, Dec 7 2017

Thanks stk!
M-65 should have the clean-toolbar enabled where this issue is not happening. 
Instead of reverting any CL's I'd go with any fix like https://chromium-review.googlesource.com/c/chromium/src/+/813973 that could help on the meantime.  
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 8 2017

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

commit f2dfa460b70841647d1b5dcb7700fd0fd614737c
Author: stkhapugin@chromium.org <stkhapugin@chromium.org>
Date: Fri Dec 08 15:30:21 2017

Set target location bar view frame to trigger autolayout.

Set the view frame in addition to animating view's layer bounds because
just changing layer bounds doesn't allow autolayout inside LBV to
trigger.
Note that after [_locationBarView layer].bounds = toBounds; the
LBV's frame will become LayoutRectGetRect(toLayout) anyway, but the
since the setter is not called, autolayout seems to be confused.
Also note that this is not necessary in new toolbar.

Bug:  792503 ,  789471 , 788814
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: If3ccac317bb8d850f71275ee32c7b82d38f6af24
Reviewed-on: https://chromium-review.googlesource.com/813973
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522784}
[modify] https://crrev.com/f2dfa460b70841647d1b5dcb7700fd0fd614737c/ios/chrome/browser/ui/toolbar/web_toolbar_controller.mm

Status: Fixed (was: Started)
Status: Verified (was: Fixed)
https://drive.google.com/file/d/1Ujp06BalrQZaBGCMfpQVeiNfQFGBHfaU/view?usp=sharing
Verified on M65.0.3292.0 Canary on iPhone X iOS 11.1, iPhone 7+ iOS 10.3.3, iPad Pro 12'5 iOS 11.2
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 18 2017

Labels: merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/843d606a97a222a5ff7556fffb9a1f3eeeab5ca2

commit 843d606a97a222a5ff7556fffb9a1f3eeeab5ca2
Author: stkhapugin@chromium.org <stkhapugin@chromium.org>
Date: Mon Dec 18 15:07:14 2017

Set target location bar view frame to trigger autolayout.

Set the view frame in addition to animating view's layer bounds because
just changing layer bounds doesn't allow autolayout inside LBV to
trigger.
Note that after [_locationBarView layer].bounds = toBounds; the
LBV's frame will become LayoutRectGetRect(toLayout) anyway, but the
since the setter is not called, autolayout seems to be confused.
Also note that this is not necessary in new toolbar.

Bug:  792503 ,  789471 , 788814
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: If3ccac317bb8d850f71275ee32c7b82d38f6af24
Reviewed-on: https://chromium-review.googlesource.com/813973
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#522784}(cherry picked from commit f2dfa460b70841647d1b5dcb7700fd0fd614737c)
Reviewed-on: https://chromium-review.googlesource.com/832646
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#262}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/843d606a97a222a5ff7556fffb9a1f3eeeab5ca2/ios/chrome/browser/ui/toolbar/web_toolbar_controller.mm

Verified in:

App Version: 64.0.3282.41 beta
Devices: iPhone 6S Plus, iPad Mini, iPhone 8 Plus
iOS Versions: 10.3.3, 11.2.5 beta 2

Clear Omnibox button is aligned properly when tapped on omnibox to focus.

Sign in to add a comment