Closed
Bug 933739
Opened 11 years ago
Closed 10 years ago
Issues in URL domain autocompletion
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox27 wontfix, firefox28 wontfix, firefox29 wontfix, firefox30 wontfix, firefox31 verified, relnote-firefox -, fennec+)
RESOLVED
FIXED
Firefox 31
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
Reporter | ||
Updated•11 years ago
|
Whiteboard: [testday-20131101]
Comment 1•11 years ago
|
||
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•11 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.
Comment 3•11 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? 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•11 years ago
|
||
Assignee | ||
Comment 5•11 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•11 years ago
|
Assignee: nobody → lucasr.at.mozilla
Flags: needinfo?(lucasr.at.mozilla)
Comment 6•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b0b537deacb9
https://hg.mozilla.org/mozilla-central/rev/b0b537deacb9
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Comment 9•11 years ago
|
||
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•11 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.
Comment 11•11 years ago
|
||
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).
Comment 12•11 years ago
|
||
... so what's the story here? What am I doing wrong?
Comment 13•11 years ago
|
||
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.
Comment 14•11 years ago
|
||
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•11 years ago
|
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Assignee | ||
Comment 15•11 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•11 years ago
|
||
Assignee | ||
Comment 17•11 years ago
|
||
Assignee | ||
Comment 18•11 years ago
|
||
Assignee | ||
Comment 19•11 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•11 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•11 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•11 years ago
|
Attachment #830290 -
Flags: review? → review?(bnicholson)
Comment 22•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(lucasr.at.mozilla)
Comment 23•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #830291 -
Flags: review?(bnicholson)
Updated•11 years ago
|
Attachment #830292 -
Flags: review?(bnicholson)
Assignee | ||
Comment 24•11 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•11 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 26•11 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]: ----------------------------------------------------------------- Ah, yes it is. Looks good.
Attachment #830292 -
Flags: review?(bnicholson) → review+
Updated•11 years ago
|
status-firefox27:
--- → ?
status-firefox28:
--- → affected
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
tracking-fennec: ? → +
Comment 27•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [testday-20131101] → [testday-20131101] [leave open]
Comment 28•11 years ago
|
||
(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 29•11 years ago
|
||
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
Assignee | ||
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
Assignee | ||
Comment 33•10 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•10 years ago
|
Attachment #830290 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #830291 -
Attachment is obsolete: true
Assignee | ||
Comment 34•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8395620 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 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)
Updated•10 years ago
|
Attachment #8395619 -
Flags: review?(bnicholson) → review+
Comment 36•10 years ago
|
||
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•10 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•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/968a2848d8ab
Whiteboard: [testday-20131101] [leave open] → [testday-20131101]
Comment 39•10 years ago
|
||
(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•10 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]
Assignee | ||
Comment 42•10 years ago
|
||
Assignee | ||
Comment 43•10 years ago
|
||
Assignee | ||
Comment 44•10 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•10 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)
Updated•10 years ago
|
Attachment #8399914 -
Flags: review?(bnicholson) → review+
Comment 46•10 years ago
|
||
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•10 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
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 28 → Firefox 31
Updated•10 years ago
|
relnote-firefox:
--- → ?
Updated•10 years ago
|
Comment 49•10 years ago
|
||
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 ;)
Assignee | ||
Comment 50•10 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.
Comment 51•10 years ago
|
||
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)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•