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

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Fanolian]

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.

Comment 1

[Marco Bonardo [:mak]]

Confirmed

Comment 3

[Marco Bonardo [:mak]]

IS this really a dupe? bug 719890 doesn't seem to involve selecting in the popup

Comment 4

[Justin Lebar (not reading bugmail)]

Oh, you're right, it's different!

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Marco Bonardo [:mak]]

Attachment: 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.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v2.0

This is a better alternative to how we detect backspace

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 12

[Marco Bonardo [:mak]]

(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.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

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...

Comment 14

[Marco Bonardo [:mak]]

yes, that's correct, mBackspaced is only a way to say "we are likely able to reuse something"

Comment 15

[Mark Hammond [:markh] [:mhammond]]

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)

Comment 16

[Marco Bonardo [:mak]]

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.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Attachment: 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?

Comment 18

[Matthew N. [:MattN]]

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.

Comment 19

[Marco Bonardo [:mak]]

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

Comment 20

[Marco Bonardo [:mak]]

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.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Attachment: 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

Comment 22

[Marco Bonardo [:mak]]

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.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 24

[Mark Hammond [:markh] [:mhammond]]

Doh! s/comment 24/comment 21/ (but also comment 17 ;)

Comment 25

[Marco Bonardo [:mak]]

(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?

Comment 26

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 28

[Marco Bonardo [:mak]]

(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.

Comment 29

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 30

[Marco Bonardo [:mak]]

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?

Comment 33

[kirill.rakhman]

This bug is 7 years old and I'm still hitting it on a daily basis. Are there any updates?

Comment 34

[Marco Bonardo [:mak]]

we are rewriting the code, so it's possible this may improve in v68. We'll revisit.

Comment 36

[Marco Bonardo [:mak]]

Bug 1557555 fixes this in the Quantum Bar code, we don't plan to fix it in the legacy code.