Issue metadata
Sign in to add a comment
|
Clear text omnibox button is displayed incorrectly |
||||||||||||||||||||||
Issue descriptionApp 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
,
Dec 7 2017
,
Dec 7 2017
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.
,
Dec 7 2017
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.
,
Dec 7 2017
Issue 789471 has been merged into this issue.
,
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.
,
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
,
Dec 8 2017
,
Dec 12 2017
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
,
Dec 18 2017
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
,
Dec 20 2017
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 |
|||||||||||||||||||||||
Comment 1 by justincohen@chromium.org
, Dec 6 2017Labels: M-64 ReleaseBlock-Stable
Owner: stkhapugin@chromium.org
Status: Assigned (was: Untriaged)