Closed Bug 312867 Opened 18 years ago Closed 17 years ago

Implement textbox.reset(), expose input's editor

Categories

(Core :: XUL, enhancement)

Product:
Core
Component:
XUL
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: WeirdAl, Assigned: martijn.martijn)

References

Details

(Keywords: fixed1.8.1)

Attachments

(4 files, 9 obsolete files)

testcase (run as chrome)
1.08 KB, application/vnd.mozilla.xul+xml
Details
patch2
4.98 KB, patch
neil
: review+
Gavin
: review+
neil
: superreview+
Details | Diff | Splinter Review
testcase
556 bytes, application/vnd.mozilla.xul+xml
Details
Unit test for reset() and defaultValue
3.71 KB, application/vnd.mozilla.xul+xml
Details 
Some applications may want access to a textbox's editor for various reasons (see
bug 303727 for the html:input and html:textarea equivalents).  In addition, with
the editor exposed, we now have a possibility of resetting the textbox to its
initial conditions.  This bug is on file to implement these features for:

XUL textbox
XUL multiline textbox
XUL autocomplete textbox
XUL editable menulist

The patches I submit will be for both xpfe and toolkit.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for xpfe (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #200099 -
        Flags: superreview?(neil.parkwaycc.co.uk)

        Attachment #200099 -
        Flags: review?(neil.parkwaycc.co.uk)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for toolkit (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #200100 -
        Flags: review?(mconnor)

    
    

    
  
Comment on attachment 200099 [details] [diff] [review]
patch for xpfe

>+      <!-- Set to 1 so we only look it up once. -->      
>+      <field name="mEditor">1</field>
I'm not convinced that this property is worth cacheing.

>+          this.value = this.getAttribute("value");
Nice optimization in the textbox case :-)

    
    

    
  
> I'm not convinced that this property is worth cacheing.

Perhaps it isn't.  But I don't know if we want to call instanceof repeatedly,
catch errors for trying to reference the property repeatedly, etc.  (We do get
an exception thrown when a remote page's textbox tries to get the editor
object.)  Venkman users would hate us for that.

Also, in the reset() method, I refer to the editor property twice.  I could
rewrite it to say:

var editor = this.editor;
if (editor) {
  editor.transactionManager.clear();
  return true;
}

Finally, we may yet find other uses for this.editor.  reset() is only the first
so far.

Your call, Neil (and mconnor).  In this case, I want the code consistent between
xpfe and toolkit.

    
    

    
  
Note to self:  if the patches get r-, I should also implement a
resetToValue(aValue) method.  

    
    

    
  
Attachment 200100 [details] [diff] is not for toolkit as stated.


    
    

    
  
Comment on attachment 200100 [details] [diff] [review]
patch for toolkit

Whoops.  Uploaded the wrong file.

        Attachment #200100 -
        Attachment is obsolete: true

        Attachment #200100 -
        Flags: review?(mconnor)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for toolkit (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #200195 -
        Flags: review?(mconnor)

    
    

    
  
Comment on attachment 200099 [details] [diff] [review]
patch for xpfe

>+            var iface = Components.interfaces.nsIDOMNSEditableElement;
Not too keen on your naming here...

>+            try { 
>+              this.mEditor = (this.inputField instanceof iface) ?
>+                             this.inputField.editor :
>+                             null;
>+            }
I don't think this should be in a try/catch. Content should just know not to call this method. This also means you can use QueryInterface directly instead of instanceof.

>+      <method name="reset">
>+        <body><![CDATA[
>+          this.value = this._initialValue;
(In reply to comment #6)
>Note to self:  if the patches get r-, I should also implement a
>resetToValue(aValue) method.  
Actually you should just expose and use the defaultValue property.

>+          if (this.editor) {
>+            this.editor.transactionManager.clear();
>+            return true;
>+          }
You can now wrap this in a try/catch (in which case you don't even need the initial if).

        Attachment #200099 -
        Flags: superreview?(neil.parkwaycc.co.uk)

        Attachment #200099 -
        Flags: superreview-

        Attachment #200099 -
        Flags: review?(neil.parkwaycc.co.uk)

        Attachment #200099 -
        Flags: review-

    
    

    
  
Comment on attachment 200195 [details] [diff] [review]
patch for toolkit

Toolkit would be crazy to accept a patch whose equivalent has been r-'d for xpfe.

        Attachment #200195 -
        Flags: review?(mconnor)

    
  

        Attachment #200195 -
        Attachment is obsolete: true

    
    

    
  
(In reply to comment #11)
>(From update of attachment 200195 [details] [diff] [review] [edit])
>Toolkit would be crazy to accept a patch whose equivalent has been r-'d for xpfe.
Actually I'm sure I remember seeing one, something to do with radio access keys.

    
    

    
  
I'm going to split this into two separate patches.  One for exposing input's editor (there's now code in place that could be optimized using it), and one for textbox.reset() (which is an enhancement req.)
Status: NEW → ASSIGNED

    
    

    
  


  

        Attachment #200099 -
        Attachment is obsolete: true

        Attachment #206647 -
        Flags: superreview?(neil.parkwaycc.co.uk)

        Attachment #206647 -
        Flags: review?(neil.parkwaycc.co.uk)

    
    

    
  


  

        Attachment #206648 -
        Flags: review?(mconnor)

    
    

    
  
Comment on attachment 206647 [details] [diff] [review]
patch for xpfe (implement editor)

Nit: please put editor just before select in menulist.xml like it is in textbox.xml

        Attachment #206647 -
        Flags: superreview?(neil.parkwaycc.co.uk)

        Attachment #206647 -
        Flags: superreview+

        Attachment #206647 -
        Flags: review?(neil.parkwaycc.co.uk)

        Attachment #206647 -
        Flags: review+

    
    

    
  
Neil is a toolkit peer, no need to split the patches when they're identical like this one! :)

    
    

    
  
gavin: if Neil wants to give r+ to the toolkit patch, that's his call.  :)

    
    

    
  
Comment on attachment 206647 [details] [diff] [review]
patch for xpfe (implement editor)

xpfe patch for editor checked in by timeless.

        Attachment #206647 -
        Attachment is obsolete: true

    
  
Blocks: 324354

        Attachment #206648 -
        Flags: review?(mconnor) → review+

    
    

    
  
I'll need someone to check in the patch, please.

    
    

    
  
Checked in the toolkit version and fixed comment 16's nit.
mozilla/toolkit/content/widgets/menulist.xml;                1.20;
mozilla/toolkit/content/widgets/textbox.xml;                 1.27;
mozilla/xpfe/global/resources/content/bindings/menulist.xml; 1.37;

        Attachment #206648 -
        Attachment description: patch for toolkit (expose editor) → patch for toolkit (expose editor) [checked in]

        Attachment #206648 -
        Attachment is obsolete: true
OS: Windows XP → All
Hardware: PC → All

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1.8 branch patch (toolkit) (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #210365 -
        Flags: branch-1.8.1?(mconnor)

        Attachment #210365 -
        Attachment description: 1.8 branch patch (toolkit → 1.8 branch patch (toolkit)

    
    

    
  
oh, I forgot about the FF2 series.

gavin, do you want to patch xpfe (SeaMonkey) as well?

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1.8 branch patch (xpfe) (obsolete)
        — 
          — Splinter Review
      

    


  
Not sure what the SM branch plans are, but here's the patch.

        Attachment #210368 -
        Flags: branch-1.8.1?(neil)

    
    

    
  
Comment on attachment 210365 [details] [diff] [review]
1.8 branch patch (toolkit)

Landed on the 1.8 branch.
mozilla/toolkit/content/widgets/menulist.xml; new revision: 1.19.8.1;
mozilla/toolkit/content/widgets/textbox.xml; new revision: 1.21.4.1;

        Attachment #210365 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 210368 [details] [diff] [review]
1.8 branch patch (xpfe)

This is the combination of the previous patches, right?

        Attachment #210368 -
        Flags: branch-1.8.1?(neil) → branch-1.8.1+

    
    

    
  
(In reply to comment #26)
> (From update of attachment 210368 [details] [diff] [review] [edit])
> This is the combination of the previous patches, right?

It's the same as attachment 206647 [details] [diff] [review], minus the change to code introduced in bug 253481, which isn't on the branch.

    
    

    
  
But with the menulist.xml change affecting only editable menulists, right?

    
    

    
  
(In reply to comment #28)
> But with the menulist.xml change affecting only editable menulists, right?

Oh, yes, that too.

    
    

    
  
Comment on attachment 210368 [details] [diff] [review]
1.8 branch patch (xpfe)

Checked in on the 1.8 branch.
mozilla/xpfe/global/resources/content/bindings/menulist.xml; 1.35.8.1;
mozilla/xpfe/global/resources/content/bindings/textbox.xml;  1.32.4.1;

        Attachment #210368 -
        Attachment is obsolete: true

    
    

    
  


  
Not tested yet.

    
    

    
  


  
Not tested yet.

    
    

    
  
Comment on attachment 211582 [details] [diff] [review]
patch, reset and defaultValue for xpfe

The defaultValue property is in fact equivalent to the value attribute, so you can't actually reset a menulist because its value is dynamically updated.

    
  
Blocks: 326556

    
  
No longer blocks: 326556

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch2
        — 
          — Splinter Review
      

    


  
Ah, makes sense. Ok, only for textbox.xml then. Still not tested.

        Attachment #211581 -
        Attachment is obsolete: true

        Attachment #211582 -
        Attachment is obsolete: true

    
  
Assignee: ajvincent → martijn.martijn
Status: ASSIGNED → NEW

    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  
Ok, I tested with this (under chrome), which seems to work well.

    
  

        Attachment #215272 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 215272 [details] [diff] [review]
patch2

It's a pity we can't get at nsHTMLInputElement::Reset()

        Attachment #215272 -
        Flags: review?(neil) → review+

    
    

    
  
Comment on attachment 215272 [details] [diff] [review]
patch2

Sorry, I also need sr+, and I have no idea who els to ask for sr in this component.

        Attachment #215272 -
        Flags: superreview?(neil)

    
    

    
  
Comment on attachment 215272 [details] [diff] [review]
patch2

I'll mark sr but as this includes toolkit and xpfe parts I'd prefer if you got another review instead.

        Attachment #215272 -
        Flags: superreview?(neil) → superreview+

    
  

        Attachment #215272 -
        Flags: review?(mconnor)

    
  

        Attachment #215272 -
        Flags: review?(mconnor) → review?(gavin.sharp)

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

    
    

    
  
Checking in toolkit/content/widgets/textbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/textbox.xml,v  <--  textbox.xml
new revision: 1.42; previous revision: 1.41
done
Checking in xpfe/global/resources/content/bindings/textbox.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/textbox.xml,v  <--  text
box.xml
new revision: 1.45; previous revision: 1.44
done

Checked patch2 into trunk.
I added some explanation about defaultValue and reset() on devmo.
So with that patch checked in, I believe this bug can be resolved->fixed.
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

    
  
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets

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