Closed Bug 905190 Opened 7 years ago Closed 7 years ago

The XPFE autocomplete popup alternately shows and hides when text is typed in the textbox

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 911090

People

(Reporter: philip.chee, Unassigned)

Details

Attachments

(2 files)

STR: Install Console2 http://console2.mozdev.org/#C_latest_dev
The eval textbox has the following XUL markup:
 autocompletesearch="form-history"
 autocompletesearchparam="Console2#TextboxEval"
Put something in the form-history e.g.
Components.classes["@mozilla.org/dom/storagemanager;1"].getService(Components.interfaces.nsIDOMStorageManager);

Then start typing "components.classes". After the third character the popup shows/hides for each subsequent character typed.
Comment on attachment 790246 [details] [diff] [review]
Autocomplete for the Find Dialog.

If I bind the tookit autocomplete to this textbox the blink effect is not present. Tested in safe-mode. I notice this bug exists in SeaMonkey 2.0 so not a recent regression.
If I try this I get:

TypeError: setting a property that has only a getter
nsFormAutoComplete.js Line: 184

Perhaps the toolkit autocomplete ignores this error and leaves the popup open; XPFE autocomplete responds by closing the popup.
The old C++ form manager utilised the autocomplete result cache by deleting the elements that no longer matched the new search string. However it did not update the result's search string itself. Since toolkit's autocomplete does not read the resulting search string this did not affect it. XPFE's autocomplete.xml does however, and would have failed the search because the string does not match the string that it wanted results for.

Bug 469443 rewrote the form manager in JS, porting the searchString behaviour.

Bug 497541 changed toolkit autocomplete to allow the text to be changed in the middle. This incidentally meant that it could now send previous results for an old search string which was not a prefix of the new search string. The code happens to have been tweaked to update the search string in the results.

Bug 556007 then changed DOM autocomplete to include datalist values. It does this by creating its own result object which replaces the one created by the form manager. The result object goes to the trouble of copying the entries from the form manager's result object. This means that when the form manager tries to use its cache it does actually find its previous entries, even though it's looking at an input list autocomplete results object.

However the flaw in the ointment (well, this munging of results is pretty gross, really) is that the form manager expects to be able to write to the result object's search string, which is now longer true.

Since we have a type error, we definitely need a fix. I'd rather have a fix that sees the result object gain the correct search string.
Product: SeaMonkey → Toolkit
Attached patch Fix type errorSplinter Review
Attachment #790460 - Flags: review?(dolske)
Comment on attachment 790460 [details] [diff] [review]
Fix type error

I have vague recollections of some of this stuff not wanting to have search strings changing underneath it, but I might be remembering something else. Mechanically looks ok to me, but MattN should take a closer look if he wants.
Attachment #790460 - Flags: review?(dolske) → review?(mnoorenberghe+bmo)
Comment on attachment 790460 [details] [diff] [review]
Fix type error

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

searchString is marked readOnly in the idl so how are you planning on changing the searchString value? via wrappedJSObject? If that's the case then why can't you just modify wrappedJSObject._searchString like _autoCompleteSearchShared does[1] in nsFormAutocomplete.js? The wrappedJSObject was added specifically for this purpose but it wouldn't be right for random consumer to use that.

Does this affect Firefox? If so, could you explain how. I'm having a hard time understanding what this bug is about since the first bunch of comments are talking about very different things than what this patch is doing. If it doesn't affect Firefox, could you point me to the comm-central code that is trying to write to .searchString?

[1] I filed bug 911090 because this isn't working. That may be the same bug that this is trying to solve but it was unclear.
Attachment #790460 - Flags: review?(mnoorenberghe+bmo)
(In reply to Matthew N. from comment #7)
> I filed bug 911090 because this isn't working. That may be the same bug
> that this is trying to solve but it was unclear.

Indeed, it's the same bug, but you managed to find STR that works on Firefox (SeaMonkey is stricter with its search strings which is why it hides the popup rather than displaying incorrect results).

I didn't want to change it to write to _searchString because it's not obvious that you're not actually being given your own results back; I thought it was up to the implementation of the replacement results to support the "existing API".
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 911090
You need to log in before you can comment on or make changes to this bug.