If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Issues in URL domain autocompletion

RESOLVED FIXED in Firefox 31

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: Soumya Kanti Chakraborty, Assigned: lucasr)

Tracking

26 Branch
Firefox 31
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox27 wontfix, firefox28 wontfix, firefox29 wontfix, firefox30 wontfix, firefox31 verified, relnote-firefox -, fennec+)

Details

(Whiteboard: [testday-20131101] )

Attachments

(5 attachments, 4 obsolete attachments)

1.68 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
1.17 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
1.89 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
1.00 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
2.73 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Steps to reproduce:

1) Opened www.wikipedia.org in one tab. It loads completely.
2) Opened about:home in a separate tab, and tapped on the URL bar and typed "w"


Actual results:

Only "w" remained in the URL bar and nothing else came up.


Expected results:

The existing URL is substituted by "w" followed by the rest of the URL("ikipedia.org") which is highlighted
(Reporter)

Updated

4 years ago
Whiteboard: [testday-20131101]
Brian, we auto-complete on second character insert; whereas desktop auto-completes on first-character insert, what was the explanation?

Note: Typing 'w' will still show you switch-to-tab for your open Wikipedia tab; and we'll still get search suggestions if you opt-in.
Flags: needinfo?(bnicholson)
(Reporter)

Comment 2

4 years ago
(In reply to Aaron Train [:aaronmt] from comment #1)
> Brian, we auto-complete on second character insert; whereas desktop
> auto-completes on first-character insert, what was the explanation?
In the test case it was written first letter, but for me even after 2nd letter I don't get it auto completed. 

> 
> Note: Typing 'w' will still show you switch-to-tab for your open Wikipedia
> tab; and we'll still get search suggestions if you opt-in.
Yes that is showing up.
(In reply to Aaron Train [:aaronmt] from comment #1)
> Brian, we auto-complete on second character insert; whereas desktop
> auto-completes on first-character insert, what was the explanation?

Testing Nightly on my HTC One, I see URL autocomplete after just one character. If it requires more than that, I suspect we're hitting an IME bug. I didn't write the autocomplete logic, though, so moving NEEDINFO to Lucas.
Flags: needinfo?(bnicholson) → needinfo?(lucasr.at.mozilla)
(Assignee)

Comment 4

4 years ago
Created attachment 827496 [details] [diff] [review]
Don't handle text changes if not in editing mode (r=bnicholson)
(Assignee)

Comment 5

4 years ago
Comment on attachment 827496 [details] [diff] [review]
Don't handle text changes if not in editing mode (r=bnicholson)

The state of the autocompletion bits is getting messed because of text changes happening while the toolbar is *not* in editing mode. We shouldn't care about text changes if we're not in editing mode.
Attachment #827496 - Flags: review?(bnicholson)
(Assignee)

Updated

4 years ago
Assignee: nobody → lucasr.at.mozilla
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 827496 [details] [diff] [review]
Don't handle text changes if not in editing mode (r=bnicholson)

Review of attachment 827496 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.
Attachment #827496 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/b0b537deacb9
https://hg.mozilla.org/mozilla-central/rev/b0b537deacb9
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
On Nightly (11/06), I don't get the auto-completion as per the expected results as in comment #0 still. What does this patch do?
(Assignee)

Comment 10

4 years ago
(In reply to Aaron Train [:aaronmt] from comment #9)
> On Nightly (11/06), I don't get the auto-completion as per the expected
> results as in comment #0 still. What does this patch do?

This is working for me in latest Nightly. The patch ensures the first type letter will trigger autocompletion as expected.
I open Wikipedia in a tab; open a new tab, hit 'w', and don't get any auto-completion at all in the URL field. I do get results though (e.g, switch to tab for Wikipedia) -- Nightly (11/06).
... so what's the story here? What am I doing wrong?
I only get the autocomplete when I type a '.' cnn. completes to cnn.com but any of the previous letters do not. Google keyboard Android 4.3.
I'm actually seeing this too. Basically, enter a URL for any site and go, open editing mode again, and type one letter. Alternatively, I can enter a few letters, hit back to close the keyboard, tap the URL bar again to open the keyboard, and the enter one letter. In both cases, I never see suggestions for that one letter. 

Lucas, do we ever reset mAutoCompleteResult after leaving editing mode? Given the symptoms, it looks like we're comparing the single letter to the last full string that was typed, mistaking it for a deletion.
Flags: needinfo?(lucasr.at.mozilla)

Updated

4 years ago
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
(Assignee)

Comment 15

4 years ago
(In reply to Brian Nicholson (:bnicholson) from comment #14)
> I'm actually seeing this too. Basically, enter a URL for any site and go,
> open editing mode again, and type one letter. Alternatively, I can enter a
> few letters, hit back to close the keyboard, tap the URL bar again to open
> the keyboard, and the enter one letter. In both cases, I never see
> suggestions for that one letter. 
> 
> Lucas, do we ever reset mAutoCompleteResult after leaving editing mode?
> Given the symptoms, it looks like we're comparing the single letter to the
> last full string that was typed, mistaking it for a deletion.

Yeah, it seems my patch only covered the issue with the first autocompletion once fennec starts. More patches coming.
Flags: needinfo?(lucasr.at.mozilla)
(Assignee)

Comment 16

4 years ago
Created attachment 830290 [details] [diff] [review]
Reset autocompletion state when entering reader mode (r=bnicholson)
(Assignee)

Comment 17

4 years ago
Created attachment 830291 [details] [diff] [review]
Reset search term when BrowserSearch is detached (r=bnicholson)
(Assignee)

Comment 18

4 years ago
Created attachment 830292 [details] [diff] [review]
Use given search term in handleAutocomplete, not mSearchTerm (r=bnicholson)
(Assignee)

Comment 19

4 years ago
Comment on attachment 830290 [details] [diff] [review]
Reset autocompletion state when entering reader mode (r=bnicholson)

Ensures clean state in BrowserToolbar before the first autocompletion.
Attachment #830290 - Flags: review?
(Assignee)

Comment 20

4 years ago
Comment on attachment 830291 [details] [diff] [review]
Reset search term when BrowserSearch is detached (r=bnicholson)

BrowserSearch instance is reused across multiple searches. This patch resets state once it gets detached.
Attachment #830291 - Flags: review?(bnicholson)
(Assignee)

Comment 21

4 years ago
Comment on attachment 830292 [details] [diff] [review]
Use given search term in handleAutocomplete, not mSearchTerm (r=bnicholson)

Slightly related. We were using the wrong searchterm value when using handleAutocomplete. This should avoid potential racy behaviour.
Attachment #830292 - Flags: review?(bnicholson)
(Assignee)

Updated

4 years ago
Attachment #830290 - Flags: review? → review?(bnicholson)
Even with these patches, I think there are autocomplete issues when unfocusing/focusing the URL bar within edit mode itself. For example:

1) Open editing mode
2) Type "w" to make wikipedia.org appear
3) Click back to close the keyboard (but stay in editing mode)
4) Click the URL bar again
5) Type "f" to make facebook.com appear

Result:
Nothing happens; I have to enter the subsequent "a" before seeing the facebook.com autocomplete suggestion.

This happens since there's some disconnect between our editing mode state and the actual EditText editing state. Rather than using the editing mode state like you have in your patches, could we use IME focus events?
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 830290 [details] [diff] [review]
Reset autocompletion state when entering reader mode (r=bnicholson)

Cancelling review for now.
Attachment #830290 - Flags: review?(bnicholson)
Attachment #830291 - Flags: review?(bnicholson)
Attachment #830292 - Flags: review?(bnicholson)
(Assignee)

Comment 24

4 years ago
Comment on attachment 830292 [details] [diff] [review]
Use given search term in handleAutocomplete, not mSearchTerm (r=bnicholson)

Review of attachment 830292 [details] [diff] [review]:
-----------------------------------------------------------------

This one is good for review anyway.
Attachment #830292 - Flags: review?(bnicholson)
(Assignee)

Comment 25

4 years ago
(In reply to Brian Nicholson (:bnicholson) from comment #22)
> Even with these patches, I think there are autocomplete issues when
> unfocusing/focusing the URL bar within edit mode itself. For example:
> 
> 1) Open editing mode
> 2) Type "w" to make wikipedia.org appear
> 3) Click back to close the keyboard (but stay in editing mode)
> 4) Click the URL bar again
> 5) Type "f" to make facebook.com appear
> 
> Result:
> Nothing happens; I have to enter the subsequent "a" before seeing the
> facebook.com autocomplete suggestion.
> 
> This happens since there's some disconnect between our editing mode state
> and the actual EditText editing state. Rather than using the editing mode
> state like you have in your patches, could we use IME focus events?

Nice catch. Yeah, sounds like the right thing to do.
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 830292 [details] [diff] [review]
Use given search term in handleAutocomplete, not mSearchTerm (r=bnicholson)

Review of attachment 830292 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, yes it is. Looks good.
Attachment #830292 - Flags: review?(bnicholson) → review+
status-firefox27: --- → ?
status-firefox28: --- → affected
tracking-fennec: --- → ?
tracking-fennec: ? → +
Comment on attachment 830292 [details] [diff] [review]
Use given search term in handleAutocomplete, not mSearchTerm (r=bnicholson)

https://hg.mozilla.org/integration/fx-team/rev/8f90c8059733
Whiteboard: [testday-20131101] → [testday-20131101] [leave open]
(In reply to Mark Finkle (:mfinkle) from comment #27)
> Comment on attachment 830292 [details] [diff] [review]
> Use given search term in handleAutocomplete, not mSearchTerm (r=bnicholson)
> 
> https://hg.mozilla.org/integration/fx-team/rev/8f90c8059733

Part of a backout
https://hg.mozilla.org/integration/fx-team/rev/871fab42fa64
Comment on attachment 830292 [details] [diff] [review]
Use given search term in handleAutocomplete, not mSearchTerm (r=bnicholson)

https://hg.mozilla.org/integration/fx-team/rev/6b1d80107aad
https://hg.mozilla.org/mozilla-central/rev/6b1d80107aad
(Assignee)

Comment 31

4 years ago
Created attachment 8395619 [details] [diff] [review]
Reset autocompletion state when the toolbar gets focus (r=bnicholson)
(Assignee)

Comment 32

4 years ago
Created attachment 8395620 [details] [diff] [review]
Reset BrowserSearch filter on detach or focus change (r=bnicholson)
(Assignee)

Comment 33

4 years ago
Comment on attachment 8395619 [details] [diff] [review]
Reset autocompletion state when the toolbar gets focus (r=bnicholson)

This is now a little better encapsulated with the new toolbar code.
Attachment #8395619 - Flags: review?(bnicholson)
(Assignee)

Updated

4 years ago
Attachment #830290 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #830291 - Attachment is obsolete: true
(Assignee)

Comment 34

4 years ago
Created attachment 8395622 [details] [diff] [review]
Reset BrowserSearch filter when the toolbar loses focus (r=bnicholson)
(Assignee)

Updated

4 years ago
Attachment #8395620 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
Comment on attachment 8395622 [details] [diff] [review]
Reset BrowserSearch filter when the toolbar loses focus (r=bnicholson)

This covers the issue pointed out in comment #22.
Attachment #8395622 - Flags: review?(bnicholson)
Attachment #8395619 - Flags: review?(bnicholson) → review+
Comment on attachment 8395622 [details] [diff] [review]
Reset BrowserSearch filter when the toolbar loses focus (r=bnicholson)

Review of attachment 8395622 [details] [diff] [review]:
-----------------------------------------------------------------

mSearchTerm is also used for the search engine suggestion views, so won't clearing mSearchTerm break them? If you were to type something and then the URL bar loses focus, this looks it will make search suggestions disappear.
Attachment #8395622 - Flags: review?(bnicholson) → review-
(Assignee)

Comment 37

4 years ago
Comment on attachment 8395622 [details] [diff] [review]
Reset BrowserSearch filter when the toolbar loses focus (r=bnicholson)

(In reply to Brian Nicholson (:bnicholson) from comment #36)
> Comment on attachment 8395622 [details] [diff] [review]
> Reset BrowserSearch filter when the toolbar loses focus (r=bnicholson)
> 
> Review of attachment 8395622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> mSearchTerm is also used for the search engine suggestion views, so won't
> clearing mSearchTerm break them? If you were to type something and then the
> URL bar loses focus, this looks it will make search suggestions disappear.

This patch doesn't cause any issues in the search suggestion views but it seems this patch is not even necessary to fix this bug :-)
Attachment #8395622 - Attachment is obsolete: true
(Assignee)

Comment 38

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/968a2848d8ab
Whiteboard: [testday-20131101] [leave open] → [testday-20131101]
(In reply to Lucas Rocha (:lucasr) from comment #37)
> This patch doesn't cause any issues in the search suggestion views but it
> seems this patch is not even necessary to fix this bug :-)

Hmm...I just tried this locally, and I have to disagree on both points.

There's still a case where autocomplete fails with these STR:
1) Type "w" to show an autocomplete result, e.g., wikipedia.org
2) Touch the result list to make the URL bar lose focus
3) Touch the URL bar and type "w" again

After step 3, you should see wikipedia.org as an autocomplete suggestion again, but it doesn't appear.

Having your other patch applied fixes this issue, but I see the issue I mentioned in comment 36 with these STR:
1) Type something (e.g., "fkdlajf") to make the search engine results at the bottom visible
2) Touch the result list

After step 2, the list goes completely blank for me.
Flags: needinfo?(lucasr.at.mozilla)
(Assignee)

Comment 40

4 years ago
Grrr, didn't test this properly. Let's leave this open.
Flags: needinfo?(lucasr.at.mozilla)
Whiteboard: [testday-20131101] → [testday-20131101] [leave open]
https://hg.mozilla.org/mozilla-central/rev/968a2848d8ab
(Assignee)

Comment 42

4 years ago
Created attachment 8399914 [details] [diff] [review]
Make mSearchTerm volatile in BrowserSearch (r=bnicholson)
(Assignee)

Comment 43

4 years ago
Created attachment 8399915 [details] [diff] [review]
Trigger autocompletion even if the search term hasn't changed (r=bnicholson)
(Assignee)

Comment 44

4 years ago
Comment on attachment 8399914 [details] [diff] [review]
Make mSearchTerm volatile in BrowserSearch (r=bnicholson)

Ensures proper thread visibility as mSearchTerm is changed and accessed from multiple threads in BrowserSearch.
Attachment #8399914 - Flags: review?(bnicholson)
(Assignee)

Comment 45

4 years ago
Comment on attachment 8399915 [details] [diff] [review]
Trigger autocompletion even if the search term hasn't changed (r=bnicholson)

The remaining issue seems to happen because we don't trigger the SearchLoader if the search term passed to the filter() call is the same (in which case the autocompletion just doesn't happen).

We need to ensure we always trigger autocompletion (even if the search term hasn't changed) but still avoid running unnecessary queries for the same search term.

This can be easily accomplished by initializing the loader instead of restarting when the search term is the same. This will reuse any query results cached in the loader but always trigger an onLoadFinished() call.
Attachment #8399915 - Flags: review?(bnicholson)
Attachment #8399914 - Flags: review?(bnicholson) → review+
Comment on attachment 8399915 [details] [diff] [review]
Trigger autocompletion even if the search term hasn't changed (r=bnicholson)

Review of attachment 8399915 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: mobile/android/base/home/BrowserSearch.java
@@ +689,5 @@
>          GeckoAppShell.unregisterEventListener(eventName, this);
>      }
>  
> +    private void restartSearchLoader() {
> +        SearchLoader.restart(getLoaderManager(), LOADER_ID_SEARCH, mCursorLoaderCallbacks, mSearchTerm);        

Nit: trailing ws
Attachment #8399915 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 47

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/cbbf33ba0c77
https://hg.mozilla.org/integration/fx-team/rev/9f527e5397c2
Whiteboard: [testday-20131101] [leave open] → [testday-20131101]
https://hg.mozilla.org/mozilla-central/rev/cbbf33ba0c77
https://hg.mozilla.org/mozilla-central/rev/9f527e5397c2
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 28 → Firefox 31

Updated

4 years ago
relnote-firefox: --- → ?
status-firefox27: ? → wontfix
status-firefox28: affected → wontfix
status-firefox29: --- → wontfix
status-firefox30: --- → wontfix
status-firefox31: --- → fixed
I added this item in the release notes with the description "Improved autocompletion". If you want something more precise, don't hesitate to propose something better (which is not going to be hard ;)
relnote-firefox: ? → 31+
(Assignee)

Comment 50

4 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #49)
> I added this item in the release notes with the description "Improved
> autocompletion". If you want something more precise, don't hesitate to
> propose something better (which is not going to be hard ;)

Not sure if it's worth mentioning this bug fix in the release notes. Yes, it's an important-ish fix but I don't expect a lot of people to notice it.
Verified as fixed in:
Build: Nightly 31.0a1 (2014-04-18)
Devices: Asus Transformer (Android 4.2.1), Nexus 4 (Android 4.4.2) and Acer Iconia (Android 3.2.1)
status-firefox31: fixed → verified
Not in the release notes. cf comment #50
relnote-firefox: 31+ → -
You need to log in before you can comment on or make changes to this bug.