Closed Bug 1130651 Opened 10 years ago Closed 8 years ago

`document.execCommand('insertText', false, '\n')` with `contentEditable` element throws `IndexSizeError`.

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
16 Branch
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mikol, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

index-size-error.html
2.41 KB, text/html
Details
Bug 1130651 - Don't throw IndexSizeError from insertText command
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Attached file index-size-error.html —
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.94 Safari/537.36 Steps to reproduce: 1. Open index-size-error.html. 2. Type something. For example: “This is a test.” 3. Position the text caret in the middle of the string you typed after a space, immediately before a character. For example: “This is |a test.” 4. Press the Return/Enter key to insert a line break. Actual results: The following error will be thrown: “IndexSizeError: Index or size is negative or greater than the allowed amount”. The operation that causes the error (`document.execCommand('insertText', false, '\n')`) actually succeeds so it’s not clear why it throws an error after it succeeds but before it returns. Expected results: No error should have been thrown.
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=02566635767f&tochange=1533eef79502 Suspect: Bug 1088054 and Bug 1086349
Blocks: 1088054, 1086349
Status: UNCONFIRMED → NEW
status-firefox35: --- → unaffected
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox-esr31: --- → unaffected
Component: Untriaged → Editor
Ever confirmed: true
Flags: needinfo?(ayg)
Keywords: regression
OS: Mac OS X → All
Product: Firefox → Core
Version: 33 Branch → 36 Branch
wait Please ignote comment #1
No longer blocks: 1086349, 1088054
status-firefox35: unaffected → affected
Flags: needinfo?(ayg)
Keywords: regression
Version: 36 Branch → 33 Branch
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bcb614c5e7fa&tochange=cd62c4b8f500 The error is thrown since landing Bug 748307. So, this is not a regression.
Blocks: 748307
Version: 33 Branch → 16 Branch
It doesn't entirely surprise me that this is broken, because it's an edge case. The correct behavior should be identical to document.execCommand('insertparagraph'), I think, but it would be good to check against WebKit/Blink. Tests should include inside a block (like a <p> or list item) to make sure we're doing insertparagraph and not insertlinebreak. And obviously, it shouldn't throw.
also, an empty string with insertText throws NS_ERROR_NOT_IMPLEMENTED Same problem affected inserthtml Bug 309731
More minimal test-case: data:text/html,<!doctype html> <div contenteditable>a b</div> <script> var div = document.querySelector("div"); div.focus(); getSelection().collapse(div.firstChild, 2); try { document.execCommand("inserttext", false, "\n"); document.body.textContent = "success"; } catch(e) { document.body.textContent = e; } </script>
Assignee: nobody → ayg
Status: NEW → ASSIGNED
I opened bug 1352144 for the empty-string issue.
Attachment #8853012 - Flags: review?(masayuki)
Comment on attachment 8853012 [details] Bug 1130651 - Don't throw IndexSizeError from insertText command https://reviewboard.mozilla.org/r/125142/#review128358 ::: editor/libeditor/HTMLEditRules.cpp:1417 (Diff revision 1) > else if (subStr.Equals(newlineStr)) { > + NS_ENSURE_STATE(mHTMLEditor); > + AutoTrackDOMPoint tracker(mHTMLEditor->mRangeUpdater, > + address_of(selNode), &selOffset); > nsCOMPtr<Element> br = wsObj.InsertBreak(address_of(curNode), > &curOffset, > nsIEditor::eNone); I don't understand why does HTMLEditRules::WillInsertText() need to track selection range during an insertion only in this case. What's the difference from the other cases (especially, from |if (isPRE || IsPlaintextEditor())| cases)? Could you explain the reason in the commit message?
Comment on attachment 8853012 [details] Bug 1130651 - Don't throw IndexSizeError from insertText command https://reviewboard.mozilla.org/r/125142/#review128358 > I don't understand why does HTMLEditRules::WillInsertText() need to track selection range during an insertion only in this case. What's the difference from the other cases (especially, from |if (isPRE || IsPlaintextEditor())| cases)? > > Could you explain the reason in the commit message? Yes, you're right, my fix should have been more general.
Comment on attachment 8853012 [details] Bug 1130651 - Don't throw IndexSizeError from insertText command https://reviewboard.mozilla.org/r/125142/#review128506
Attachment #8853012 - Flags: review?(masayuki) → review+
Pushed by ayg@aryeh.name: https://hg.mozilla.org/integration/autoland/rev/0aeb3c980745 Don't throw IndexSizeError from insertText command r=masayuki
Oops, I got this confused with bug 1352144 and landed a fixed version. If you're not happy with the fixed version, please back out and let me know what to change. (You gave r+, but it seems like you just wanted me to explain my reason and not change the code, and I changed the code instead.)
Flags: needinfo?(masayuki)
https://hg.mozilla.org/mozilla-central/rev/0aeb3c980745
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
No problem, the landed patch is what I gave r+.
Flags: needinfo?(masayuki)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: