491520 - Tag autocomplete prevents changing the case of tags, when adding tags

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Tracy Walker [:tracy]]

Fix for bug 490200 caused this regression
This fix makes it impossible to add new tags beginning with upper case letters
if there is any tag that already begins with that letter.

For example:  Have a tag of name "test" Then attempt to add a tag "Tuba". You
can't because auto-compete is turning the uppercase T to lower case.  

seen with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US;
rv:1.9.1b5pre) Gecko/20090505 Shiretoko/3.5b5pre

Comment 1

[Nochum Sossonko [:Natch]]

You can do it if you type in the tag name, then go back and switch the first letter to uppercase. Like I observed in the original bug, the tagging service doesn't really have a notion of lower/uppercase, it can do both but not at the same time. Eg: you cannot have "auto" and "Auto" as two separate tag names.

Another side-effect of that bug, type the first letter of a tag you already have and then hit backspace, the autocomplete erases and then comes right back, you have to hit backspace again to erase it.

Comment 2

[Tracy Walker [:tracy]]

ok, good work-around, but certainly not an acceptable regression.

O noticed that double action backspace too. is that filed?

Comment 3

[Nochum Sossonko [:Natch]]

> O noticed that double action backspace too. is that filed?

Just filed it as bug 491523.

Comment 4

[Dietrich Ayala (:dietrich)]

This will not block the release - it doesn't prevent the core functionality, and has a workaround.

Comment 5

[Nochum Sossonko [:Natch]]

Dietrich: I think you misunderstood this bug, as your change in title suggests. You cannot create _any_ tag with the same first letter as another tag you currently have if you choose a different case for it. Unless you use the workround.

re-noming.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

A deserved renomination, Natch! Dietrich and I did, indeed, misunderstand. Blocking.

Comment 7

[Nochum Sossonko [:Natch]]

Attachment: bandaid patch.

Thought I'd throw this up, this is the obvious fix for this bug and bug 491523, although I'm not sure this is wanted...

Comment 8

[Nochum Sossonko [:Natch]]

Attachment: better fix

Leaving it up to you which one you want to take, but this one leaves the functionality.

Caveat:

If the tag has a case mismatch, the tag will be changed to the new case of the typed in tag. Tags in the autocomplete show as the old tags, that can be easily changed, although I figured it should be what it was before, even if it's going to change.

Comment 9

[Nochum Sossonko [:Natch]]

Other than changing the tagging service to throw out changed values for tags when the change is only a case change, I don't know how else to fix the caveats.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: autocomplete patch

We don't want to discard completeDefaultIndex functionality.
Your second patch is effectively better, but causes more unwanted casing changes.

We have 2 possibilities here:
1. this patch changes how completeDefaultIndex acts, practically it will retain user casing unless user chooses explicitely to autocomplete, in such a case it will get result casing. This would solve all our issues, but i'm unsure if the change is wanted for the widget (or we could try to make this configurable)
2. stop applying user casing changes in Tags field, if the user wants to change casing he will have to do that explicitely in the Library, all other fields will autocomplete case insensitive but won't update tag casing.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

in both cases, taking blocker.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

i've tried to add some comment to clarify what the code is doing.

basicly, while the user is typing in an autocomplete field that has a result with a different casing from what he is typing, we don't want to force his input.
So if he has a result like "Test", and he wants to put in "tuna", we should not change the first t -> T since he did not request that (we made that trying to autocomplete to Test).
Instead if the user explicitly ask to use the autocomplete result (by popup selection or VK_RIGHT) we should revert to the result we have. This is more coherent and better visually associated imo, the user wants exactly that result, if not he would type a new result by himself.

Modified test_autocomplete3.xul to take this in count, so this has some sort of test coverage.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

a better example from Mardak on IRC, if i have tag CA, and i want to add tag cats, now i would end up with CAts in the autocomplete widget, with no way to fix it.

Comment 14

[Nochum Sossonko [:Natch]]

VK_ENTER (or clicking beyond the result) selects the autocomplete result as well, from my initial look at the patch it seems like that shouldn't be an issue, just felt like mentioning it.

In general I find the interaction of the tags field a bit strange, VK_ENTER doesn't tag the bookmark, only a blur does...

Comment 15

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

Comment on attachment 376050 [details] [diff] [review]
patch v1.1

>diff --git a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp

>+        PRUint32 count = mSearches.Count();
>+        for (PRUint32 i = 0; i < count; ++i) {
>+          nsIAutoCompleteResult *result = mResults[i];

you're using mSearches for the count, but accessing mResults? is that just a typo?

Seems like ideally this code would just call CompleteDefaultIndex, with a parameter to indicate that it shouldn't do the case check. Or maybe it would be better to factor this code and the inner part of CompleteDefaultIndex into a helper function.

>diff --git a/toolkit/content/tests/chrome/test_autocomplete3.xul b/toolkit/content/tests/chrome/test_autocomplete3.xul

>-    this._param = "result";
>+    this._param = "Result";

>-  { completeDefaultIndex: "true", key: "t", result: "ret >> result",
>+  { completeDefaultIndex: "true", key: "t", result: "ret >> Result",

I'm having a hard time understanding this test. How does the input end up containing the " >> " string? AFAICT the nsIAutocompleteResult only returns it's _param, and the keys being entered won't cause those to appear...

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

> >diff --git a/toolkit/content/tests/chrome/test_autocomplete3.xul b/toolkit/content/tests/chrome/test_autocomplete3.xul
> 
> >-    this._param = "result";
> >+    this._param = "Result";
> 
> >-  { completeDefaultIndex: "true", key: "t", result: "ret >> result",
> >+  { completeDefaultIndex: "true", key: "t", result: "ret >> Result",
> 
> I'm having a hard time understanding this test. How does the input end up
> containing the " >> " string? AFAICT the nsIAutocompleteResult only returns
> it's _param, and the keys being entered won't cause those to appear...

it's a feature of the autocomplete code, for Result, if you write ret, the suggested result is ret[ >> Result], this is how autocomplete in the middle works.

Comment 17

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

Where is the code that implements that functionality?

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#1410

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

I'm not sure i've interpreted correctly your requirement for an helper, so feel free to nag me.

Comment 21

[Martin Packer]

Sorry for duplicate bug 492757. What I countenance is storing the string in the case the user types it and reverting to this stored string (with the case preserved) once they deviate from the presumed matched tag. In my example SHARE was pre-existing and sharp was wanted so store shar in a temp buffer and when the p is typed use shar+p to give the correct result.


Unless anyone thinks it should work differently.

Comment 22

[Asaf Romano (gone)]

Comment on attachment 376709 [details] [diff] [review]
patch v1.2

r=mano

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/477546db4a94

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2704ea8db562

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 on all platforms with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090522 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090522044436

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre (.NET CLR 3.5.30729) ID:20090522045120

Comment 26

[Simon Bünzli]

This seems to have broken browser.urlbar.autoFill (bug 494410).

Comment 27

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv