494169 - mochitest-chrome: .../toolkit/content/tests/chrome/test_autocomplete4.xul | Test '... key': autocomplete.value should equal 'Result' - got "result", expected "Result"

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

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

Comment 1

[[e-mail]]

Attachment: Possible patch

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

Comment 2

[Frank Wein [:mcsmurf]]

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

Comment 3

[Serge Gautherie (:sgautherie)]

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

Comment 4

[[e-mail]]

Attachment: Updated patch

* 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 5

[Frank Wein [:mcsmurf]]

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.

Comment 6

[[e-mail]]

(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.

Comment 7

[Serge Gautherie (:sgautherie)]

Ping for review(s)?

Comment 8

[Frank Wein [:mcsmurf]]

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

Comment 9

[[e-mail]]

Pushed changeset e024c43987fa to mozilla-central.

Pushed changeset e024c43987fa to releases/mozilla-1.9.1

Comment 10

[Serge Gautherie (:sgautherie)]

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

c4eb672645c4!

Comment 11

[Serge Gautherie (:sgautherie)]

[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 12

[Mark Banner (:standard8)]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c4cd5cc4d356
Tweak xpfe autocomplete to track toolkit change and pass tests from bug 494410 r=mcsmurf