Closed Bug 340572 Opened 18 years ago Closed 18 years ago

Autocomplete textbox maxrows gets reset to 6 after first popup

Categories

(Toolkit :: Autocomplete, defect)

Product:
Toolkit
Component:
Autocomplete
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: mozilla, Assigned: mozilla)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 4 obsolete files)

as landed (updated for bitrot)
4.99 KB, patch
Details | Diff | Splinter Review 
If I set the maxrows attribute on an autocompleting textbox, I get the right amount of rows the first time, but every time after that I get 6.  I suspect that the culprit is:
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/autocomplete.xml#751
Wow, the fix for bug 179666 really sucked. It looks like it hardcoded the number of rows in the binding specifically for the location bar...
Assignee: nobody → joe
Status: NEW → ASSIGNED

        Attachment #224618 -
        Flags: first-review?(gavin.sharp)

    
    

    
  
Comment on attachment 224618 [details] [diff] [review]
Removes some magical literal number assignments from the autocomplete binding

Discussed this on IRC, we need to preserve the difference in behavior between the dropdown size when opened with the button, and the dropdown size you get when just typing.

        Attachment #224618 -
        Flags: first-review?(gavin.sharp) → first-review-

    
    

    
  


  

        Attachment #224618 -
        Attachment is obsolete: true

        Attachment #224633 -
        Flags: first-review?(gavin.sharp)

    
    

    
  
Comment on attachment 224633 [details] [diff] [review]
Restores ability to have a different maxRows when triggering popup via dropmarker, moves some literals into fields.

>Index: toolkit/content/widgets/autocomplete.xml

>+      <!-- This is the maximum number of drop-down rows we get when we
>+           hit the drop marker beside fields that have it (like the URLbar).-->
>+      <field name="maxDropMarkerRows"><![CDATA[
>+        maxDropMarkerRows = 14;
>+      ]]></field>

This should just be: <field name="maxDropMarkerRows" readonly="true">14</field>. The fact that the value of the assignment statement is the value of what's being assigned is what made this work.

>       <method name="closePopup">
>         <body><![CDATA[
>           this.popup.closePopup();
>+          this.maxRows = this._normalMaxRows;

Shouldn't this be in hidepopup where it was originally being reset? What happens when you close the popup by putting focus elsewhere? I would think that this only catches the cases where the popup is closed programmatically.

>+      <!-- This is the default number of rows that we give the autocomplete
>+           popup when the textbox doesn't have a "maxrows" attribute
>+           for us to use. -->
>+      <field name="defaultMaxRows"><![CDATA[
>+        defaultMaxRows = 6;
>+      ]]></field>

Same comment as above, make it readonly and remove the CDATA and variable assignment.

        Attachment #224633 -
        Flags: first-review?(gavin.sharp) → first-review-

    
    

    
  


  

        Attachment #224633 -
        Attachment is obsolete: true

        Attachment #224638 -
        Flags: first-review?(gavin.sharp)

    
    

    
  
Comment on attachment 224638 [details] [diff] [review]
Removed unnecessary init code from fields; handled case when drop-down was closed by focus loss

>Index: toolkit/content/widgets/autocomplete.xml

>+      <!-- This is the maximum number of drop-down rows we get when we
>+           hit the drop marker beside fields that have it (like the URLbar).-->
>+      <field name="maxDropMarkerRows">14</field>

make this readonly="true"

>+          this._normalMaxRows = this.maxRows;
>+          this.maxRows = this.maxDropMarkerRows;
>           // Eusure this having focus.

fix this comment while you're here?

>-      
>+     

If you're going to fiddle with trailing whitespace, just remove it entirely :)

>+      <field name="defaultMaxRows">6</field>

readonly

>-      <handler event="popupshowing">
>+      <handler event="popupshowing"><![CDATA[
>+        if (this.mInput && this.mInput._normalMaxRows < 0)
>+          this.mInput._normalMaxRows = this.mInput.maxRows;
>         this.mPopupOpen = true;
>-      </handler>
>+      ]]></handler>
> 
>       <handler event="popuphiding">
>         setTimeout(this.clearOpenProperty, 0, this);
>-        this.mInput.maxRows = 6;
>+        if (this.mInput) {
>+          this.mInput.maxRows = this.mInput._normalMaxRows;
>+          this.mInput._normalMaxRows = -1;
>+        }
>       </handler>

I don't think the null checks in these two methods are necessary, since the popup should never show if mInput is null. The old handler didn't null check, so you shouldn't need to either.

        Attachment #224638 -
        Flags: first-review?(gavin.sharp) → first-review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Addresses comments from #7 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #224638 -
        Attachment is obsolete: true

        Attachment #225049 -
        Flags: approval-branch-1.8.1?(mconnor)

        Attachment #225049 -
        Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+

    
    

    
  


  

        Attachment #225049 -
        Attachment is obsolete: true

    
    

    
  
Landed on branch and trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: fixed1.8.1
Keywords: fixed1.8.1
Whiteboard: fixed1.8.1

          You need to log in
          before you can comment on or make changes to this bug.