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

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: tracy, Assigned: mak)

Tracking

({regression, verified1.9.1})

Trunk
Firefox 3.6a1
regression, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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
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.
Flags: blocking-firefox3.5?
Blocks: 490200
(Reporter)

Comment 2

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

O noticed that double action backspace too. is that filed?
Depends on: 491523
No longer depends on: 491523
> O noticed that double action backspace too. is that filed?

Just filed it as bug 491523.
This will not block the release - it doesn't prevent the core functionality, and has a workaround.
Flags: blocking-firefox3.5? → blocking-firefox3.5-
Hardware: x86 → All
Summary: Tag autocomplete prevents creation of upper case Tags → Tag autocomplete prevents changing the case of tags, when adding tags
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.
Flags: blocking-firefox3.5- → blocking-firefox3.5?
A deserved renomination, Natch! Dietrich and I did, indeed, misunderstand. Blocking.
Flags: blocking-firefox3.5? → blocking-firefox3.5+
Created attachment 375853 [details] [diff] [review]
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...
Attachment #375853 - Flags: review?(dietrich)
Created attachment 375880 [details] [diff] [review]
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.
Attachment #375880 - Flags: review?(dietrich)
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.
(Assignee)

Comment 10

9 years ago
Created attachment 375934 [details] [diff] [review]
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.
Attachment #375880 - Attachment is obsolete: true
Attachment #375880 - Flags: review?(dietrich)
Attachment #375853 - Attachment is obsolete: true
Attachment #375853 - Flags: review?(dietrich)
(Assignee)

Comment 11

9 years ago
in both cases, taking blocker.
Status: NEW → ASSIGNED
Assignee: nobody → mak77
(Assignee)

Comment 12

9 years ago
Created attachment 376050 [details] [diff] [review]
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.
Attachment #375934 - Attachment is obsolete: true
Attachment #376050 - Flags: review?(gavin.sharp)
(Assignee)

Comment 13

9 years ago
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.
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...
Whiteboard: [has patch][needs review gavin]
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...
(Assignee)

Comment 16

9 years ago
> >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.
Where is the code that implements that functionality?
(Assignee)

Comment 19

9 years ago
Created attachment 376709 [details] [diff] [review]
patch v1.2

I'm not sure i've interpreted correctly your requirement for an helper, so feel free to nag me.
Attachment #376050 - Attachment is obsolete: true
Attachment #376709 - Flags: review?(gavin.sharp)
Attachment #376050 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Duplicate of this bug: 492757

Comment 21

9 years ago
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 on attachment 376709 [details] [diff] [review]
patch v1.2

r=mano
Attachment #376709 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review gavin] → [can land]
(Assignee)

Comment 23

9 years ago
http://hg.mozilla.org/mozilla-central/rev/477546db4a94
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → Firefox 3.6a1
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
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Keywords: fixed1.9.1 → verified1.9.1

Comment 26

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

Updated

9 years ago
Depends on: 494410
Blocks: 491523
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.