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)

defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Whiteboard: [unifiedcomplete][fxsearch])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8635633 - Flags: review?(mak77)
(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.
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [unifiedcomplete][fxsearch]
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-
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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 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+
Attached patch patch v3Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/7d5c1cc7b5c3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.