Closed Bug 1131685 Opened 9 years ago Closed 9 years ago

left arrow key only skips the last character of the highlighted search suggestion

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox39 --- verified

People

(Reporter: kjozwiak, Assigned: enndeakin)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached image poc.gif
When you have search suggestions being listed under the search toolbar, selecting one of the suggestions using the arrow keys and than pressing the left arrow on a particular suggestion will only go to the left once (one character). Pressing it the second time onwards won't do anything.

An example would be a user searching for something and wants to either correct the current selected suggestions or append another term. Right now, the user would only be able to move backwards once (a single character).

Reproducible on all the OS's:
- Win 8.1 x64
- OSX 10.10.2 x64
- Ubuntu 14.04.1 x64

STR:

- click on the search toolbar and type "firefox"
- once you get the suggested terms appearing, press the down arrow key and select a suggestion from the list
- once one of the listed suggestions is highlighted, press the "left" arrow key and you'll notice that you'll only be able to skip the last character and won't be able to go any further.

Attached a .gif of the issue (notice that I can only skip the last character and can't go any further than that)
Attached patch Fix (obsolete) — Splinter Review
Skip all of the autocomplete handling when the popup is open and noRollupOnCaretMove is set.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
This is sort of a regression caused by 1116865, although without that change the popup would close and not cause the issue when the caret was moved twice.
Points: --- → 2
No longer depends on: 1114707
Flags: firefox-backlog+
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Attachment #8562787 - Flags: review?(mak77)
Comment on attachment 8562787 [details] [diff] [review]
Fix

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

there is a behavioral change here, where left/right/home with this change won't autofill anymore, making this field act very differently from the other autocompletes.
In most cases the suggested behavior sounds fine, but what if an ac field doesn't have completeSelectedIndex, but it has noRollup? If I select an entry and press right, I'd expect the selected entry to be completed, so I can keep typing after it, but it won't be completed anymore.

Maybe we should make it be if (noRollup && "CompleteSelectedIndex"), so basically noRollup will only work for CompleteSelectedIndex fields?

Alternatively, isn't the problem here the call to
input->SelectTextRange(value.Length(), value.Length());
this seems to make sense only when the cursor is already at the end before we setTextIndex, would changing that make sense more generally for autocomplete fields?
Flags: needinfo?(enndeakin)
> Alternatively, isn't the problem here the call to
> input->SelectTextRange(value.Length(), value.Length());
> this seems to make sense only when the cursor is already at the end before
> we setTextIndex, would changing that make sense more generally for
> autocomplete fields?

I tried both of these and the latter doesn't feel quite right. The selected item gets filled in inconsistently. For instance, when at the end of the field and moving the caret left, the item gets filled, the textbox changes and the cursor is one step from the end.
Flags: needinfo?(enndeakin)
Attached patch searchcarettwiceSplinter Review
This is the former suggestion.
Attachment #8562787 - Attachment is obsolete: true
Attachment #8562787 - Flags: review?(mak77)
Attachment #8567111 - Flags: review?(mak77)
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Comment on attachment 8567111 [details] [diff] [review]
searchcarettwice

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

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +491,5 @@
>      uint32_t minResultsForPopup;
>      input->GetMinResultsForPopup(&minResultsForPopup);
>      if (isOpen || (mRowCount > 0 && mRowCount < minResultsForPopup)) {
> +      // For completeSelectedIndex autocomplete fields, if the popup shouldn't
> +      // close when the the caret is moved, don't adjust the value or caret.

typo: the the caret and s/value or caret/position of the caret/
Attachment #8567111 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e73726c6d30
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/9e73726c6d30
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
I was able to reproduce this issue on Firefox 38.0a1 (2015-02-10) using Windows 7 64bit and Ubuntu 14.04 32bit.

Verified fixed on Firefox 39.0a1 (2015-03-12) using Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.