Closed Bug 309731 Opened 15 years ago Closed 5 years ago

js exception when using execcommand inserthtml with an empty string

Categories

(Core :: DOM: Editor, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: martijn.martijn, Assigned: nika)

References

Details

(Keywords: helpwanted, testcase, Whiteboard: midas)

Attachments

(3 files, 1 obsolete file)

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.
Attached file testcase
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);
  }
Whiteboard: midas
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?
Flags: blocking1.9a1?
Attached file 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.
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.)
(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('').

> 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.
OK, so now all we need to do is get some editor guy to comment on the two approaches in comment 5....  Good luck.  :(
(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...
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.
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
OK, so implementing the no-args DoCommand() would be fine (with a comment to say when it's called).
The only question is whether it's ever called from non-Midas editor code...
(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 ?
Summary: js exception when using execcommand inserthml with an empty string → js exception when using execcommand inserthtml with an empty string
Flags: blocking1.9a1? → blocking1.9-
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...
Keywords: helpwanted
I doubt Joe is gonna do anything here.
Assignee: mozeditor → nobody
QA Contact: editor
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.
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.
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.
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.
Assignee: nobody → michael
Attachment #8605544 - Flags: review?(ehsan)
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);
Attachment #8605544 - Flags: review?(ehsan) → review+
Fix up some tests & style improvements
Attachment #8605544 - Attachment is obsolete: true
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>.
https://hg.mozilla.org/mozilla-central/rev/559f6b949a0a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
The same problem affects insertText
You need to log in before you can comment on or make changes to this bug.