Closed
Bug 1185183
Opened 9 years ago
Closed 9 years ago
[UnifiedComplete] Broken completion after selecting with the keyboard, then editing
Categories
(Toolkit :: Places, defect, P1)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: mbrubeck, Assigned: mbrubeck)
References
Details
(Whiteboard: [unifiedcomplete][fxsearch])
Attachments
(1 file, 2 obsolete files)
4.57 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce (in Firefox Nightly with browser.urlbar.unifiedcomplete enabled): 1. Begin typing a URL in the address bar. 2. Press the "down" key to select the second entry in the autocomplete list. 3. Type more letters (or backspace) to edit the URL, such that the autocomplete popup now shows a different suggestion in the second entry. 4. Press Enter. Expected result: The URL selected in step 2 loads. Actual result; The URL of the new second autocomplete suggestion loads, even though it is not the URL in the address bar when you press Enter. This happens because, when unifiedcomplete is enabled, the urlbar autocomplete binding resets the selectedIndex to 0 when new autocomplete results are added... https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/browser/base/content/urlbarBindings.xml#l1637 ...but nsAutoCompleteController's "mCompletedSelectionIndex" is still set (incorrectly) to 1, so in EnterMatch it will overwrite the user-entered value with whatever value is now at index 1: https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/toolkit/components/autocomplete/nsAutoCompleteController.cpp#l1339 This patch invalidates mCompletedSelectionIndex when the popup is invalidated. This fixes the problem, but I'm not sure whether it's the best fix. This may be related to bug 1173202.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8635633 -
Flags: review?(mak77)
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #0) > This may be related to bug 1173202. Nope, after testing it seems these are different bugs. The patch here does *not* fix bug 1173202. This bug is also *not* affected by the patch in bug 1172937.
Updated•9 years ago
|
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [unifiedcomplete][fxsearch]
Comment 4•9 years ago
|
||
Comment on attachment 8635633 [details] [diff] [review] patch Review of attachment 8635633 [details] [diff] [review]: ----------------------------------------------------------------- writing a mochitest-browser test for this should be easy, you can add a couple visits (PlacesTestUtils.addVisits), then use promiseAutocompleteResultPopup(), simulate a VK_DOWN and check the tab location. See http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_autocomplete_enter_race.js or http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_urlbarEnterAfterMouseOver.js for examples. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1529,5 @@ > // Refresh the popup view to display the new search results > nsCOMPtr<nsIAutoCompletePopup> popup; > input->GetPopup(getter_AddRefs(popup)); > NS_ENSURE_TRUE(popup != nullptr, NS_ERROR_FAILURE); > + mCompletedSelectionIndex = -1; I have a couple problems with this fix: 1. I think there are cases where you can change the text, but that won't issue a new search, as an optimization (don't research if it's likely to give same results, or don't search at all). In such a case we won't go through ProcessResult 2. processResult is invoked every time the search updates the result, the search could just append more matches, but it could also add matches in the middle, after such a call mCompletedSelectionIndex could still be valid, or the entry it was pointing to could have been replaced. with this change we'd lose the selected index information at every call, even if it's still valid. A safer fix could be in EnterMatch to compare inputValue with GetResultValueAt(selectedIndex, false, value), if they differ it's clear the user modified the text and we should not replace value with finalValue.
Attachment #8635633 -
Flags: review?(mak77) → review-
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #0) > 1. Begin typing a URL in the address bar. > 2. Press the "down" key to select the second entry in the autocomplete list. > 3. Type more letters (or backspace) to edit the URL, such that the > autocomplete popup now shows a different suggestion in the second entry. > 4. Press Enter. > > Expected result: The URL selected in step 2 loads. Oops, I mis-typed above. The actual expected result is that the URL entered in step *3* loads, of course.
Assignee | ||
Comment 6•9 years ago
|
||
Added a browser-chrome test and modified the patch based on the suggestions above.
Attachment #8635633 -
Attachment is obsolete: true
Attachment #8638690 -
Flags: review?(mak77)
Comment 7•9 years ago
|
||
Comment on attachment 8638690 [details] [diff] [review] patch v2 Review of attachment 8638690 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch, it looks mostly good. I just have some comments on the test. If something is fishy with the test changes please let me know. Please get a try run with all mochitests on lin/mac/win, just in case. ::: browser/base/content/test/general/browser_autocomplete_edit_completed.js @@ +13,5 @@ > + Services.prefs.setBoolPref("browser.urlbar.unifiedcomplete", true); > + yield* do_test(); > + Services.prefs.setBoolPref("browser.urlbar.unifiedcomplete", false); > + yield* do_test(); > + Services.prefs.clearUserPref("browser.urlbar.unifiedcomplete"); please move this to registerCleanupFunction @@ +17,5 @@ > + Services.prefs.clearUserPref("browser.urlbar.unifiedcomplete"); > +}); > + > +function* do_test() { > + yield new Promise(resolve => waitForFocus(resolve, window)); This should not be needed, since browser-test does this already, focusing gURLBar should be enough. @@ +24,5 @@ > + > + info("Begin typing a URI."); > + gURLBar.value = "http://example.co"; > + EventUtils.synthesizeKey("m", {}); > + yield promiseSearchComplete(); yield promiseAutocompleteResultPopup("http://example.com"); should do too and it's shorter, didn't it work? @@ +32,5 @@ > + let initialIndex = list.selectedIndex; > + > + info("Key Down to select the next item."); > + EventUtils.synthesizeKey("VK_DOWN", {}); > + yield promiseSearchComplete(); This should not be needed, VK_DOWN shouldn't cause a new search, it should just move through the already provided results. I'd expect this to be a no-op (or at a maximum skipping to the end of the current tick)
Attachment #8638690 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Test updated based on review comments. Thanks! Pushed to Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab56fa2b1e6f
Attachment #8638690 -
Attachment is obsolete: true
Attachment #8639561 -
Flags: review+
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7d5c1cc7b5c3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•9 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•