309731 - js exception when using execcommand inserthtml with an empty string

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

The testcase that i'll attach gives a js error when clicking on the button:
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMNSHTMLDocument.execCommand]" 
nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame ::
file:///C:/Documents%20and%20Settings/Michel%20Wissink/Bureaublad/bug_241191.htm
:: doe1 :: line 18"  data: no]

I don't think that should happen when the argument is an empty string for
inserthtml.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Niels]

If an empty string is passed, inserthtml should behave like the delete command, i.e. a selection should be deleted, otherwise nothing should happen. It should NOT through an error message, as confirmed with SeaMonkey 1.0 and Firefox 1.5.0.1.

This is a workaround for this annoying bug:

  if (text != '') {
    frameDocument.execCommand('inserthtml', false, text);
  }
  else {
    frameDocument.execCommand('delete', false, text);
  }

Comment 3

[Boris Zbarsky [:bzbarsky]]

So as I see it, there are two options here:

1)  Implement nsInsertHTMLCommand::DoCommand (the no-args version) to do
    something weird.
2)  Make ExecCommand map 'inserthtml' to 'delete' if the arg is empty (if that's
    what IE does).

Martijn, what does IE do?

Comment 4

[Martijn Wargers (dead)]

Attachment: testcase2

IE6 doesn't use execcommand for this, pasteHTML has to be used:
document.selection.createRange().pasteHTML('');
This is equivalent to Mozilla's inserthtml and it accepts an empty string.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I guess my question is where the "inserthtml should behave like the delete command" in comment 2 came from...  Could we just make inserthtml with empty string be a no-op, for example?  (I'm not saying we should, but I really don't know much about the design of this designMode stuff and how it _should_ behave... and I'm not sure anyone does, offhand.)

Comment 6

[Martijn Wargers (dead)]

(In reply to comment #5)
> I guess my question is where the "inserthtml should behave like the delete
> command" in comment 2 came from...  
Because that's what IE6 is doing with pasteHTML('').

Comment 7

[Niels]

> I guess my question is where the "inserthtml should behave like the delete
> command" in comment 2 came from...  

Come on, inserthtml replaces the selected text with a new string. It would be absolutely counterintuitive if an empty string would be handled differently from any other string.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, so now all we need to do is get some editor guy to comment on the two approaches in comment 5....  Good luck.  :(

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #5)
> I guess my question is where the "inserthtml should behave like the delete
> command" in comment 2 came from...  Could we just make inserthtml with empty
> string be a no-op, for example?  (I'm not saying we should, but I really don't
> know much about the design of this designMode stuff and how it _should_
> behave... and I'm not sure anyone does, offhand.)

inserthtml with empty string cannot be no-op. It's supposed to delete the
existing selection. I am 100% with Niels here.

brade, do you confirm ?

(In reply to comment #8)
> OK, so now all we need to do is get some editor guy to comment on the two
> approaches in comment 5....  Good luck.  :(

Tssssk, tsssk...

Comment 10

[Simon Fraser [no longer active]]

My guess is that just removing this one line:
http://lxr.mozilla.org/mozilla/source/editor/composer/src/nsComposerCommands.cpp#1471
would do the trick.

Comment 11

[Boris Zbarsky [:bzbarsky]]

It wouldn't.  We're calling nsInsertHTMLCommand::DoCommand, not nsInsertHTMLCommand::DoCommandParams because of http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.667&mark=4002-4005#4002

Comment 12

[Simon Fraser [no longer active]]

OK, so implementing the no-args DoCommand() would be fine (with a comment to say when it's called).

Comment 13

[Boris Zbarsky [:bzbarsky]]

The only question is whether it's ever called from non-Midas editor code...

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #13)
> The only question is whether it's ever called from non-Midas editor code...

It's not the only question. Another question is : you plan to introduce a
behavioural difference between the inserthtml command and
nsHTMLEditor::insertHTML. Is that good or bad ?

Comment 15

[Boris Zbarsky [:bzbarsky]]

This isn't going to go anywhere until an editor peer actually makes a decision.  I don't have time anymore to lug editor bugs forward...

Comment 16

[Simon Fraser [no longer active]]

I doubt Joe is gonna do anything here.

Comment 17

[Kathleen :Brade]

Going through some old bug mail...

regarding comment 14:
I think it's ok to introduce a behavioural difference (if there really is one).  Perhaps nsHTMLEditor::InsertHTML should be renamed to make any subtleties clearer.

regarding comment 13:
At this point, there are two many users of the code so I would assume that someone may be using the command.  However, I think it's ok to change this behavior because it *is* a bug.  It wasn't caught earlier because it is an odd scenario (but reasonable for users to run into it).  Probably most existing callers of this command are working around the bug.  I doubt many people are relying on this broken aspect of the command.

Comment 18

[Cacycle]

I can verify this bug under Firefox 3.0.1 and I think it should be fixed. I had to add the above workaround for this bug to my program.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I tried mailing you personally, Cacycle, but since you choose to block all email, this will have to be public:

Of course it's still there (bug is not marked resolved, is it?) and of 
course it should be fixed (bug is not marked invalid, is it?).

I'd suggest reading 
<https://bugzilla.mozilla.org/page.cgi?id=etiquette.html>, especially 
the part about "me too" comments.

Comment 20

[Cacycle]

The bug is still around six years later and causes problems under Firefox 31.0. Adding an empty string via inserthtml works fine in Chrome without any error message.

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Attachment: Allow document.execCommand('inserthtml') with an empty string parameter

Comment 22

[(no longer active)]

Comment on attachment 8605544 [details] [diff] [review]
Allow document.execCommand('inserthtml') with an empty string parameter

Review of attachment 8605544 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/composer/nsComposerCommands.cpp
@@ +1318,5 @@
> +  nsCOMPtr<nsIHTMLEditor> editor = do_QueryInterface(refCon);
> +  NS_ENSURE_TRUE(editor, NS_ERROR_NOT_IMPLEMENTED);
> +
> +  nsString html = EmptyString();
> +  return editor->InsertHTML(html);

Nit: Please just pass EmptyString() directly to InsertHTML.

::: editor/libeditor/nsHTMLDataTransfer.cpp
@@ +303,4 @@
>                             streamEndParentNode, streamEndOffset);
>  
>    if (nodeList.Length() == 0) {
> +    // we aren't inserting anything, but if aDeleteSelection is set, we do want to delete everything

Nit: Please break this line at 80 chars.  Also, it would be nice if you formatted comments as properly capitalized sentences ending in periods, etc.  :-)

::: editor/libeditor/tests/mochitest.ini
@@ +162,4 @@
>  skip-if = toolkit == 'android'
>  [test_bug1068979.html]
>  [test_bug1109465.html]
> +[test_bug309731.html]
\ No newline at end of file

Nit: Please move this up to preserve alphabetical order.

::: editor/libeditor/tests/test_bug309731.html
@@ +23,5 @@
> +function selectNode(node) {
> +  var range = document.createRange();
> +  range.selectNodeContents(node);
> +  window.getSelection().removeAllRanges();
> +  window.getSelection().addRange(range);

Nit: This could be written as:

  getSelection().selectAllChildren(node);

@@ +31,5 @@
> +  var range = document.createRange();
> +  range.setStart(node, 0);
> +  range.setEnd(node, 0);
> +  window.getSelection().removeAllRanges();
> +  window.getSelection().addRange(range);

Nit: This could be written as:

  getSelection().collapse(node, 0);

Comment 23

[Nika Layzell [:nika] (ni? for response)]

Attachment: Allow document.execCommand('inserthtml') with an empty string parameter

Fix up some tests & style improvements

Comment 24

[(no longer active)]

Is this ready to land?  If yes, you can include a try server link and ask checkin-needed, as per <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree>.

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=268f3c15da0a

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/559f6b949a0a

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/559f6b949a0a

Comment 28

[Zac Spitzer]

The same problem affects insertText