js exception when using execcommand inserthtml with an empty string

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
14 years ago
2 years ago

People

(Reporter: martijn.martijn, Assigned: Nika)

Tracking

({helpwanted, testcase})

Trunk
mozilla41
x86
Windows 2000
helpwanted, testcase
Points:
---
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: midas)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Posted file testcase

Comment 2

13 years ago
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);
  }

Updated

13 years ago
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?
(Reporter)

Comment 4

13 years ago
Posted 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.)
(Reporter)

Comment 6

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

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

Comment 10

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

Comment 12

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

Updated

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

Comment 16

13 years ago
I doubt Joe is gonna do anything here.
Assignee: mozeditor → nobody
QA Contact: editor

Comment 17

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

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

Comment 20

5 years ago
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)

Comment 21

4 years ago
Assignee: nobody → michael
Attachment #8605544 - Flags: review?(ehsan)

Comment 22

4 years ago
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+
(Assignee)

Comment 23

4 years ago
Fix up some tests & style improvements
Attachment #8605544 - Attachment is obsolete: true

Comment 24

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

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/559f6b949a0a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Comment 28

2 years ago
The same problem affects insertText
You need to log in before you can comment on or make changes to this bug.