Inline autocomplete: Backspace highlights the last character instead of deleting, when first search result is highlighted

NEW
Unassigned

Status

()

Firefox
Address Bar
P3
normal
6 years ago
4 months ago

People

(Reporter: Fanolian, Unassigned)

Tracking

(Blocks: 2 bugs)

Trunk
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inline-autocomplete:normal][fxsearch])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120120 Firefox/12.0a1
Build ID: 20120120031125

Steps to reproduce:

1. Type any letter in the URL bar.
2. Press down once to highlight the first result. The caret is now at the end of the URL.
3. Press backspace.


Actual results:

The last character of the URL is highlighted instead of deleted. I have to press backspace the second time to start deleting characters.
If I set browser.urlbar.autoFill to false the problem is gone.
(Reporter)

Updated

6 years ago
Component: Untriaged → Location Bar
Confirmed
Blocks: 566489
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 719890
IS this really a dupe? bug 719890 doesn't seem to involve selecting in the popup
Oh, you're right, it's different!
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Looks like on backspace we autoFill again, and since we add a trailing / to autoFill results, backspace is removing this / and autoFill is adding it back.
The global effect is that it looks like backspace selects the last char, but really it removes, adds back and selects.
Status: REOPENED → NEW
Created attachment 591608 [details] [diff] [review]
patch v1.0

The fact is that the code doesn't handle backspacing when both completeDefaultIndex and completeSelectedIndex are enabled, and there is a popup selection. It doesn't consider that the current textValue has been set by the popup selection, so it always compares with mSearchString.

This patch fixes the problem, though, due to trimURL, it's more hackish than I would like.  My idea was to compare the current value with the currently selected value in the popup, but the current value is trimmed, while the result value is not.  What I did is cache the textValue just after completeSelection happens and use this cached value.
Not perfect, but I don't have better ideas for now.

Updated

6 years ago
Depends on: 721225
Created attachment 591771 [details] [diff] [review]
patch v2.0

This is a better alternative to how we detect backspace

Updated

6 years ago
Whiteboard: [inline-autocomplete:normal]
Comment on attachment 591771 [details] [diff] [review]
patch v2.0

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

no hurry, when you have time I'd like some feedback on the approach.
Attachment #591771 - Flags: feedback?(gavin.sharp)
Comment on attachment 591771 [details] [diff] [review]
patch v2.0

Using handleKeyNavigation for this seems a little hacky (backspace isn't really "navigation"), but whatever works I guess.

Should mBackspacedValue be cleared at some point?

Reviewing this thoroughly might be tricky.
Attachment #591771 - Flags: feedback?(gavin.sharp) → feedback+

Updated

6 years ago
QA Contact: untriaged → location.bar
Duplicate of this bug: 789934

Updated

5 years ago
Assignee: nobody → mak77

Updated

5 years ago
Blocks: 835779

Updated

5 years ago
Blocks: 719890
I've been looking at this patch as (a) the bug annoys me but (b) I wanted to better understand how this ties together.

The patch looks alot more complicated than I'm expecting. Note that this first backspace doesn't work correctly, but subsequent ones do. The subsequent ones work as mBackspaced is true - so I was wondering why mBackspaced *isn't* true on the problematic first one when clearly backspace is being pressed.

It looks to me that the problem can be solved by having https://dxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#230 check against mPlaceholderCompletionString instead of/as well as mSearchString. As a simple hack, I just changed to checking against mPlaceholderCompletionString and the STR now work as expected. There's likely some subtleties (eg, we probably want to take into account whether there is a selection or not rather than doing this unconditionally) but it seems a simpler approach to take.

Mak, your patch touches this exact code so I'm sure I'm missing something, but don't know what - can you please enlighten me?
Flags: needinfo?(mak77)
(In reply to Mark Hammond [:markh] from comment #11)
> Mak, your patch touches this exact code so I'm sure I'm missing something,
> but don't know what - can you please enlighten me?

The fact is that the code is trying to "guess" if backspace was used, and it uses a bunch of heuristics to do so. As you can see, that "works"  but it's very fragile to any other change and it's still a guess.
My try with the patch was to actually detect real backspace keypresses, rather than guessing them. Apart that the patch just moves the detection to a common util, so it's not particularly complex.

Sure, there are other ways to fix this bug, what you suggest is likely working, but for ex ample notice mPlaceholderCompletionString has been added 2 years after my patch. That means this bug existed before too, that means this is somehow workarounding the issue.
As you can see, this is really fragile and can break or be fixed by simple changes/additions. That's why I was rather trying to find a better solution to solve it once and for all, and to keep the code in a single place.
Flags: needinfo?(mak77)
ITSM mBackspaced is itself a hack - I don't see any reason "last char removed" is different than "any manual edit" - it's just that the former means new matches are almost always identical, whereas an edit at the start matches nothing (and ones in the middle may match some, but that's an edge-case)

So I think I'm saying "mBackspaced" is the too-narrow solution to a more general "user is editing this url" state (in which case auto-complete and event location changes should not change the being-typed value). No doubt the devil's in the detail though...
yes, that's correct, mBackspaced is only a way to say "we are likely able to reuse something"
I noticed that pressing "left + delete" doesn't have the same problem - it seems like it should. The reason is that as "left" is processed, we hit https://dxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#564 - which unconditionally sets mSearchString to the current value in the input box. This means the subsequent "delete" is treated as a backspace given mSearchString is what we expect.

So this code is confusing :) It's not clear when it's safe to unconditionally update the search string and when it's not - at face value, if mSearchString gets updated as soon as you press an arrow key, it seems safe to set it at basically *any* time :) Maybe the code handling the arrow keys also needs to handle VK_END - conceptually this seems identical to the arrow keys.

Alternatively, another way to "solve" this is to have autocomplete.xml do |this.mController.searchString = val;| as it sends the "ValueChanged" event - this causes the mBackspaced to be set to true correctly - but I'm still no closer to understand what else breaks (and indeed, why the code does what it does at all)
I feel your pain. the controller is a pile of hacks with few comments, changing anything can cause regressions in too many places. That's why so far we tried to look for small self-contained features, and planned a rewrite to split the awesomebar controller from other autocomplete controllers.
It's possible mSearchString is not updated often enough, the problem is having comprehensive testing to be sure we don't regress the awesomebar, form fill, passwords fill and any other stuff using the autocomplete widget.
Btw, now that we are at the beginning of the iteration, we could more easily take changes to be tested in nightly.

(In reply to Mark Hammond [:markh] from comment #15)
> I noticed that pressing "left + delete" doesn't have the same problem - it
> seems like it should. The reason is that as "left" is processed, we hit
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> autocomplete/nsAutoCompleteController.cpp#564 - which unconditionally sets
> mSearchString to the current value in the input box. This means the
> subsequent "delete" is treated as a backspace given mSearchString is what we
> expect.

Note that is a very recent fix Matt landed a few days ago... Again, that's an unrelated fix that is somehow workarounding the problem of backspace detection.
Created attachment 8606864 [details] [diff] [review]
0007-Bug-719888-VK_END-and-mouse-clicks-update-the-autoco.patch

So I'm sure it's no surprise to anyone that the autocompletecontroller is a mess. It looks like that code is nearly an adult, so I think it's just been getting drunk with its friends recently...

> Note that is a very recent fix Matt landed a few days ago... Again, that's an unrelated
> fix that is somehow workarounding the problem of backspace detection.

Hrm - I'm not sure that's unrelated - it seems very relevant.  That bug (bug 998893) is updating the search string whenever keyboard navigation inside the input box is performed. However, that code does *not* handle VK_END - if it did, the obvious STR for this bug would also be fixed. I can't see a reason why every other navigation key has this handling but not VK_END - so I added that key too.

What that leaves unfixed is when using the mouse to select the end of the field and then press backspace. ISTM that a mouse-click inside the field is conceptually the same as using the arrow keys to move the caret, so I added a click handler to also update the search string.

With these changes I can't reproduce the bug and all existing tests seem to pass. I should really add a test-case for this, but thought I'd get feedback on this approach before wasting any more time on this.

Matt, did you get any other insights from working on that bug that might indicate if this approach is viable?
Attachment #8606864 - Flags: feedback?(MattN+bmo)
Comment on attachment 8606864 [details] [diff] [review]
0007-Bug-719888-VK_END-and-mouse-clicks-update-the-autoco.patch

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

Sorry, I don't think I have any insight to add to this nor do I have time to swap in the context of all the history now.
Attachment #8606864 - Flags: feedback?(MattN+bmo)
Comment on attachment 8606864 [details] [diff] [review]
0007-Bug-719888-VK_END-and-mouse-clicks-update-the-autoco.patch

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

you can find some reasoning about VK_END in bug 1114013
fwiw, I'd prefer to handle END in a separate bug. I still think we must improve backspace detection regardless of other fixes here.
If we fix bug 1148959, then we can probably handle END along with the other keys.
Created attachment 8608443 [details] [diff] [review]
0006-Bug-719888-VK_END-and-mouse-clicks-update-the-autoco.patch

Thanks for the pointer to the discussion.

(In reply to Marco Bonardo [::mak] from comment #20)
> fwiw, I'd prefer to handle END in a separate bug. I still think we must
> improve backspace detection regardless of other fixes here.
> If we fix bug 1148959, then we can probably handle END along with the other
> keys.

I believe the problem we are facing here is simply that mSearchString isn't updated in some cases where it should be, and this is the root of the "backspace problem", as the code detecting a backspace is checking against mSearchString.  In other words, the only reason END has this problem (ie, it can't be reproduces by pressing VK_RIGHT) is that the key navigation always updates the search string.  Ditto with the mouse-click - the search string isn't updated whereas positioning the caret in the exact same place with the keyboard does update it.

Given that we don't need all the magic keyboard handling on VK_END, this patch is limited to autocomplete.xml and handles both clicks and VK_END to update the search string. This way the popup remains open etc, but backspace works correctly. I agree the "bigger questions" re VK_END are best addressed in bug 1148959
Attachment #8606864 - Attachment is obsolete: true
Attachment #8608443 - Flags: feedback?(mak77)
Comment on attachment 8608443 [details] [diff] [review]
0006-Bug-719888-VK_END-and-mouse-clicks-update-the-autoco.patch

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

The mouseup change scares me a little bit... this is still code used by various products for many scopes, it's hard to guess what might happen. I don't feel brave enough at this time.

So let's see if I can suggest an alternative, I don't know if it's going to work but maybe you might try.
We are already handling VK_BACK_SPACE for Mac in onKeyPress, we could handle it for all platforms (with the due changes), and before anything else we could update .searchString there.
That is basically a merge of my patch and yours... it's a limited change that basically only affects backspace.
Attachment #8608443 - Flags: feedback?(mak77)
(In reply to Marco Bonardo [::mak] from comment #22)

> The mouseup change scares me a little bit... this is still code used by
> various products for many scopes, it's hard to guess what might happen. I
> don't feel brave enough at this time.

As I said in comment 24, the mouse handler is for a STR that doesn't involve BS at all.
 
> So let's see if I can suggest an alternative, I don't know if it's going to
> work but maybe you might try.
> We are already handling VK_BACK_SPACE for Mac in onKeyPress, we could handle
> it for all platforms (with the due changes), and before anything else we
> could update .searchString there.
> That is basically a merge of my patch and yours... it's a limited change
> that basically only affects backspace.

As above, I'm addressing a STR that doesn't involve the BS key at all, so I'm confident that approach will not fix that. Therefore, I suspect my patch *excluding* the mouse handler fixes the same things your patch does.
Doh! s/comment 24/comment 21/ (but also comment 17 ;)
(In reply to Mark Hammond [:markh] from comment #23)
> As above, I'm addressing a STR that doesn't involve the BS key at all

Still the bug happens when at the last step you press backspace... So, I guess my suggestion might handle all cases where backspace is the last str, that is what this bug is about.
Are there steps to reproduce this bug that don't involve pressing backspace at the end?
(In reply to Marco Bonardo [::mak] from comment #25)
> Are there steps to reproduce this bug that don't involve pressing backspace
> at the end?

Yes.

The existing code just looks for chars removed at the end of string regardless of how they were removed - so yeah - use the mouse to click on the char *before* the trailing slash, then press DEL.  (And maybe try selecting multiple chars at the end of the string with the mouse and typing a replacement char?)  mBackSpaced is mis-named - it should be closer to "were chars removed from the end of the string" and has nothing to do with BS directly - it's just that BS is the "obvious" way to achieve that.
(In reply to Mark Hammond [:markh] from comment #26)
> mBackSpaced is mis-named - it should be closer to
> "were chars removed from the end of the string" 

To be clear, it means "were chars removed from the end of the string *since mSearchString was updated*" - END + BS doesn't update mSearchString. A mouse-click doesn't update mSearchString. Every other keyboard navigation key does, which is why it can't be reproduced with the arrow keys.  The problem here isn't about BS - in the "END + BS" repro, it's more about the "END" than the "BS", but it will be reproduced in any scenario when you can arrange to remove characters from the end of the string without updating mSearchString.
(In reply to Mark Hammond [:markh] from comment #26)
> use the mouse to click on
> the char *before* the trailing slash, then press DEL.

interesting, I cannot reproduce this... I cannot reproduce any problem that doesn't directly involve the backspace key at the end.
Here's another: use the mouse to select the last slash and press Ctrl+X.  As I mentioned, the only trick is finding a key stroke that hasn't already been overridden to update mSearchString.
OK, I'm sold about the fact my solution is broken!
The problem is clear, autofill appends text to the end of mSearchString, the text is selected, but since it's not what the user typed, mSearchString is not updated. Then a user action happens (being it backspace, mouseup, END or whatever) that makes the autofilled text the actual mSearchString, but the controller doesn't know.

What we are missing is basically a consistent way to tell that the user confirmed the appended part, pratically that means the final selected part is no more selected or is "differently" selected.
Handling mouseup or END is basically handling some of the possible user actions that change the pre-selection. The are other actions, for example I think it would still be broken for touch events (not sure how much we care honestly).

Unfortunately there's no simple event stating "the user unselected or changed selection and the last selection was at the end of the value", especially cause there's no good "onunselect".

So, I guess the only solution is indeed to handle all of the user actions, like mouseup. What I'm worried about is that we will end up regressing some fancy behavior of other autocomplete consumers (form fill, password fill, thunderbird, ecc).
For example Thunderbird uses in-the-middle completion that will append something like "oz[ >> mozilla.org]", but we don't want "oz >> mozilla.org" to become the searchString!
So, by just overwriting searchString we can end up setting a searchString value that shouldn't be.

Again, the fact the autocomplete is shared by too many different consumers complicates things, we need a separate autocomplete for the awesomebar ASAP :( 

Btw, if we take the suggested solution, we need to limit the mouseup event reach, first of all checking the target is the textbox, then by checking if this.selectionEnd == this.value.length (caret at the end, or selection at the end) before setting searchString. Then we also need to exclude in the middle complete results (by looking for >>) or we'd set invalid values. This will be easy but will also be another hack on top of autocomplete pile of hacks.

Or, we need to rely on the only code piece that knows about autofill values, the controller :( For this we'd need to handleKeyNavigation() END (but taking bug 1148959 into account), and also do the same for specific mouseups (again filtering events as above).
This would still leave one open problem, final values. Some autofill results have a final-value attached so that "moz[illa.org]" will actually be completed to "https://www.mozilla.org". We must pay attention to not end up using the final-value or we'd be changing the text unexpectedly while the user tries to edit it. So basically we should confirm the value (in-the-middle should replace the text) but not use final values... This is likely not trivial.

Thoughts?

Updated

3 years ago
Blocks: 1071461
Whiteboard: [inline-autocomplete:normal] → [inline-autocomplete:normal]èf

Updated

3 years ago
Whiteboard: [inline-autocomplete:normal]èf → [inline-autocomplete:normal]

Updated

3 years ago
Priority: -- → P3

Updated

2 years ago
Duplicate of this bug: 960522
Assignee: mak77 → nobody
Whiteboard: [inline-autocomplete:normal] → [inline-autocomplete:normal][fxsearch]

Updated

4 months ago
Duplicate of this bug: 1430740

Updated

4 months ago
Blocks: 1425024
You need to log in before you can comment on or make changes to this bug.