561116 - [SeaMonkey] mochitests-5: 8 "test_form_autocomplete.html | Checking length of expected menu - got 4, expected n"

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Serge Gautherie (:sgautherie)]

This was already failing when SeaMonkey (re?)started to run mochitest-plain:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1267275418.1267288402.7110.gz
OS X 10.5 comm-central-trunk debug test mochitests on 2010/02/27 04:56:58
{
148077 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu - got 4, expected 3
148082 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu - got 4, expected 2
148086 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu - got 4, expected 1
148089 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu - got 4, expected 0
148097 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu - got 4, expected 3
148102 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu - got 4, expected 2
148106 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu - got 4, expected 1
148109 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete.html | Checking length of expected menu - got 4, expected 0
}

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100421
SeaMonkey/2.1a1pre] (comm-central-trunk-win32/1271913096)

Reproducible.

Comment 1

[Robert Kaiser]

The failures happen with form7 / field5.

If I comment out everything from "Look at form 7" (in |case 256:|) to |case 309|, the test succeeds.

The failing tests use different |input.maxLength| values and expect us not to offer entries for completion that are too long for that maxLength. This works in Firefox and fails in SeaMonkey.

Neil, any idea what difference the apps may have so that this is the case?

Comment 2

[[e-mail]]

There's an optimisation in Toolkit code that means that hitting Esc, Down simply reopens the existing popup. I don't know how setting the maxLength is supposed to bypass that optimisation.

Comment 3

[Robert Kaiser]

(In reply to comment #2)
> There's an optimisation in Toolkit code that means that hitting Esc, Down
> simply reopens the existing popup. I don't know how setting the maxLength is
> supposed to bypass that optimisation.

Strange - why does Firefox pass the test, then?

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> If I comment out everything from "Look at form 7" (in |case 256:|) to |case
> 309|, the test succeeds.

These cases were added by bug 444728.

Comment 5

[[e-mail]]

So presumably we're passing the unit test, just not the mochitest?

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> If I comment out everything from "Look at form 7" (in |case 256:|) to |case
> 309|, the test succeeds.

Ftr, the failing checks are in cases: 302-305 and 307-310.


(In reply to comment #2)
> There's an optimisation in Toolkit code that means that hitting Esc, Down
> simply reopens the existing popup. I don't know how setting the maxLength is
> supposed to bypass that optimisation.

If you're referring to 
200 if (aPreviousResult && aPreviousResult.searchString.trim().length > 1 &&
these tests don't trigger it because the search is for '' then for '1'.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)

This issue is related to |input.maxLength = n;|.

> Ftr, the failing checks are in cases: 302-305 and 307-310.

The formers use 10, the latters use 4. (whereas it should be less.)

Comment 8

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #2)
> > There's an optimisation in Toolkit code that means that hitting Esc, Down
> > simply reopens the existing popup. I don't know how setting the maxLength is
> > supposed to bypass that optimisation.
> If you're referring to 
> 200 if (aPreviousResult && aPreviousResult.searchString.trim().length > 1 &&
> these tests don't trigger it because the search is for '' then for '1'.
No, I mean in Toolkit code, not in the test:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#464
>// Open the popup if there has been a previous search, or else kick off a new search
>if (mResults.Count() > 0) {
>  if (mRowCount) {
>    OpenPopup();
>  }
>} else
>  StartSearchTimer();

Comment 9

[Robert Kaiser]

(In reply to comment #7)
> (In reply to comment #6)
> This issue is related to |input.maxLength = n;|.

Yes, I said that in comment #1 ;-)

> > Ftr, the failing checks are in cases: 302-305 and 307-310.

Hmm, you're saying 306 is succeeding? Ah, it's actually what 305 sets that gets tested in 306, so that's logical, as that one sets maxLength=4 and expects all 4 entries to display, which we always do.
 
> The formers use 10, the latters use 4. (whereas it should be less.)

so what you're saying is that you did let it check the actual value of maxLength and it doesn't get set correctly if it's smaller than the longest entry in the dropdown?

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)

> > 200 if (aPreviousResult && aPreviousResult.searchString.trim().length > 1 &&

This is in the component: nsFormAutoComplete.js...

> No, I mean in Toolkit code, not in the test:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#464

I had a doubt. I leave that one to you ;->


(In reply to comment #9)
> Yes, I said that in comment #1 ;-)

Well, your comment 1 sounded like "SeaMonkey results ignore maxlength" to me.
What I was trying to add is that actually "SeaMonkey does not change maxlength in the first place".

> Hmm, you're saying 306 is succeeding?

Yes, I'm saying that for an yet-unknown reason there is one maxlength change that actually works.

> so what you're saying is that you did let it check the actual value of
> maxLength and it doesn't get set correctly if it's smaller than the longest
> entry in the dropdown?

Yes (I added a dump() where maxlength is checked), but it's unrelated to the dropdown "width": higher values are ignored just the same.
I tried to see if it was a timing/order issue but got mixed results, so I gave up on finding something I could explain for sure.

(Now, I'm calling for helpwanted...)

Comment 11

[[e-mail]]

(In reply to comment #10)
> (In reply to comment #8)
> > > 200 if (aPreviousResult && aPreviousResult.searchString.trim().length > 1 &&
> This is in the component: nsFormAutoComplete.js...
Ah, well, we don't actually reach that code during the failing tests, since that requires us to call StartSearch(Timer), which we don't.

> (In reply to comment #9)
> > Hmm, you're saying 306 is succeeding?
> Yes, I'm saying that for an yet-unknown reason there is one maxlength change
> that actually works.
Well, none of the changes work. But there are only 4 values, so a maxlength of 4 or more will appear to work.

Comment 12

[Serge Gautherie (:sgautherie)]

Arf, thanks for your comments,
let me reset what I wrote: (maybe nothing new, but confirming at l(e)ast)

1) |input.maxLength = n;|
Forget what I wrote: SeaMonkey maxLength setting does work :->

2) "FormAutoComplete: Creating new autocomplete search result."
(All of the previous cases produce the same log in FF 3.7 and SM 2.1.)
2a)
I confirm that SeaMonkey 2.1 creates a new result in 256(+300) and 305(+306) only.
2b)
This is the difference from Firefox 3.7, which creates a new result for each of these (form7) cases.

Fwiw, I wonder if that would be somehow related to
http://mxr.mozilla.org/mozilla-central/find?string=%2Fautocomplete%5C.xml%24

(I don't think it's worth that I try to figure this out further by myself: actual helpwanted.)

Comment 13

[Robert Kaiser]

(In reply to comment #12)
> 1) |input.maxLength = n;|
> Forget what I wrote: SeaMonkey maxLength setting does work :->

Good to know but comment #7 does confuse me, then, as you implied there that we do actually change maxLength at least once, and I deducted that it was at the one case where we set it to 4 and not something smaller, and 4 is the legth of the longest string in the popup.
I may just have deducted the wrong things there, though.

> This is the difference from Firefox 3.7, which creates a new result for each of
> these (form7) cases.

This is definitely interesting.

> Fwiw, I wonder if that would be somehow related to
> http://mxr.mozilla.org/mozilla-central/find?string=%2Fautocomplete%5C.xml%24

That's one reason why I dragged in Neil, as he should know about those things. :)

Comment 14

[[e-mail]]

Attachment: Copy toolkit attributes

This makes it so that the XUL popup manager ignores the Escape keypress (which silently closes the popup), so that the form fill controller can do its cleanup (which includes resetting the search).

The change doesn't appear to have any effect on XPFE autocomplete. (Tested with Open Location, URLbar and Message Compose.)

Comment 15

[Robert Kaiser]

Comment on attachment 441284 [details] [diff] [review]
Copy toolkit attributes

I'm not too good at what those attibutes actually do, but I can confirm that 1) they're the same as in toolkit's version and 2) the patch fixes that test.
If that's all you wanted for review, so be it!

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)

> comment #7 does confuse me, [...]
> I may just have deducted the wrong things there, though.

I understood my comment 7 well (enough).
But, as I wrote in comment 12, my testing was wrong:
the 10 and 4 values were actually those for 256(+300) and 305(+306);
I was just missing those for 302-305 and 307-310 the way I tested at that time :-/
Which also explains why, in comment 10, I was kind of puzzled by my own testing results :-<

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 441284 [details] [diff] [review]
Copy toolkit attributes

>-    <content>
>+    <content ignorekeys="true" level="top">

Firefox got level="top" from bug 454531.
It looks like we should clean up
http://mxr.mozilla.org/comm-central/search?string=level%3D%22top%22&case=1&find=%5C.xul%24
{
/mail/base/content/specialTabs.xul
    * line 44 -- <panel type="autocomplete" id="PopupAutoComplete" level="top"
/suite/browser/navigator.xul
    * line 149 -- <panel type="autocomplete" id="PopupAutoComplete" level="top" noautofocus="true"/>
}
while we're here.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 441284 [details] [diff] [review]
Copy toolkit attributes

>-    <content>
>+    <content ignorekeys="true" level="top">

Firefox got ignorekeys="true" from
http://hg.mozilla.org/mozilla-central/rev/1b867255b2ff

After a quick check, it looks like the other changes are unrelated,
but mentioning it, just in case.

Comment 19

[Robert Kaiser]

(In reply to comment #17)
> It looks like we should clean up
> [...]
> while we're here.

Could we please leave this to fixing the test and file a followup for this?

Thanks a lot for digging up the reasons and this possible cleanup, BTW!

Comment 20

[[e-mail]]

Pushed changeset ae4c0cac01ea to mozilla-central.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> Could we please leave this to fixing the test and file a followup for this?

I filed bug 561645.

***

V.Fixed, per tinderboxes.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c0376d573ac4
Autocomplete controller expects to be able to process ESC key r=KaiRo