Closed Bug 933739 Opened 11 years ago Closed 10 years ago

Issues in URL domain autocompletion

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

26 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

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

RESOLVED FIXED
Firefox 31
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- verified
relnote-firefox --- -
fennec + ---

People

(Reporter: soumyakanti.chakraborty, Assigned: lucasr)

Details

(Whiteboard: [testday-20131101] )

Attachments

(5 files, 4 obsolete files)

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
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
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)
(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)
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: 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+
https://hg.mozilla.org/mozilla-central/rev/b0b537deacb9
Status: UNCONFIRMED → RESOLVED
Closed: 11 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?
(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)
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
(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)
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?
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)
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)
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)
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)
(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+
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
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)
Attachment #830290 - Attachment is obsolete: true
Attachment #830291 - Attachment is obsolete: true
Attachment #8395620 - Attachment is obsolete: true
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-
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
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)
Grrr, didn't test this properly. Let's leave this open.
Flags: needinfo?(lucasr.at.mozilla)
Whiteboard: [testday-20131101] → [testday-20131101] [leave open]
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)
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+
https://hg.mozilla.org/mozilla-central/rev/cbbf33ba0c77
https://hg.mozilla.org/mozilla-central/rev/9f527e5397c2
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 28 → Firefox 31
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 ;)
(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)
Not in the release notes. cf comment #50
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: