Last Comment Bug 253481 - xul:textbox attribute to specify what happens to pasted line breaks
: xul:textbox attribute to specify what happens to pasted line breaks
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Ted Mielczarek [:ted.mielczarek]
: sairuh (rarely reading bugmail)
Mentors:
Depends on: 319882 321238
Blocks: 23485 264568 329668 483385
  Show dependency treegraph
 
Reported: 2004-07-28 17:55 PDT by Jesse Ruderman
Modified: 2009-03-14 05:19 PDT (History)
10 users (show)
ted: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (717 bytes, text/html)
2005-09-15 16:21 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details
editor changes (8.79 KB, patch)
2005-09-15 16:37 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
content and layout changes (6.24 KB, patch)
2005-09-15 16:41 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Updated editor changes (9.40 KB, patch)
2005-09-15 19:42 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
textbox XBL binding changes (2.20 KB, patch)
2005-09-16 08:00 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
xul testcase (will not work remotely) (737 bytes, application/vnd.mozilla.xul+xml)
2005-09-16 08:02 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details
editor patch with review comments (10.15 KB, patch)
2005-09-17 18:54 PDT, Ted Mielczarek [:ted.mielczarek]
brade: review+
Details | Diff | Splinter Review
Updated binding changes (2.38 KB, patch)
2005-09-17 18:55 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Updated editor patch (10.09 KB, patch)
2005-11-03 06:43 PST, Ted Mielczarek [:ted.mielczarek]
ted: review+
Details | Diff | Splinter Review
editor patch without that misplaced comma (10.02 KB, patch)
2005-11-03 07:17 PST, Ted Mielczarek [:ted.mielczarek]
ted: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
updated binding patch (2.47 KB, patch)
2005-11-03 08:11 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
equivalent XPFE binding patch (2.42 KB, patch)
2005-11-03 08:22 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
final editor patch (10.28 KB, patch)
2005-11-03 14:48 PST, Ted Mielczarek [:ted.mielczarek]
ted: review+
ted: superreview+
Details | Diff | Splinter Review
more final editor patch (10.26 KB, patch)
2005-11-06 14:26 PST, Ted Mielczarek [:ted.mielczarek]
ted: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
reworked toolkit binding patch (2.06 KB, patch)
2005-11-06 14:27 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
updated testcase (requires chrome privs) (778 bytes, application/vnd.mozilla.xul+xml)
2005-11-06 14:32 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details
toolkit patch (1.95 KB, patch)
2005-11-10 21:01 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
xpfe patch (1.90 KB, patch)
2005-11-10 21:02 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
toolkit patch (1.93 KB, patch)
2005-11-15 09:06 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
xpfe patch (1.89 KB, patch)
2005-11-15 09:06 PST, Ted Mielczarek [:ted.mielczarek]
neil: review+
Details | Diff | Splinter Review
editor patch (10.97 KB, patch)
2005-12-11 10:54 PST, Ted Mielczarek [:ted.mielczarek]
neil: review+
ted: superreview+
Details | Diff | Splinter Review
Updated toolkit patch (1.88 KB, patch)
2005-12-11 11:00 PST, Ted Mielczarek [:ted.mielczarek]
neil: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
Toolkit patch (2.71 KB, patch)
2005-12-17 21:25 PST, Ted Mielczarek [:ted.mielczarek]
ted: review+
ted: superreview+
Details | Diff | Splinter Review
xpfe patch (1.89 KB, patch)
2005-12-17 21:30 PST, Ted Mielczarek [:ted.mielczarek]
ted: review+
ted: superreview+
Details | Diff | Splinter Review
mochichrome test (3.60 KB, patch)
2007-09-06 16:48 PDT, Ted Mielczarek [:ted.mielczarek]
enndeakin: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2004-07-28 17:55:17 PDT
XUL textboxes should be able to specify what happens when text containing line
breaks is pasted.  It's silly that Thunderbird has to set a global pref to make
pasting multiple addresses into the address widget work, and it sucks that
Firefox has no way to strip line breaks from address bar pastes.

This attribute could override or replace editor.singleLine.pasteNewlines.  I
think it should replace it.
Comment 1 Akkana Peck 2004-07-29 10:58:24 PDT
Changing platform to all.

The main reason for the pref is that there are platform differences.  Is it
realistic to assume that most XUL apps will have separate XUL code for each
platform?  It's probably best to keep the current pref for use as a default, but
allow XUL to override it.  For instance, allowing multiline paste in the urlbar
could solve the "broken url" problem for Windows and Mac (bug 23485; it would
probably be better to fix urlbar paste behavior, but no one seems to have
figured out how to do that), without creating unexpected behavior in other text
fields.

Another attribute that could perhaps be considered at the same time: the word
selection attribute, layout.word_select.stop_at_punctuation.  This would neatly
solve problems like people wanting word selection to stop at punctuation in the
editor, yet select entire urls with a doubleclick in the urlbar (e.g. bug
190615).  Though that might be harder since it would require hooks in the frame
code (the editor paste code is fairly straightforward, but can the frame code
query XUL to see whether it has any related attributes?)
Comment 2 Ted Mielczarek [:ted.mielczarek] 2005-09-15 14:00:07 PDT
I wrote some code, but I haven't tested it yet.  Here's what I did.  Feel free
to sanity check my logic, hopefully I'll post a patch tonight.

in content/html/content/src/nsHTMLAtomList.h:
   add newlines "newlines"

in editor/idl/nsIPlaintextEditor:
   add a property for setting and getting the "handle newlines" value
   add constants for the various values

in editor/libeditor/text/nsPlainTextEditor.{h,cpp}:
   implement property getter/setter
   respect the pref for the default value

in layout/forms/nsFormControlHelper.{h,cpp}:
   add methods to get the value of the "newlines" attribute

in layout/forms/nsTextControlFrame.cpp ::CreateAnonymousContent:
   // Initialize the plaintext editor
   get the property value
   set the value in the plaintext editor

in editor/libeditor/text/nsTextEditRules.cpp:
   use the plaintext editor value to drive the single line newline behavior
Comment 3 Ted Mielczarek [:ted.mielczarek] 2005-09-15 16:21:21 PDT
Created attachment 196255 [details]
Testcase

Testcase for the patch I'm going to attach next.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2005-09-15 16:37:43 PDT
Created attachment 196258 [details] [diff] [review]
editor changes

Changes to editor.  Adds a "newlineHandling" attribute to nsIPlaintextEditor
(along with enumerated constants), sets the default for text input controls
based on the pref "editor.singleLine.pasteNewlines", and when pasting
multi-line data into single-line controls, uses the value of newlineHandling
for the plain text editor to control the behavior.

I have also added a sixth(!) option for the newline handling, which I still
have to implement: strip newlines and all whitespace surrounding them.	I think
this would make the most sense for the URL bar.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2005-09-15 16:41:04 PDT
Created attachment 196259 [details] [diff] [review]
content and layout changes

Content and layout changes.  This basically adds an attribute "newlines" to
html:input type="text" that sets the newlineHandling of the associated plain
text editor.  The valid values for newlines are "intact", "firstline",
"spaces", "strip", "commas", and "stripwhitespace".

I'm not terribly keen on adding an attribute to html:input, so if someone can
point me in a better direction using the editor changes in the other patch, I'd
appreciate it.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2005-09-15 19:42:17 PDT
Created attachment 196274 [details] [diff] [review]
Updated editor changes

Implemented the last option, it strips CR/LF and surrounding whitespace. 
That's the option I intend for use in the URL bar.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-15 22:59:09 PDT
(In reply to comment #5)
> I'm not terribly keen on adding an attribute to html:input, so if someone can
> point me in a better direction using the editor changes in the other patch, I'd
> appreciate it.

Nor am I.  Any chance you could somehow expose the editor to the textbox XBL
binding and set things directly on the editor?  Not sure how, though.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2005-09-16 06:23:47 PDT
Oh yeah, it looks like I can probably just use the interface added in bug 303727
to get an editor from an input, and set it that way.  The editor changes patch
from here is still valid, but the content/layout stuff can be tossed out.  I'll
write an XBL patch soon for this.
Comment 9 Kathleen Brade 2005-09-16 06:46:58 PDT
Comment on attachment 196274 [details] [diff] [review]
Updated editor changes

I'm not sure what platforms you have already tested this on; please get some
testing on all platforms before this lands.

In nsTextEditRules.cpp, please renumber the six possible options; start with 1
and put the default item as first in the list.

I'm a little concerned about the eNewlinesStripWhitespace block of code (should
this be a switch?):
 * If you remove all of the whitespace before and after it, won't the words
wrap together?	Is the comment wrong or am I missing something or ?
Comment 10 Ted Mielczarek [:ted.mielczarek] 2005-09-16 07:16:20 PDT
(In reply to comment #9)
> (From update of attachment 196274 [details] [diff] [review] [edit])
> I'm not sure what platforms you have already tested this on; please get some
> testing on all platforms before this lands.

Tested on win32 only so far, I'll test on linux after I address your changes. 
I'll have to get someone else for Mac coverage.

> In nsTextEditRules.cpp, please renumber the six possible options; start with 1
> and put the default item as first in the list.
> 
> I'm a little concerned about the eNewlinesStripWhitespace block of code (should
> this be a switch?):

I'll make it a switch, I was just reusing the existing code structure for now.

>  * If you remove all of the whitespace before and after it, won't the words
> wrap together?	Is the comment wrong or am I missing something or ?

No, that's correct.  My intent is to use this only for the URL bar.  URLs that
are forcibly wrapped tend to have whitespace along with the newlines  (think IRC
in a terminal window).  In this scenario, I think removing whitespace
surrounding the newlines is the right thing to do.  Keep in mind that this
action will only happen if a xul:textbox is explicitly set to
newlines="stripwhitespace".

I'll address your points in an updated patch soon.

Comment 11 Ted Mielczarek [:ted.mielczarek] 2005-09-16 08:00:21 PDT
Created attachment 196328 [details] [diff] [review]
textbox XBL binding changes

Uses the exposed editor field to set the newline handling of the html:input
contained in a xul:textbox.  This won't work in untrusted XUL, due to that bug
about XBL bindings executing in the context of the page they're bound to.
Comment 12 Ted Mielczarek [:ted.mielczarek] 2005-09-16 08:02:32 PDT
Created attachment 196330 [details]
xul testcase (will not work remotely)

To try this testcase you'll have to download it and put it somewhere with
chrome privileges.  Sorry.
Comment 13 Kathleen Brade 2005-09-16 13:14:53 PDT
>No, that's correct.  My intent is to use this only for the URL bar.  URLs that
>are forcibly wrapped tend to have whitespace along with the newlines  (think IRC
>in a terminal window).  In this scenario, I think removing whitespace
>surrounding the newlines is the right thing to do.  Keep in mind that this
>action will only happen if a xul:textbox is explicitly set to
>newlines="stripwhitespace".

Perhaps the enum (eNewlinesStripWhitespace) should be renamed to something like: 
 eNewlinesStripForURLBar
(Maybe someone else can suggest a better one?)

If I see "strip whitespace" I think that it removes all whitespace including
spaces in the middle (which this does not do).

One last nit I noticed:
+  while(wsEnd < tString.Length() && NS_IS_SPACE(tString[wsEnd + 1]))
Please add a space after 'while'.
Thanks!
Comment 14 Kathleen Brade 2005-09-16 13:15:20 PDT
Comment on attachment 196274 [details] [diff] [review]
Updated editor changes

(remove review request since there will be changes)
Comment 15 Ted Mielczarek [:ted.mielczarek] 2005-09-17 18:54:06 PDT
Created attachment 196489 [details] [diff] [review]
editor patch with review comments

Fixes all of brade's comments.	The last newline handling option has been
renamed to eNewlinesStripSurroundingWhitespace, which is verbose but works.
Comment 16 Ted Mielczarek [:ted.mielczarek] 2005-09-17 18:55:27 PDT
Created attachment 196490 [details] [diff] [review]
Updated binding changes

Changed to reflect the naming change in the editor patch.  I'm not hot on the
attribute value I've used here, newlines="stripsurroundingwhitespace" is
awfully verbose.  Suggestions are welcome.
Comment 17 Kathleen Brade 2005-09-21 03:27:36 PDT
Comment on attachment 196489 [details] [diff] [review]
editor patch with review comments

r=brade
Comment 18 neil@parkwaycc.co.uk 2005-11-02 00:21:54 PST
Comment on attachment 196490 [details] [diff] [review]
Updated binding changes

Is this going to get enough use to make it more worthwhile than gURLBar.editor.QueryInterface(nsIPlaintextEditor).newlineHandling = nsIPlaintextEditor.eNewlinesStripSurroundingWhitespace; ?
Comment 19 neil@parkwaycc.co.uk 2005-11-02 05:05:25 PST
Comment on attachment 196489 [details] [diff] [review]
editor patch with review comments

> #include "nsIDOMKeyEvent.h"
> %}
> 
> [ptr] native nsIDOMKeyEvent(nsIDOMKeyEvent);
Not your fault, but this stuff could be replaced by
interface nsIDOMKeyEvent;
thus making HandleKeyPress scriptable.
>   [noscript]void handleKeyPress(in nsIDOMKeyEvent aKeyEvent);

>+, mNewlineHandling(nsIPlaintextEditor::eNewlinesPasteToFirst)
> , mMaxTextLength(-1)
> , mInitTriggerCounter(0)
Initializers should be in class declaration order. If your compiler didn't warn you, get a better compiler :-P

>+  nsresult prefres;
>+  nsCOMPtr<nsIPrefBranch> prefBranch =
>+    do_GetService(NS_PREFSERVICE_CONTRACTID, &prefres);
>+  if (NS_SUCCEEDED(prefres) && prefBranch)
You don't need to check prefres if you're also doing a null check.

>+  if (! aNewlineHandling)
>+    return NS_ERROR_NULL_POINTER;
NS_ENSURE_ARG_POINTER(aNewlineHandling);

>+    mEditor->GetNewlineHandling(&singleLineNewlineBehavior);
nsTextEditRules is a friend of nsPlaintextEditor so just use switch(mEditor->mNewlineHandling) here and below.
Comment 20 Ted Mielczarek [:ted.mielczarek] 2005-11-02 11:00:00 PST
(In reply to comment #18)
> (From update of attachment 196490 [details] [diff] [review] [edit])
> Is this going to get enough use to make it more worthwhile than
> gURLBar.editor.QueryInterface(nsIPlaintextEditor).newlineHandling =
> nsIPlaintextEditor.eNewlinesStripSurroundingWhitespace; ?

Given the number of different interpretations of how newlines should be handled, I think there's enough demand to justify an attribute on xul:textbox.  It's going to be a lot nicer to see <textbox newlines="stripsurroundingwhitespace" id=...> than to have to use JS to define it anywhere we want it.  This will likely affect more than just the URL bar.  I believe Thunderbird sets the newlines pref to handle pasting into the To: textbox, and there's a dependent bug on this for Chatzilla handling newlines in the chat input box.
Comment 21 Ted Mielczarek [:ted.mielczarek] 2005-11-03 06:43:23 PST
Created attachment 201743 [details] [diff] [review]
Updated editor patch

Addresses neil's comments, fixes the array out of bounds in the "strip surrounding whitespace" loop that jag pointed out, added a default case to the switch.
Comment 22 Ted Mielczarek [:ted.mielczarek] 2005-11-03 07:17:18 PST
Created attachment 201747 [details] [diff] [review]
editor patch without that misplaced comma

That'll teach me not to build before I submit a patch.
Comment 23 Ted Mielczarek [:ted.mielczarek] 2005-11-03 08:11:10 PST
Created attachment 201754 [details] [diff] [review]
updated binding patch

Apparently on trunk you need to QI the input field to get at its editor.
Comment 24 Ted Mielczarek [:ted.mielczarek] 2005-11-03 08:22:12 PST
Created attachment 201758 [details] [diff] [review]
equivalent XPFE binding patch

Same thing for XPFE.
Comment 25 jag (Peter Annema) 2005-11-03 14:22:01 PST
Comment on attachment 201747 [details] [diff] [review]
editor patch without that misplaced comma

Just picking some nits, your patch looks fine otherwise:

>Index: editor/idl/nsIPlaintextEditor.idl
>===================================================================
>RCS file: /cvsroot/mozilla/editor/idl/nsIPlaintextEditor.idl,v
>retrieving revision 1.13
>diff -u -p -5 -r1.13 nsIPlaintextEditor.idl
>--- editor/idl/nsIPlaintextEditor.idl	17 Oct 2005 00:54:02 -0000	1.13
>+++ editor/idl/nsIPlaintextEditor.idl	3 Nov 2005 15:15:49 -0000
>@@ -41,11 +41,11 @@
> #include "nsIDOMKeyEvent.h"
> %}
> 
> [ptr] native nsIDOMKeyEvent(nsIDOMKeyEvent);

Though not related to this patch, could you help us clean this up by changing this to interface nsIDOMKeyEvent? Then you can also remove the above c++ section (I'm assuming that everyone who needs nsIDOMKeyEvent.h is already including it).

>Index: editor/libeditor/text/nsPlaintextEditor.cpp
>===================================================================

>+  // check the "single line editor newline handling" pref
>+  nsresult prefres;
>+  nsCOMPtr<nsIPrefBranch> prefBranch =
>+    do_GetService(NS_PREFSERVICE_CONTRACTID, &prefres);
>+  if (NS_SUCCEEDED(prefres))
>+    prefBranch->GetIntPref("editor.singleLine.pasteNewlines",
>+                           &mNewlineHandling);

Since you don't return |prefres| I'd rather you do

  nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID));
  if (prefBranch)

>Index: editor/libeditor/text/nsTextEditRules.cpp
>===================================================================

Instead of looking ahead (wsEnd + 1) you can look at the current character to decide when to stop. You'll end up one character past the last character of the range you're interested in. The nice property you then get is that (end - begin) is the length of the range. Here is the changed code:

          PRUint32 wsBegin = firstCRLF, wsEnd = firstCRLF + 1;

          while (wsEnd < tString.Length() && NS_IS_SPACE(tString[wsEnd]))
            ++wsEnd;

          // now cut this range out of the string
          tString.Cut(wsBegin, wsEnd - wsBegin);

sr=jag with these nits addressed.
Comment 26 Ted Mielczarek [:ted.mielczarek] 2005-11-03 14:48:18 PST
Created attachment 201792 [details] [diff] [review]
final editor patch

Addresses jag's comments.  This patch can be checked in without the binding patches, since it checks the pref to use as the default value everything should continue to work as normal.
Comment 27 neil@parkwaycc.co.uk 2005-11-03 16:11:52 PST
For future proofing, you could require that all newlines attribute values be names taken from the .idl but without the "eNewline" prefix, like so:
const nsIPlaintextEditor = Components.interfaces.nsIPlaintextEditor;
const nsIDOMNSEditableElement = Components.interfaces.nsIDOMNSEditableElement;
str = "eNewline" + this.getAttribute('newlines');
if (str in nsIPlaintextEditor)
  this.inputField.QueryInterface(nsIDOMNSEditableElement)
      .editor.QueryInterface(nsIPlaintextEditor)
      .newlineHandling = nsIPlaintextEditor[str];
Comment 28 Ted Mielczarek [:ted.mielczarek] 2005-11-03 19:05:25 PST
Neil: my only issue with that is that either the attribute values then have to be MixedCase, or we have to rename the enumerated values to be lowercase, both of which contradict traditional style.  I do agree that it's a nicer solution, however.  Any ideas?
Comment 29 jag (Peter Annema) 2005-11-04 13:46:55 PST
Add a lookup table that maps the lowercase strings to the enums.
Comment 30 Ted Mielczarek [:ted.mielczarek] 2005-11-06 14:26:57 PST
Created attachment 202039 [details] [diff] [review]
more final editor patch

With jag's other nit addressed.  jag, want to make sure you're happy with it?
Comment 31 Ted Mielczarek [:ted.mielczarek] 2005-11-06 14:27:52 PST
Created attachment 202040 [details] [diff] [review]
reworked toolkit binding patch

Reworked to be more spiffy
Comment 32 Ted Mielczarek [:ted.mielczarek] 2005-11-06 14:32:08 PST
Created attachment 202041 [details]
updated testcase (requires chrome privs)
Comment 33 neil@parkwaycc.co.uk 2005-11-07 11:47:03 PST
Comment on attachment 202040 [details] [diff] [review]
reworked toolkit binding patch

>+              if(x.match(/^eNewlines/)) {
match is inappropriate for a boolean test.

>+                if(str == x.replace(/^eNewlines/,'').toLowerCase()) {
Could use RegExp.rightContext here.

Also, space between if and (

>+          catch(ex) {} // we'll get a security error in non-chrome xul
Then they shouldn't set the attribute...

I did a quick check for upper case attributes values, and only found two; one used in Mail menus and one used in the Suite component bar (Mail component!); we have quite a lot of mixed-case attribute names and ids (which I suppose are a special case of attribute values).
Comment 34 jag (Peter Annema) 2005-11-09 17:16:19 PST
Comment on attachment 202040 [details] [diff] [review]
reworked toolkit binding patch

I like this approach, except for having to enumerate all the properties on nsIPlainttextEditor. I don't see a better approach though.

And Neil's right, the security exception in non-chrome XUL will be more informative than silently failing.

One more nit:

+          this.inputField.value=str;

Put spaces around =

Attach a new patch with this and Neil's comments addressed, for both toolkit and xpfe, and I think we've got a winner.
Comment 35 Ted Mielczarek [:ted.mielczarek] 2005-11-09 22:51:29 PST
I'm not sure if not catching an exception in an XBL constructor will break things in XUL.  I'll check that out before I update the patch.
Comment 36 Ted Mielczarek [:ted.mielczarek] 2005-11-10 21:01:05 PST
Created attachment 202621 [details] [diff] [review]
toolkit patch

Done and done
Comment 37 Ted Mielczarek [:ted.mielczarek] 2005-11-10 21:02:01 PST
Created attachment 202622 [details] [diff] [review]
xpfe patch

Same thing, xpfe
Comment 38 Ted Mielczarek [:ted.mielczarek] 2005-11-15 09:06:07 PST
Created attachment 203133 [details] [diff] [review]
toolkit patch

Changed to use RegExp.test
Comment 39 Ted Mielczarek [:ted.mielczarek] 2005-11-15 09:06:44 PST
Created attachment 203134 [details] [diff] [review]
xpfe patch

same for xpfe
Comment 40 neil@parkwaycc.co.uk 2005-11-16 01:01:51 PST
(In reply to comment #25)
>I'm assuming that everyone who needs nsIDOMKeyEvent.h is already including it).
Everyone except mozInlineSpellChecker.cpp :-/
Comment 41 neil@parkwaycc.co.uk 2005-11-16 02:23:30 PST
Two more issues:
1. You changed the value of some of the enums without updating anyone's prefs
2. I assume you are requiring the newlines attribute value to be in lower case?
Comment 42 neil@parkwaycc.co.uk 2005-11-16 02:40:33 PST
Oh, and which fields are you proposing to affect? For starters, a quick scan of LXR turns up the following URL autocomplete fields: editor link properties; DOM Inspector; Chatzilla prefs; URLbar; open location dialog; home page pref; email addressing; address lists; mail start page pref; attach page dialog.
Comment 43 Ted Mielczarek [:ted.mielczarek] 2005-11-16 07:59:37 PST
(In reply to comment #41)
> Two more issues:
> 1. You changed the value of some of the enums without updating anyone's prefs

Ouch, good catch.  I guess for compatibility's sake we need to switch back to the previous values.  Maybe file a followup bug on removing use of the pref from the tree?

> 2. I assume you are requiring the newlines attribute value to be in lower case?

Yes.

(In reply to comment #42)
> Oh, and which fields are you proposing to affect? For starters, a quick scan of
> LXR turns up the following URL autocomplete fields: editor link properties; DOM
> Inspector; Chatzilla prefs; URLbar; open location dialog; home page pref; email
> addressing; address lists; mail start page pref; attach page dialog.
> 

The primary target would be the URLbar, but any textbox expecting URL input is a fair target.  I would expect one or more followup bugs to use the pref anywhere it's appropriate.  If we leave the enum values unchanged from the originals, we shouldn't break any existing pref'ed behavior.(In reply to comment #40)

> (In reply to comment #25)
> >I'm assuming that everyone who needs nsIDOMKeyEvent.h is already including it).
> Everyone except mozInlineSpellChecker.cpp :-/

Do we need to undo that IDL change, or just fix that separately?


Comment 44 Ted Mielczarek [:ted.mielczarek] 2005-12-11 10:54:44 PST
Created attachment 205562 [details] [diff] [review]
editor patch

Ok.  Restored numbering of the options back to the originals, so as not to break existing pref users.  Added an include of nsIDOMKeyEvent.h in mozInlineSpellCheck.cpp to avoid breaking spellcheck.
Comment 45 Ted Mielczarek [:ted.mielczarek] 2005-12-11 11:00:05 PST
Created attachment 205563 [details] [diff] [review]
Updated toolkit patch

Updating toolkit patch due to changes in bug 302050
Comment 46 neil@parkwaycc.co.uk 2005-12-13 10:06:38 PST
So, I tried this, and it works great in the URLbar; I then tried it in the Compose addressing widget, but that doesn't work so well; editor is null :-(
Comment 47 neil@parkwaycc.co.uk 2005-12-13 17:07:38 PST
(In reply to comment #46)
>So, I tried this, and it works great in the URLbar; I then tried it in the
>Compose addressing widget, but that doesn't work so well; editor is null :-(
Sigh. Compose is just being weird. Something to do with it cloning nodes.
Comment 48 neil@parkwaycc.co.uk 2005-12-13 17:10:00 PST
Comment on attachment 205562 [details] [diff] [review]
editor patch

r=me on the changes from attachment 202039 [details] [diff] [review].
Comment 49 neil@parkwaycc.co.uk 2005-12-13 17:14:55 PST
Comment on attachment 205563 [details] [diff] [review]
Updated toolkit patch

>-        var str = this.boxObject.getProperty("value");
>+        var str = this.boxObject.getProperty("value"); 
Oops, you dropped a space in at the end of this line. (I hope your xpfe patch doesn't have any lines with trailing spaces, I didn't think to check)

>         }
>         str = this.getAttribute("spellcheck");
...
>         }
>+        str = this.getAttribute("newlines");
I'd probably put blank lines between your sections. And I'd put your code before spellcheck, it's obviously more important ;-)
Comment 50 jag (Peter Annema) 2005-12-13 20:31:28 PST
Ted: do address Neil's comments before checking in, of course

Neil: So editor being null in Composer means that we currently shouldn't put this attribute on those editors?
Comment 51 Ted Mielczarek [:ted.mielczarek] 2005-12-17 21:25:43 PST
Created attachment 206224 [details] [diff] [review]
Toolkit patch

Updated per Neil's comments.  Also cleans up some fallout from bug 302050 which resulted in two constructor and destructor elements in the textbox binding.
Comment 52 Ted Mielczarek [:ted.mielczarek] 2005-12-17 21:30:42 PST
Created attachment 206225 [details] [diff] [review]
xpfe patch

For pedantry's sake, I removed that trailing space from the one line in the xpfe patch.
Comment 53 Ted Mielczarek [:ted.mielczarek] 2005-12-20 12:21:09 PST
Checked in by timeless.
Comment 54 Alex Vincent [:WeirdAl] 2005-12-22 13:46:58 PST
Hm, we could've trimmed a couple lines of code from these patches if I'd implemented the editor property of textbox.xml in bug 312867.  Sorry I didn't notice.
Comment 55 Darin Fisher 2006-01-20 13:26:39 PST
My debug build of Firefox on Windows throws an exception on this bit of code:

+                this.inputField.QueryInterface(nsIDOMNSEditableElement)
+                    .editor.QueryInterface(nsIPlaintextEditor)
+                    .newlineHandling = nsIPlaintextEditor[x];

It seems that the QI to nsIPlaintextEditor fails, and as a result the XBL binding is not properly initialized.  This causes the URL bar to be blank on startup, and after link clicks, etc.

The exception:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://global/content/bindings/textbox.xml ::  :: line 126"  data: no]

My tree seems to be pretty clean, so I don't know yet how to explain this.  Is anyone else seeing this?
Comment 56 Darin Fisher 2006-01-20 13:40:43 PST
> My debug build of Firefox on Windows throws an exception on this bit of code:

False alarm.  I believe this was due to a depend build problem on Windows :-(
Comment 57 James Ross 2006-01-20 17:23:23 PST
I'm still getting that error here and have a blank URLbar with a clean debug build (but not with a release build). Bug 321238 was the original one for the issue, although the asserts seem to have gone now, but the JS exception you mentioned is still occuring here.

And if Neil mentioned build dependancy problems I will hurt him. This is a clean build ffs.
Comment 58 Ted Mielczarek [:ted.mielczarek] 2007-09-06 16:48:36 PDT
Created attachment 279994 [details] [diff] [review]
mochichrome test

Better late than never!
Comment 59 Ted Mielczarek [:ted.mielczarek] 2007-09-15 08:29:17 PDT
Comment on attachment 279994 [details] [diff] [review]
mochichrome test

Not sure if tests need approval, but asking anyway.
Comment 60 Ted Mielczarek [:ted.mielczarek] 2007-09-18 16:14:54 PDT
Comment on attachment 279994 [details] [diff] [review]
mochichrome test

mconnor said tests don't need approval.  Checked in.

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