720066 - Tagging broken, cannot type in the tag field

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Adam Kowalczyk]

Typing works fine as long as there are autocomplete results from your existing tags. As soon as there are no matches, the field gets cleared. I believe it may be caused by the recent autocomplete changes.

Works: http://hg.mozilla.org/mozilla-central/rev/58e933465c36
Broken: http://hg.mozilla.org/mozilla-central/rev/5c2bc94d359c

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58e933465c36&tochange=5c2bc94d359c

Comment 1

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

taking to not lose it, but feel free to steal it.

Comment 2

[Alice0775 White]

Tags Field of Bookmark Properties dialog , Edit Bookmark panel and and Library window are also broken.

Regression window(m-c)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/eea95e86541f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120119 Firefox/12.0a1 ID:20120119024012
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/19a5e75b8ed8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120119 Firefox/12.0a1 ID:20120119033312
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=eea95e86541f&tochange=19a5e75b8ed8

Triggered by:
51dcdc85081c	Michael Ventnor — Bug 660592 - Allow autocomplete results to hide themselves from the popup. r=mak sr=gavin

Comment 4

[Abdullah Diaa]

had the same problem again after removing some addons and restarting the browser

Comment 8

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

taking to not lose it from my radar, though feel free to steal it if you have a fix.

Comment 9

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

So the patch removed a mRowCount == 0 bailout from CompleteDefaultIndex(), cause mRowCount does not take into account typeAheadResults. The problem is tags autocomplete result notifies RESULT_SUCCESS instead of RESULT_NOMATCH even if it has no results, so we actually go through CompleteDefaultIndex even if the complete value is an empty string. The previous check was protecting us from this kind of weirdness.
So I think to add back a protection from these cases (Actually adding a check that the index we try to complete is < than the number of matches), and fix the tags autocomplete implementation to return a proper message.

Comment 11

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

Attachment: patch v1.0

This would be the patch, looking into a test now, if possible.

Comment 12

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

Attachment: patch v1.1

Comes with a test now.
I was not sure if we would like to also add a fatal assertion for debug mode there, though it would make impossible to test the fix in debug mode. I may add a warning if you wish, though it will create noise in the test.

Comment 14

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

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

I was going to suggest putting this check in the loop above, but that'd be more complicated and wouldn't really make much of a difference in practice, so this is fine. This also made me find bug 720598...

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/360bd1a6e72b

Comment 16

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

follow up to fix a typo, I should not code in cpp and js at the same time, I end up confusing array types :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f35bd9404ee4

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/360bd1a6e72b
https://hg.mozilla.org/mozilla-central/rev/f35bd9404ee4

Comment 19

[Daniel]

working great in today's nightly over here
well done!