bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Form Autocomplete not working in Firebird 0705 onwards

VERIFIED FIXED

Status

()

Firefox
Address Bar
--
major
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Goi, Assigned: Joe Hewitt (gone))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

15 years ago
Created attachment 127444 [details] [diff] [review]
Fix some assertions

On the offchance that it's relevant.

Updated

15 years ago
Attachment #127444 - Flags: superreview?(jaggernaut)
Attachment #127444 - Flags: review?(varga)

Comment 3

15 years ago
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
Attachment #127444 - Flags: review?(varga) → review-

Comment 4

15 years ago
Created attachment 127459 [details] [diff] [review]
Better fix

I had to move the clearResults so that I could batch it.
Attachment #127444 - Attachment is obsolete: true

Updated

15 years ago
Attachment #127459 - Flags: superreview?(jaggernaut)
Attachment #127459 - Flags: review?(varga)

Comment 5

15 years ago
Comment on attachment 127459 [details] [diff] [review]
Better fix

looks much better
Attachment #127459 - Flags: review?(varga) → review+

Comment 6

15 years ago
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.
Attachment #127459 - Flags: superreview?(jaggernaut) → superreview+

Comment 7

15 years ago
.

Comment 8

15 years ago
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

15 years ago
Created attachment 127532 [details] [diff] [review]
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.
Created attachment 127600 [details] [diff] [review]
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.
Attachment #127600 - Flags: review?(blaker)

Comment 11

15 years ago
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?
Hm, I'm not seeing anything like that.
Oops! i see the problem.
Attachment #127600 - Flags: review?(blaker)
Created attachment 127606 [details] [diff] [review]
suggested fix

Fixed a bug that would cause infinite recursion.
Attachment #127600 - Attachment is obsolete: true
Attachment #127606 - Flags: review?(blaker)

Comment 15

15 years ago
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

15 years ago
steps to reproduce the warning: 
1. fireup Firebird
2. type something into location bar
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 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

15 years ago
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?
Attachment #127606 - Flags: review?(blaker)
Created attachment 127635 [details] [diff] [review]
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.
Attachment #127606 - Attachment is obsolete: true
Attachment #127635 - Flags: review?(blaker)
Created attachment 127639 [details] [diff] [review]
oops

Ok, I suck for attaching that last patch without testing it.  && can't be used
unescaped in an attribute value.  Rather than using &amp;&amp; and making it
ugly, I just used a <getter>.  This one works, I swear.
Attachment #127635 - Attachment is obsolete: true
(Assignee)

Comment 22

15 years ago
Comment on attachment 127639 [details] [diff] [review]
oops

r=hewitt
Attachment #127639 - Flags: review+
checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 24

15 years ago
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
verified with 20030714 windows build.
Status: RESOLVED → VERIFIED
Attachment #127635 - Flags: review?(blake)

Updated

15 years ago
Attachment #127444 - Flags: superreview?(jag)
You need to log in before you can comment on or make changes to this bug.