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

RESOLVED FIXED

Status

()

Core
XUL
--
enhancement
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: WeirdAl, Assigned: Martijn Wargers (dead))

Tracking

({fixed1.8.1})

Trunk
fixed1.8.1
Points:
---
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@parkwaycc.co.uk
: review+
Gavin
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
556 bytes, application/vnd.mozilla.xul+xml
Details
3.71 KB, application/vnd.mozilla.xul+xml
Details
(Reporter)

Description

12 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

12 years ago
Created attachment 200098 [details]
testcase (run as chrome)
(Reporter)

Comment 2

12 years ago
Created attachment 200099 [details] [diff] [review]
patch for xpfe
Attachment #200099 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #200099 - Flags: review?(neil.parkwaycc.co.uk)
(Reporter)

Comment 3

12 years ago
Created attachment 200100 [details] [diff] [review]
patch for toolkit
Attachment #200100 - Flags: review?(mconnor)

Comment 4

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

12 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

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

Comment 7

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

Comment 8

12 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

12 years ago
Created attachment 200195 [details] [diff] [review]
patch for toolkit
Attachment #200195 - Flags: review?(mconnor)

Comment 10

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

12 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

12 years ago
Attachment #200195 - Attachment is obsolete: true

Comment 12

12 years ago
(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

12 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

12 years ago
Created attachment 206647 [details] [diff] [review]
patch for xpfe (implement editor)
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

12 years ago
Created attachment 206648 [details] [diff] [review]
patch for toolkit (expose editor) [checked in]
Attachment #206648 - Flags: review?(mconnor)

Comment 16

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

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

Comment 19

12 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

12 years ago
Blocks: 324354

Updated

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

Comment 20

12 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
Created attachment 210365 [details] [diff] [review]
1.8 branch patch (toolkit)
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

12 years ago
oh, I forgot about the FF2 series.

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

Updated

12 years ago
Attachment #210365 - Flags: branch-1.8.1?(mconnor) → branch-1.8.1+
Created attachment 210368 [details] [diff] [review]
1.8 branch patch (xpfe)

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 26

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

Comment 28

12 years ago
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
Keywords: fixed1.8.1
(Assignee)

Comment 31

12 years ago
Created attachment 211581 [details] [diff] [review]
patch, reset and defaultValue for toolkit

Not tested yet.
(Assignee)

Comment 32

12 years ago
Created attachment 211582 [details] [diff] [review]
patch, reset and defaultValue for xpfe

Not tested yet.

Comment 33

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

12 years ago
Blocks: 326556

Updated

12 years ago
No longer blocks: 326556
(Assignee)

Comment 34

12 years ago
Created attachment 215272 [details] [diff] [review]
patch2

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

11 years ago
Assignee: ajvincent → martijn.martijn
Status: ASSIGNED → NEW
(Assignee)

Comment 35

11 years ago
Created attachment 227183 [details]
testcase

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

Updated

11 years ago
Attachment #215272 - Flags: review?(neil)

Comment 36

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

11 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 38

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

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

Updated

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

Comment 39

11 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: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Assignee)

Comment 40

11 years ago
Created attachment 249096 [details]
Unit test for reset() and defaultValue

Updated

9 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.