Plus two subsequent related failures. The test looks to see what value gets completed after you hit the right arrow. Our autocomplete doesn't agree with toolkits as to when to complete. As yet I can't find out what toolkit's completion policy is, but currently ours is: * Don't complete when an entry is selected with (page)up/down * Don't complete if there's no match filled and we're not forcing complete * Don't complete if we're not forcing complete and we didn't use Enter * Don't complete if we've selected an entry and we didn't use Enter What I think might make sense is the following policy: * Don't complete when an entry is selected with (page)up/down Otherwise: * Always complete if we're forcing complete * Always complete is Enter was used * Always complete if completeDefaultIndex was used

Seems to work, but more eyes would be good :-)

btw: The test failure happened because Bug 491520 changed code and tests.

Fwiw, bug 494410 landing changed the failure report: it's now new test_autocomplete4.xul which fails.

* Cleared mFinishAfterSearch when typing because of a race condition * Forced left/right to complete if there was a default value filled * Made the completion behaviour apply to home/end as well as left/right This now passes the new autocomplete tests.

Comment on attachment 379863 [details] [diff] [review] Updated patch >diff --git a/xpfe/components/autocomplete/resources/content/autocomplete.xml b/xpfe/components/autocomplete/resources/content/autocomplete.xml >--- a/xpfe/components/autocomplete/resources/content/autocomplete.xml >+++ b/xpfe/components/autocomplete/resources/content/autocomplete.xml >@@ -776,19 +776,20 @@ > } else if (this.mTransientValue || >- !(this.forceComplete || this.mDefaultMatchFilled) || >- !(this.forceComplete || aForceComplete) || >- !(this.mNeedToComplete || aForceComplete)) { >+ !(this.forceComplete || >+ (aForceComplete && >+ this.mDefaultMatchFilled && >+ this.mNeedToComplete))) { So this part of the patch also changes the logic a bit? I was not sure if Comment 0 from the bug still applies about what we should do on autocomplete.

(In reply to comment #5) > (From update of attachment 379863 [details] [diff] [review]) > So this part of the patch also changes the logic a bit? I was not sure if > Comment 0 from the bug still applies about what we should do on autocomplete. Yes, test_autocomplete4.xul expects behaviour which is different yet again.

Ping for review(s)?

Comment on attachment 379863 [details] [diff] [review] Updated patch Looks good and seems to work. btw for other people looking at this, the old code for the else if part as simplified boolean expression was (excluding the this.mTransientValue part): !A && !B || !A && !C || !C && !D the new code there does this: !A && !B || !A && !C || !A && !D with A=this.forceComplete B=this.mDefaultMatchFille C=aForceComplete D=this.mNeedToComplete

Pushed changeset e024c43987fa to mozilla-central. Pushed changeset e024c43987fa to releases/mozilla-1.9.1

(In reply to comment #9) > Pushed changeset e024c43987fa to releases/mozilla-1.9.1 c4eb672645c4!

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090711 SeaMonkey/2.1a1pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/47bfcd275ede +http://hg.mozilla.org/comm-central/rev/291cbe3374b9) V.Fixed

Comment on attachment 379863 [details] [diff] [review] Updated patch This patch got reviewed and landed, the extra requests were just for more feedback. Still welcome but given that was 9 months ago, clearing requests down.

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/c4cd5cc4d356 Tweak xpfe autocomplete to track toolkit change and pass tests from bug 494410 r=mcsmurf