212220 - Form Autocomplete not working in Firebird 0705 onwards

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Goi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030702 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030705 Mozilla Firebird/0.6

Form Autocomplete isn't working at allsince Firebird 0705 builds onwards. Its as
if it has been disabled, and there's no drop down menu while filling in forms
whatsoever. I've tried it on the 0705, 0706 and 0709 builds, and they all do not
work. Reverting back to 0702 will make it work.

Reproducible: Always

Steps to Reproduce:
1. Type in a form field that has been visited before

Actual Results:  
No drop down menu indicating past choices

Expected Results:  
Autocomplete Drop down menu should appear

Comment 1

[noririty]

confirming.

Comment 2

[[e-mail]]

Attachment: Fix some assertions

On the offchance that it's relevant.

Comment 3

[Jan Varga [:janv]]

Comment on attachment 127444 [details] [diff] [review]
Fix some assertions

I think this needs work.

clearResults() should probably call rowCountChanged with negative oldCount

if we can't figure out at what index we're adding new results in addResults
then we need to just call rowCountChanged(0, -rowCountBefore) and then
rowCountChanged(0, rowCountAfter). all wrapped in a batch

I didn't check the code around this diff, so I might miss something

Comment 4

[[e-mail]]

Attachment: Better fix

I had to move the clearResults so that I could batch it.

Comment 5

[Jan Varga [:janv]]

Comment on attachment 127459 [details] [diff] [review]
Better fix

looks much better

Comment 6

[jag (Peter Annema)]

Comment on attachment 127459 [details] [diff] [review]
Better fix

Hmm, in clearResults, could you avoid having to use |oldCount| if you moved the
rowCountChanged call up before you changed |this.mRowCount|? sr=jag either way.

Comment 7

[Erik]

.

Comment 8

[Chu Alan "sprite"]

I am not familiar with Mozilla or Firebird, but I seem to get autocomplete
working again by changing getter of maxRows in
toolkit/content/widgets/autocomplete.xml (which causes some errors to be
outputed to console)

Comment 9

[Chu Alan "sprite"]

Attachment: a little patch which seem to fix the problem

i am not sure if i am doing the correct thing in this patch, but it seems to
make autocomplete works again in my build.

Comment 10

[Brian Ryner (not reading)]

Attachment: suggested fix

The problem here is that some code was added that does .getAttribute('maxRows')
on the autocomplete input, but that autocomplete input isn't necessarily a
content node.  In this case, it's an nsFormFillController.  This patch adds a
maxRows property on nsIAutoCompleteInput and uses that instead.

Comment 11

[Chu Alan "sprite"]

brian, 
after applying your patch, i have got this error: 

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "too much recursion" {file:
"chrome://global/content/widgets/autocomplete.xml" line: 0}]' when calling
method: [nsIAutoCompleteInput::popupOpen]"  nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

Is it because I have incorrectly applied the patch?

Comment 12

[Brian Ryner (not reading)]

Hm, I'm not seeing anything like that.

Comment 13

[Brian Ryner (not reading)]

Oops! i see the problem.

Comment 14

[Brian Ryner (not reading)]

Attachment: suggested fix

Fixed a bug that would cause infinite recursion.

Comment 15

[Chu Alan "sprite"]

brian, 
another trivial thing, sometimes there is an error like this: 
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this.mInput has no properties" {file:
"chrome://global/content/widgets/autocomplete.xml" line: 0}]' when calling
method: [nsIAutoCompletePopup::invalidate]"  nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

I think 
onget="var t = this.mInput.maxRows; return t ? t : 6;"/>
should check against this.mInput before accessing maxRows to avoid this warning.

(may be something like this from my previous patch?
onget="return (this.mInput &amp;&amp; this.mInput.maxRows) ? this.mInput.maxRows
: 6"/>)

Comment 16

[Chu Alan "sprite"]

steps to reproduce the warning: 
1. fireup Firebird
2. type something into location bar

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 127600 [details] [diff] [review]
suggested fix

>@@ -505,7 +506,7 @@
>       </property>
> 
>       <property name="maxRows" readonly="true"
>-                onget="var t = parseInt(this.mInput.getAttribute('maxrows')); return t ? t : 6;"/>
>+                onget="var t = this.mInput.maxRows; return t ? t : 6;"/>

That return should use JS's righteous (after Perl) || operator:

    return t || 6;

returns the value of t if it converts to boolean true, else 6.	It does not
convert its result to boolean.

Any other places to fix likewise?

/be

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 127606 [details] [diff] [review]
suggested fix

Of course, you should get rid of the single-use temporary var t when you switch
to use ||.

Note that || and && in JS follow C and short-circuit evaluation, but they
preserve the deciding operand's type and value, after Perl.

/be

Comment 19

[Chu Alan "sprite"]

brendan, 
is checking the existance of this.mInput necessary? it seems that in some rare
occasions there will have an JavaScript error saying maxRows is not defined...

just want to know, the code will be 
    return (this.mInput && this.mInput.maxRows) || 6
if i want to check for this.mInput first?

Comment 20

[Brian Ryner (not reading)]

Attachment: one more time

incorporate suggestions from brendan and chu.  Note that the "return (blah) ||
0" is to ensure that we return 0 instead of NaN.

Comment 21

[Brian Ryner (not reading)]

Attachment: oops

Ok, I suck for attaching that last patch without testing it.  && can't be used
unescaped in an attribute value.  Rather than using && and making it
ugly, I just used a <getter>.  This one works, I swear.

Comment 22

[Joe Hewitt (gone)]

Comment on attachment 127639 [details] [diff] [review]
oops

r=hewitt

Comment 23

[Brian Ryner (not reading)]

checked in.

Comment 24

[Chu Alan "sprite"]

brian, 
i have cvs updated toolkit/, and it seems to work correctly (no JavaScript
warning :))

however, there is an assertion at nsTreeBodyFrame.cpp, which attachment 127459 [details] [diff] [review]
seems to be fixing, should that patch goes in too?

the assertion: 
###!!! ASSERTION: row count changed unexpectedly: 'mRowCount == rowCount', file
nsTreeBodyFrame.cpp, line 2198
Break: at file nsTreeBodyFrame.cpp, line 2198

Comment 25

[Simon Paquet [:sipaq]]

verified with 20030714 windows build.