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

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: WeirdAl, Assigned: martijn.martijn)

Tracking

({fixed1.8.1})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 9 obsolete attachments)

1.08 KB, application/vnd.mozilla.xul+xml
Details
4.98 KB, patch
neil
: review+
Gavin
: review+
Details | Diff | Splinter Review
556 bytes, application/vnd.mozilla.xul+xml
Details
3.71 KB, application/vnd.mozilla.xul+xml
Details
Reporter

Description

14 years ago
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.
Reporter

Comment 1

14 years ago
Reporter

Comment 2

14 years ago
Posted patch patch for xpfe (obsolete) — Splinter Review
Attachment #200099 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #200099 - Flags: review?(neil.parkwaycc.co.uk)
Reporter

Comment 3

14 years ago
Posted 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 :-)
Reporter

Comment 5

14 years ago
> 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.
Reporter

Comment 6

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

Comment 7

14 years ago
Attachment 200100 [details] [diff] is not for toolkit as stated.
Reporter

Comment 8

14 years ago
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)
Reporter

Comment 9

14 years ago
Posted 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-
Reporter

Comment 11

14 years ago
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)
Reporter

Updated

14 years ago
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.
Reporter

Comment 13

14 years ago
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
Reporter

Comment 14

14 years ago
Attachment #200099 - Attachment is obsolete: true
Attachment #206647 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #206647 - Flags: review?(neil.parkwaycc.co.uk)
Reporter

Comment 15

14 years ago
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! :)
Reporter

Comment 18

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

Comment 19

14 years ago
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
Assignee

Updated

14 years ago
Blocks: 324354
Attachment #206648 - Flags: review?(mconnor) → review+
Reporter

Comment 20

14 years ago
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
Posted 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)
Reporter

Comment 23

14 years ago
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+
Posted 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
Assignee

Comment 31

14 years ago
Not tested yet.
Assignee

Comment 32

14 years ago
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.

Updated

13 years ago
Blocks: 326556

Updated

13 years ago
No longer blocks: 326556
Assignee

Comment 34

13 years ago
Posted patch patch2Splinter 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
Reporter

Updated

13 years ago
Assignee: ajvincent → martijn.martijn
Status: ASSIGNED → NEW
Assignee

Comment 35

13 years ago
Posted file testcase
Ok, I tested with this (under chrome), which seems to work well.
Assignee

Updated

13 years ago
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+
Assignee

Comment 37

13 years ago
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+
Assignee

Updated

13 years ago
Attachment #215272 - Flags: review?(mconnor)
Assignee

Updated

13 years ago
Attachment #215272 - Flags: review?(mconnor) → review?(gavin.sharp)
Attachment #215272 - Flags: review?(gavin.sharp) → review+
Assignee

Comment 39

13 years ago
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
Last Resolved: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

Updated

11 years ago
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.