Last Comment Bug 312867 - Implement textbox.reset(), expose input's editor
: Implement textbox.reset(), expose input's editor
Status: RESOLVED FIXED
: fixed1.8.1
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Martijn Wargers [:mwargers] (not working for Mozilla)
:
Mentors:
Depends on:
Blocks: 324354
  Show dependency treegraph
 
Reported: 2005-10-18 09:23 PDT by Alex Vincent [:WeirdAl]
Modified: 2008-07-31 03:19 PDT (History)
7 users (show)
martijn.martijn: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (run as chrome) (1.08 KB, application/vnd.mozilla.xul+xml)
2005-10-19 09:32 PDT, Alex Vincent [:WeirdAl]
no flags Details
patch for xpfe (4.26 KB, patch)
2005-10-19 09:32 PDT, Alex Vincent [:WeirdAl]
neil: review-
neil: superreview-
Details | Diff | Review
patch for toolkit (4.26 KB, patch)
2005-10-19 09:33 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review
patch for toolkit (4.10 KB, patch)
2005-10-20 07:11 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review
patch for xpfe (implement editor) (3.67 KB, patch)
2005-12-22 14:06 PST, Alex Vincent [:WeirdAl]
neil: review+
neil: superreview+
Details | Diff | Review
patch for toolkit (expose editor) [checked in] (3.57 KB, patch)
2005-12-22 14:12 PST, Alex Vincent [:WeirdAl]
mconnor: review+
Details | Diff | Review
1.8 branch patch (toolkit) (2.71 KB, patch)
2006-02-01 10:31 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
mconnor: approval‑branch‑1.8.1+
Details | Diff | Review
1.8 branch patch (xpfe) (2.83 KB, patch)
2006-02-01 10:42 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
neil: approval‑branch‑1.8.1+
Details | Diff | Review
patch, reset and defaultValue for toolkit (4.28 KB, patch)
2006-02-12 04:17 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Review
patch, reset and defaultValue for xpfe (4.40 KB, patch)
2006-02-12 04:32 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Review
patch2 (4.98 KB, patch)
2006-03-16 08:20 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
neil: review+
gavin.sharp: review+
neil: superreview+
Details | Diff | Review
testcase (556 bytes, application/vnd.mozilla.xul+xml)
2006-06-26 20:37 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Unit test for reset() and defaultValue (3.71 KB, application/vnd.mozilla.xul+xml)
2006-12-19 03:53 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details

Description Alex Vincent [:WeirdAl] 2005-10-18 09:23:46 PDT
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.
Comment 1 Alex Vincent [:WeirdAl] 2005-10-19 09:32:00 PDT
Created attachment 200098 [details]
testcase (run as chrome)
Comment 2 Alex Vincent [:WeirdAl] 2005-10-19 09:32:36 PDT
Created attachment 200099 [details] [diff] [review]
patch for xpfe
Comment 3 Alex Vincent [:WeirdAl] 2005-10-19 09:33:16 PDT
Created attachment 200100 [details] [diff] [review]
patch for toolkit
Comment 4 neil@parkwaycc.co.uk 2005-10-19 10:03:47 PDT
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 :-)
Comment 5 Alex Vincent [:WeirdAl] 2005-10-19 11:10:56 PDT
> 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.
Comment 6 Alex Vincent [:WeirdAl] 2005-10-19 21:10:30 PDT
Note to self:  if the patches get r-, I should also implement a
resetToValue(aValue) method.  
Comment 7 Daniel Cater 2005-10-20 03:32:19 PDT
Attachment 200100 [details] [diff] is not for toolkit as stated.
Comment 8 Alex Vincent [:WeirdAl] 2005-10-20 07:10:45 PDT
Comment on attachment 200100 [details] [diff] [review]
patch for toolkit

Whoops.  Uploaded the wrong file.
Comment 9 Alex Vincent [:WeirdAl] 2005-10-20 07:11:33 PDT
Created attachment 200195 [details] [diff] [review]
patch for toolkit
Comment 10 neil@parkwaycc.co.uk 2005-11-02 05:36:56 PST
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).
Comment 11 Alex Vincent [:WeirdAl] 2005-11-02 07:23:15 PST
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.
Comment 12 neil@parkwaycc.co.uk 2005-11-02 12:04:33 PST
(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.
Comment 13 Alex Vincent [:WeirdAl] 2005-12-22 13:50:01 PST
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.)
Comment 14 Alex Vincent [:WeirdAl] 2005-12-22 14:06:08 PST
Created attachment 206647 [details] [diff] [review]
patch for xpfe (implement editor)
Comment 15 Alex Vincent [:WeirdAl] 2005-12-22 14:12:23 PST
Created attachment 206648 [details] [diff] [review]
patch for toolkit (expose editor) [checked in]
Comment 16 neil@parkwaycc.co.uk 2005-12-23 03:16:51 PST
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
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-23 06:40:31 PST
Neil is a toolkit peer, no need to split the patches when they're identical like this one! :)
Comment 18 Alex Vincent [:WeirdAl] 2005-12-23 08:50:18 PST
gavin: if Neil wants to give r+ to the toolkit patch, that's his call.  :)
Comment 19 Alex Vincent [:WeirdAl] 2005-12-23 14:52:46 PST
Comment on attachment 206647 [details] [diff] [review]
patch for xpfe (implement editor)

xpfe patch for editor checked in by timeless.
Comment 20 Alex Vincent [:WeirdAl] 2006-01-24 13:02:45 PST
I'll need someone to check in the patch, please.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-01-24 17:15:32 PST
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;
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-02-01 10:31:59 PST
Created attachment 210365 [details] [diff] [review]
1.8 branch patch (toolkit)
Comment 23 Alex Vincent [:WeirdAl] 2006-02-01 10:34:29 PST
oh, I forgot about the FF2 series.

gavin, do you want to patch xpfe (SeaMonkey) as well?
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-02-01 10:42:38 PST
Created attachment 210368 [details] [diff] [review]
1.8 branch patch (xpfe)

Not sure what the SM branch plans are, but here's the patch.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-02-01 10:45:09 PST
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;
Comment 26 neil@parkwaycc.co.uk 2006-02-01 16:53:40 PST
Comment on attachment 210368 [details] [diff] [review]
1.8 branch patch (xpfe)

This is the combination of the previous patches, right?
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-02-01 16:57:20 PST
(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.
Comment 28 neil@parkwaycc.co.uk 2006-02-01 17:13:25 PST
But with the menulist.xml change affecting only editable menulists, right?
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-02-01 17:16:44 PST
(In reply to comment #28)
> But with the menulist.xml change affecting only editable menulists, right?

Oh, yes, that too.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-02-02 07:43:55 PST
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;
Comment 31 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-02-12 04:17:56 PST
Created attachment 211581 [details] [diff] [review]
patch, reset and defaultValue for toolkit

Not tested yet.
Comment 32 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-02-12 04:32:08 PST
Created attachment 211582 [details] [diff] [review]
patch, reset and defaultValue for xpfe

Not tested yet.
Comment 33 neil@parkwaycc.co.uk 2006-02-22 16:57:24 PST
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.
Comment 34 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-16 08:20:45 PST
Created attachment 215272 [details] [diff] [review]
patch2

Ah, makes sense. Ok, only for textbox.xml then. Still not tested.
Comment 35 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-06-26 20:37:36 PDT
Created attachment 227183 [details]
testcase

Ok, I tested with this (under chrome), which seems to work well.
Comment 36 neil@parkwaycc.co.uk 2006-06-28 06:17:10 PDT
Comment on attachment 215272 [details] [diff] [review]
patch2

It's a pity we can't get at nsHTMLInputElement::Reset()
Comment 37 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-07-21 06:13:51 PDT
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.
Comment 38 neil@parkwaycc.co.uk 2006-07-21 07:34:45 PDT
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.
Comment 39 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-12-19 03:19:14 PST
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.
Comment 40 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-12-19 03:53:22 PST
Created attachment 249096 [details]
Unit test for reset() and defaultValue

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