No url displayed on loading webpages |
|||||||||
Issue descriptionApp Version: 70.0.3501.0 Canary iOS Version: 11.4.1, 12 Beta 4 Device: iPhone 8plus, iPhone 7plus, iPhone 7 Steps to reproduce: 1. Launch chrome 2. Load any webpage 3. Tap on the toolbar omnibox 4. Scroll up to hide all the suggestions with one finger and tap on (X) button Observed results: Notice that the url doesn’t appear in the omnibox and also the url doesn’t appear on loading any other webpage in a different tab Expected results: Url should appear Number of times you were able to reproduce: 5/5 Bug reproducible after clean install: Yes/No Bug reproducible after clearing cache and cookies: Yes/No 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): No on M67 and M68 (UI refresh is available from M69) Bug reproducible on the current beta channel build (App Version, iOS Version): Yes on M69 Link to video: https://drive.google.com/file/d/1W0diUY1YTeOapKwUBEsIAyDTEcO9H2WN/view?usp=sharing
,
Jul 24
,
Jul 25
This is an interesting issue. I don't think it's easy to reproduce so I'm removing RBS. Under the hood, when you scroll the suggestions, the textfield stops being the first responder so that the keyboard can disappear and let the user see all the suggestions. Then when you tap CLEAR, we refocus omnibox to let the user start typing right away (with no extra tap in the content area). This tries to trigger a focusing animation. I think we might want to change this, because we're already in the final state, so there is no need to animate. The code path is: [OmniboxViewController clearButtonPressed] -> -[AutocompleteTextFieldDelegate textFieldDidBeginEditing:] -> OmniboxViewIOS::OnDidBeginEditing() -> ... -> [AutocompleteTextFieldDelegate textFieldDidBeginEditing] -> .. -> WebOmniboxEditController::OnSetFocus() -> .. -> [BVC locationBarHasBecomeFirstResponder] -> .. [PrimaryToolbarCoordinator transitionToLocationBarFocusedState] I think at this point either BVC or PrimaryToolbarCoordinator should early-return in case the omnibox popup is shown. Then in OmniboxViewIOS::ClearText() we call -[OmniboxTextFieldIOS becomeFirstResponder] AGAIN: // Calling OnDidChange() can trigger a scroll event, which removes focus from // the omnibox. [field_ becomeFirstResponder]; ^ I have to check if we need this workaround again. I really doubt we need two calls to -becomeFirstResponder on the same runloop. Maybe we can omit the first one. Finally, the slight moving of the finger that holds the suggestions list up causes the [OmniboxPopupViewController scrollViewDidScroll:] to make the textfield resign first responder, all on the same runloop. This goes all the way to PrimaryToolbarCoordinator again. We shouldn't do this either: I guess the best way to prevent this would be to have the exclusiveTouch on the clear button and the textfield to prevent users from scrolling the popup and tapping on things at the same time. As a temporary fix, I propose https://chromium-review.googlesource.com/c/chromium/src/+/1150143 that will prevent the focus/defocus animations to happen when one such animation is already in progress. But we should really fix the two underlying problems too, after that we probably should make demanding a focus/defocus animation when one is already in progress DCHECK.
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f2569c5796282362db08eb2d40f76c6b9c3e3dd5 commit f2569c5796282362db08eb2d40f76c6b9c3e3dd5 Author: stkhapugin@chromium.org <stkhapugin@chromium.org> Date: Wed Jul 25 13:36:05 2018 Prevent conflicting focus/defocus omnibox animations. Adds a flag to the animation orchestrator to ignore all calls when there is an animation in progress. Bug: 866858 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ic8990b0c60083723d9e6727538293480cc42b9a6 Reviewed-on: https://chromium-review.googlesource.com/1150143 Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org> Cr-Commit-Position: refs/heads/master@{#577860} [modify] https://crrev.com/f2569c5796282362db08eb2d40f76c6b9c3e3dd5/ios/chrome/browser/ui/orchestrator/omnibox_focus_orchestrator.mm
,
Jul 25
For future refinement, I think it would make more sense to have the PrimaryToolbar handling this instead of BVC. Not sure the BVC has knowledge of the location bar.
,
Jul 25
Given the unlikely repro steps, I don't think this is Pri 1 after all, so I don't think we need to merge this. Kariah, do you think this is merge-worthy?
,
Jul 25
I would like to merge, even if the repro steps are unlikely, because the bug removes the URL from the omnibox. Are there any concerns that the fix might be risky and cause other regressions?
,
Jul 25
I think this fix should be merged. URL is very important to browsing and the bug affects all later opened tabs as well. Going off of Rohit's questions, I would also like to know your level of confidence in the fix?
,
Jul 26
OK, let's merge this to 69. I'm 100% confident that the fix will only reduce the number of weird states the omnibox can be in and improve overall toolbar/omnibox stability. Using the repro steps from step 1 I can sometimes still enter a weird state when the toolbar is expanded when it shouldn't be (so it shows the Cancel button and appears editable), but the user can recover by tapping on the textfield or cancel, and also the URL is visible. IMO this is a lot better - the UI doesn't look broken, and the recovery is intuitive. I want to stress that the user needs to do something really extraordinary to enter this state. To completely eliminate the weirdness, much more invasive fixes from comment #3 need to be implemented. I don't think they should be cherry-picked once implemented and they will require a lot more work :)
,
Jul 26
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 26
Great. Let's get canary verification before merge please.
,
Jul 27
Tested in M70.0.3504.0 Canary in iPhone 8plus(iOS 11.4.1) and iPhone 7(iOS 10.3.3) As mentioned in comment#9, Omnibox goes in to state where it displays cancel button and editable field but tapping on cancel button doesn't recover to normal state. This issue looks partially fixed Note: Tap on cancel button after focusing omnibox will recover the omnibox to normal state Link to video: https://drive.google.com/file/d/19Y3z5uRLx6U-cV-bEa8Gzepz8Zp6n0IN/view?usp=sharing
,
Jul 27
Since the bug is "no URL is displayed on loading webpages", and in the state in comment #12 you can see the URL, I consider this verified.
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/206bbd6a97354cbe8e45758f7bac41dde01893f8 commit 206bbd6a97354cbe8e45758f7bac41dde01893f8 Author: stkhapugin@chromium.org <stkhapugin@chromium.org> Date: Fri Jul 27 12:47:53 2018 Prevent conflicting focus/defocus omnibox animations. Adds a flag to the animation orchestrator to ignore all calls when there is an animation in progress. Bug: 866858 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ic8990b0c60083723d9e6727538293480cc42b9a6 Reviewed-on: https://chromium-review.googlesource.com/1150143 Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577860}(cherry picked from commit f2569c5796282362db08eb2d40f76c6b9c3e3dd5) Reviewed-on: https://chromium-review.googlesource.com/1152769 Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#147} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/206bbd6a97354cbe8e45758f7bac41dde01893f8/ios/chrome/browser/ui/orchestrator/omnibox_focus_orchestrator.mm
,
Aug 1
Verified in: App Version: 69.0.3497.22 beta Devices: iPhone 7 Plus, iPhone 6 Plus iOS Versions:10.3.3, 11.4.1 URL is displayed on loading webpages.
,
Aug 1
@stkhapugin, a separate issue (http://crbug/869833) is created for tracking the issue with cancel button that doesn't restore the omnibox to normal state (as per comment#12) |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by stkhapugin@chromium.org
, Jul 24Owner: stkhapugin@chromium.org
Status: Assigned (was: Untriaged)