New issue
Advanced search Search tips

Issue 866858 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug
Q2



Sign in to add a comment

No url displayed on loading webpages

Project Member Reported by rakurati@chromium.org, Jul 24

Issue description

App 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

 
Labels: ReleaseBlock-Stable M-69
Owner: stkhapugin@chromium.org
Status: Assigned (was: Untriaged)
Labels: Q2
Cc: gambard@chromium.org justincohen@chromium.org rohitrao@chromium.org
Labels: -ReleaseBlock-Stable
Status: Started (was: Assigned)
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.  
Project Member

Comment 4 by bugdroid1@chromium.org, 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

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.
Cc: kariahda@chromium.org
Status: Fixed (was: Started)
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? 
Labels: Merge-Request-69
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?
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?
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 :) 
Project Member

Comment 10 by sheriffbot@chromium.org, Jul 26

Labels: -Merge-Request-69 Hotlist-Merge-Approved Merge-Approved-69
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
Great. Let's get canary verification before merge please. 
Status: Assigned (was: Fixed)
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
Status: Verified (was: Assigned)
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.


Project Member

Comment 14 by bugdroid1@chromium.org, Jul 27

Labels: -merge-approved-69 merge-merged-3497
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

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.
@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