Closed
Bug 309731
Opened 19 years ago
Closed 9 years ago
js exception when using execcommand inserthtml with an empty string
Categories
(Core :: DOM: Editor, defect)
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)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•19 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•19 years ago
|
Whiteboard: midas
![]() |
||
Comment 3•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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•19 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('').
> 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•19 years ago
|
||
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•19 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... 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•19 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.
![]() |
||
Comment 11•19 years ago
|
||
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•19 years ago
|
||
OK, so implementing the no-args DoCommand() would be fine (with a comment to say when it's called).
![]() |
||
Comment 13•19 years ago
|
||
The only question is whether it's ever called from non-Midas editor code...
Comment 14•19 years ago
|
||
(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 ?
Updated•18 years ago
|
Summary: js exception when using execcommand inserthml with an empty string → js exception when using execcommand inserthtml with an empty string
Updated•18 years ago
|
Flags: blocking1.9a1? → blocking1.9-
![]() |
||
Comment 15•18 years ago
|
||
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
Updated•18 years ago
|
QA Contact: editor
Comment 17•17 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•16 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.
![]() |
||
Comment 19•16 years ago
|
||
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•10 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•9 years ago
|
||
Assignee: nobody → michael
Attachment #8605544 -
Flags: review?(ehsan)
Comment 22•9 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•9 years ago
|
||
Fix up some tests & style improvements
Attachment #8605544 -
Attachment is obsolete: true
Comment 24•9 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 | ||
Comment 25•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=268f3c15da0a
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/559f6b949a0a
Keywords: checkin-needed
Comment 27•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/559f6b949a0a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 28•8 years ago
|
||
The same problem affects insertText
You need to log in
before you can comment on or make changes to this bug.
Description
•