Closed Bug 494169 Opened 12 years ago Closed 3 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b1

People

(Reporter: neil, Assigned: neil)

References

(Blocks 1 open bug, )

Details

(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1rc2])

Attachments

(1 file, 1 obsolete file)

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
Attached patch Possible patch (obsolete) — Splinter Review
Seems to work, but more eyes would be good :-)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #378859 - Flags: review?(jag)
Attachment #378859 - Flags: review?(bugzilla)
Attachment #378859 - Flags: review?(ajschult)
btw: The test failure happened because Bug 491520 changed code and tests.
Blocks: SmTestFail
Depends on: 491520
Summary: mochitest-browser-chrome: .../toolkit/content/tests/chrome/test_autocomplete3.xul | Test complete: autocomplete.value should equal 'Result' - got "ret >> Result", expected "Result" → mochitest-chrome: .../toolkit/content/tests/chrome/test_autocomplete3.xul | Test complete: autocomplete.value should equal 'Result' - got "ret >> Result", expected "Result"
Fwiw, bug 494410 landing changed the failure report:
it's now new test_autocomplete4.xul which fails.
Depends on: 494410
Attached patch Updated patchSplinter Review
* 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.
Attachment #378859 - Attachment is obsolete: true
Attachment #379863 - Flags: review?(jag)
Attachment #379863 - Flags: review?(bugzilla)
Attachment #379863 - Flags: review?(ajschult)
Attachment #378859 - Flags: review?(jag)
Attachment #378859 - Flags: review?(bugzilla)
Attachment #378859 - Flags: review?(ajschult)
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)?
Summary: mochitest-chrome: .../toolkit/content/tests/chrome/test_autocomplete3.xul | Test complete: autocomplete.value should equal 'Result' - got "ret >> Result", expected "Result" → mochitest-chrome: .../toolkit/content/tests/chrome/test_autocomplete4.xul | Test '... key': autocomplete.value should equal 'Result' - got "result", expected "Result"
Attachment #379863 - Flags: review?(bugzilla) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
(In reply to comment #9)
> Pushed changeset e024c43987fa to releases/mozilla-1.9.1

c4eb672645c4!
Flags: in-testsuite+
Whiteboard: [fixed1.9.1rc1+]
Target Milestone: --- → seamonkey2.0b1
Whiteboard: [fixed1.9.1rc1+] → [fixed1.9.1rc2]
[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
Status: RESOLVED → VERIFIED
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.
Attachment #379863 - Flags: review?(jag)
Attachment #379863 - Flags: review?(ajschult)
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
Status: VERIFIED → RESOLVED
Closed: 11 years ago3 years ago
You need to log in before you can comment on or make changes to this bug.