Closed Bug 1130651 Opened 5 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Editor, defect)

16 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mikol, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
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
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: 3 years ago
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.