The default bug view has changed. See this FAQ.

xul:textbox attribute to specify what happens to pasted line breaks

RESOLVED FIXED

Status

()

Core
Editor
--
enhancement
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: ted)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 20 obsolete attachments)

778 bytes, application/vnd.mozilla.xul+xml
Details
10.97 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
2.71 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.89 KB, patch
ted
: review+
Details | Diff | Splinter Review
3.60 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

13 years ago
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?)
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 2

12 years ago
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
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Assignee: mozeditor → ted.mielczarek
Status: ASSIGNED → NEW
(Assignee)

Comment 3

12 years ago
Created attachment 196255 [details]
Testcase

Testcase for the patch I'm going to attach next.
(Assignee)

Comment 4

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

Comment 5

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

Updated

12 years ago
Blocks: 264568
(Assignee)

Comment 6

12 years ago
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.
Attachment #196258 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #196274 - Flags: review?(brade)
(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.
(Assignee)

Comment 8

12 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #196255 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 9

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

Comment 10

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

(Assignee)

Comment 11

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

Comment 12

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

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

12 years ago
Comment on attachment 196274 [details] [diff] [review]
Updated editor changes

(remove review request since there will be changes)
Attachment #196274 - Flags: review?(brade)
(Assignee)

Comment 15

12 years ago
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.
Attachment #196274 - Attachment is obsolete: true
Attachment #196489 - Flags: review?(brade)
(Assignee)

Comment 16

12 years ago
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.
Attachment #196328 - Attachment is obsolete: true

Comment 17

12 years ago
Comment on attachment 196489 [details] [diff] [review]
editor patch with review comments

r=brade
Attachment #196489 - Flags: review?(brade) → review+

Comment 18

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

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

Comment 20

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

Comment 21

12 years ago
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.
Attachment #196489 - Attachment is obsolete: true
Attachment #201743 - Flags: superreview?
Attachment #201743 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #201743 - Flags: superreview? → superreview?(jag)
(Assignee)

Comment 22

12 years ago
Created attachment 201747 [details] [diff] [review]
editor patch without that misplaced comma

That'll teach me not to build before I submit a patch.
Attachment #201743 - Attachment is obsolete: true
Attachment #201747 - Flags: superreview?(jag)
Attachment #201747 - Flags: review+
Attachment #201743 - Flags: superreview?(jag)
(Assignee)

Comment 23

12 years ago
Created attachment 201754 [details] [diff] [review]
updated binding patch

Apparently on trunk you need to QI the input field to get at its editor.
Attachment #196490 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #201754 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 24

12 years ago
Created attachment 201758 [details] [diff] [review]
equivalent XPFE binding patch

Same thing for XPFE.
Attachment #201758 - Flags: review?(neil.parkwaycc.co.uk)

Comment 25

12 years ago
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.
Attachment #201747 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 26

12 years ago
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.
Attachment #201747 - Attachment is obsolete: true
Attachment #201792 - Flags: superreview+
Attachment #201792 - Flags: review+

Comment 27

12 years ago
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];
(Assignee)

Comment 28

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

12 years ago
Add a lookup table that maps the lowercase strings to the enums.
(Assignee)

Comment 30

12 years ago
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?
Attachment #201792 - Attachment is obsolete: true
Attachment #202039 - Flags: superreview?(jag)
Attachment #202039 - Flags: review+
(Assignee)

Comment 31

12 years ago
Created attachment 202040 [details] [diff] [review]
reworked toolkit binding patch

Reworked to be more spiffy
Attachment #201754 - Attachment is obsolete: true
Attachment #202040 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #201754 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

12 years ago
Attachment #201758 - Attachment is obsolete: true
Attachment #201758 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 32

12 years ago
Created attachment 202041 [details]
updated testcase (requires chrome privs)
Attachment #196330 - Attachment is obsolete: true

Comment 33

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

Updated

12 years ago
Attachment #202039 - Flags: superreview?(jag) → superreview+

Comment 34

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

Comment 35

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

Comment 36

12 years ago
Created attachment 202621 [details] [diff] [review]
toolkit patch

Done and done
Attachment #202040 - Attachment is obsolete: true
Attachment #202621 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #202040 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 37

12 years ago
Created attachment 202622 [details] [diff] [review]
xpfe patch

Same thing, xpfe
Attachment #202622 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 38

12 years ago
Created attachment 203133 [details] [diff] [review]
toolkit patch

Changed to use RegExp.test
Attachment #202621 - Attachment is obsolete: true
Attachment #203133 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #202621 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 39

12 years ago
Created attachment 203134 [details] [diff] [review]
xpfe patch

same for xpfe
Attachment #202622 - Attachment is obsolete: true
Attachment #203134 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #202622 - Flags: review?(neil.parkwaycc.co.uk)

Comment 40

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

Comment 41

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

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

Comment 43

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


(Assignee)

Updated

11 years ago
Depends on: 319882
(Assignee)

Comment 44

11 years ago
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.
Attachment #202039 - Attachment is obsolete: true
Attachment #205562 - Flags: superreview+
Attachment #205562 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 45

11 years ago
Created attachment 205563 [details] [diff] [review]
Updated toolkit patch

Updating toolkit patch due to changes in bug 302050
Attachment #203133 - Attachment is obsolete: true
Attachment #205563 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #203133 - Flags: review?(neil.parkwaycc.co.uk)

Comment 46

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

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

Updated

11 years ago
Attachment #203134 - Flags: review?(neil.parkwaycc.co.uk) → review+

Comment 48

11 years ago
Comment on attachment 205562 [details] [diff] [review]
editor patch

r=me on the changes from attachment 202039 [details] [diff] [review].
Attachment #205562 - Flags: review?(neil.parkwaycc.co.uk) → review+

Comment 49

11 years ago
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 ;-)
Attachment #205563 - Flags: review?(neil.parkwaycc.co.uk) → review+

Updated

11 years ago
Attachment #205563 - Flags: superreview+

Comment 50

11 years ago
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?
(Assignee)

Comment 51

11 years ago
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.
Attachment #205563 - Attachment is obsolete: true
Attachment #206224 - Flags: superreview+
Attachment #206224 - Flags: review+
(Assignee)

Comment 52

11 years ago
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.
Attachment #203134 - Attachment is obsolete: true
Attachment #206225 - Flags: superreview+
Attachment #206225 - Flags: review+
(Assignee)

Comment 53

11 years ago
Checked in by timeless.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 321238
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

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

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

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

Updated

11 years ago
Blocks: 329668
(Assignee)

Comment 58

10 years ago
Created attachment 279994 [details] [diff] [review]
mochichrome test

Better late than never!
Attachment #279994 - Flags: review?(enndeakin)

Updated

10 years ago
Attachment #279994 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 59

10 years ago
Comment on attachment 279994 [details] [diff] [review]
mochichrome test

Not sure if tests need approval, but asking anyway.
Attachment #279994 - Flags: approval1.9?
(Assignee)

Comment 60

10 years ago
Comment on attachment 279994 [details] [diff] [review]
mochichrome test

mconnor said tests don't need approval.  Checked in.
Attachment #279994 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Flags: in-testsuite+
Blocks: 483385
You need to log in before you can comment on or make changes to this bug.