Hmm, our splitting code is really buggy. The point which split related methods takes as argument means the point of the last child/character of newly created left node. E.g., specifying, 0, the only the first child of existing right node is moved to the new left node. However, a lot of callers may set (aNode - aNode-Length()) as the point. This is bugs of them. However, EditorBase::SplitNodeImpl() ignores odd offset with this hack: https://searchfox.org/mozilla-central/rev/7fb4cc447c06f14fe3b5c6b0c9d103a860937250/editor/libeditor/EditorBase.cpp#2939,2941-2942,2964-2966,2973-2974 > for (int32_t i = aOffset - 1; i >= 0; i--) { > nsCOMPtr<nsIContent> childNode = childNodes->Item(i); > if (childNode) { > aExistingRightNode.RemoveChild(*childNode, rv); > if (!rv.Failed()) { > nsCOMPtr<nsIContent> firstChild = aNewLeftNode.GetFirstChild(); > aNewLeftNode.InsertBefore(*childNode, firstChild, rv); > } > } > if (rv.Failed()) { > break; > } This is same as: > for (int32_t i = aOffset - 1; i >= 0; i--) { > nsCOMPtr<nsIContent> childNode = childNodes->Item(i); > if (!childNode) { > continue; > } > aExistingRightNode.RemoveChild(*childNode, rv); > if (!rv.Failed()) { > break; > } > nsCOMPtr<nsIContent> firstChild = aNewLeftNode.GetFirstChild(); > aNewLeftNode.InsertBefore(*childNode, firstChild, rv); > if (rv.Failed()) { > break; > } So, just ignoring invalid offset at moving children...

Er, I misunderstood. According to the code moving children, given offset is start of the right node. Okay, this is odd case: > // Split the children between the two nodes. At this point, > // aExistingRightNode has all the children. Move all the children whose > // index is < aOffset to aNewLeftNode. > if (aOffset < 0) { > // This means move no children > return NS_OK; > }

Comment on attachment 8929922 [details] Bug 1413181 - part 1: Redesign EditorBase::SplitNodeImpl() with EditorDOMPoint https://reviewboard.mozilla.org/r/200948/#review206652

Comment on attachment 8929923 [details] Bug 1413181 - part 2: SplitNodeTransaction should store start of existing right node with RangeBoundary https://reviewboard.mozilla.org/r/200950/#review206654 ::: editor/libeditor/SplitNodeTransaction.cpp:66 (Diff revision 1) > } > if (NS_WARN_IF(!clone)) { > return NS_ERROR_UNEXPECTED; > } > mNewLeftNode = dont_AddRef(clone.forget().take()->AsContent()); > - mEditorBase->MarkNodeDirty(mExistingRightNode->AsDOMNode()); > + mEditorBase->MarkNodeDirty(mStartOfRightNode.Container()->AsDOMNode()); I will file a bug to create nsINode version of MarkNodeDirty since all caller can has nsINode. ::: editor/libeditor/SplitNodeTransaction.cpp:135 (Diff revision 1) > } > > - ErrorResult rv; > // First, massage the existing node so it is in its post-split state > - if (mExistingRightNode->GetAsText()) { > - rv = mExistingRightNode->GetAsText()->DeleteData(0, mOffset); > + Text* rightNodeAsText = mStartOfRightNode.Container()->GetAsText(); > + if (rightNodeAsText) { rightNodeAtText is used on true block only, so I think that the following is better. if (mStartOfRightNode.Container()->IsNodeOfType(nsINode::eTEXT)) { Text* rightNodeAsText = mStartOfRightNode.Container()->GetAsText();

Comment on attachment 8929924 [details] Bug 1413181 - part 3: EditorBase::CreateTxnForSplitNode() and EditorBase::SplitNode() should take EditorRawDOMPoint to specify the start of right node https://reviewboard.mozilla.org/r/200952/#review206656 ::: editor/libeditor/EditorBase.cpp:1541 (Diff revision 1) > - int32_t aOffset, > + ErrorResult& aError) > - ErrorResult& aResult) > { > + if (NS_WARN_IF(!aStartOfRightNode.IsSet()) || > + NS_WARN_IF(!aStartOfRightNode.Container()->IsContent())) { > + return nullptr; Why doesn't aError.Throw call? We should set error status.

Comment on attachment 8929925 [details] Bug 1413181 - part 4: Redesign nsIEditActionListener::DidSplitNode() https://reviewboard.mozilla.org/r/200954/#review206670

Comment on attachment 8929926 [details] Bug 1413181 - part 5: HTMLEditRules::SplitParagraph() should take EditorRawDOMPoint instead of a set of container node and offset https://reviewboard.mozilla.org/r/200956/#review206674

Comment on attachment 8929927 [details] Bug 1413181 - part 6: Rename mozilla::EditorBase::EmptyContainers enum class to mozilla::SplitAtEdges for making its values clearer https://reviewboard.mozilla.org/r/200958/#review206692

Comment on attachment 8929928 [details] Bug 1413181 - part 7: EditorBase::SplitNodeDeep() should stop splitting orphan node if it meets an orphan node before meeting the most ancestor node to be split https://reviewboard.mozilla.org/r/200960/#review206700

Comment on attachment 8929929 [details] Bug 1413181 - part 8: Merge two if blocks in the loop of EditorBase::SplitNodeDeep() https://reviewboard.mozilla.org/r/200962/#review206706

Comment on attachment 8929930 [details] Bug 1413181 - part 9: Make EditorBase::SplitNodeDeep() use EditorDOMPoint in the loop https://reviewboard.mozilla.org/r/200964/#review206714

Comment on attachment 8929931 [details] Bug 1413181 - part 10: Redesign EditorBase::SplitNodeDeep() https://reviewboard.mozilla.org/r/200966/#review206734

Comment on attachment 8929932 [details] Bug 1413181 - part 11: Create AutoEditorDOMPointOffsetInvalidator stack class for automatically invalidate offset of EditorDOMPoint https://reviewboard.mozilla.org/r/200968/#review206738

Comment on attachment 8929933 [details] Bug 1413181 - part 12: Redesign and rename HTMLEditRules::SplitAsNeeded() https://reviewboard.mozilla.org/r/200970/#review206772

Comment on attachment 8929934 [details] Bug 1413181 - part 13: HTMLEditRules::MaybeSplitAncestorsForInsert() should be able to return a DOM point in text node https://reviewboard.mozilla.org/r/201046/#review206774

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/b1aa0ef250e8 part 1: Redesign EditorBase::SplitNodeImpl() with EditorDOMPoint r=m_kato https://hg.mozilla.org/integration/autoland/rev/ad1488c29218 part 2: SplitNodeTransaction should store start of existing right node with RangeBoundary r=m_kato https://hg.mozilla.org/integration/autoland/rev/254338ffb5fa part 3: EditorBase::CreateTxnForSplitNode() and EditorBase::SplitNode() should take EditorRawDOMPoint to specify the start of right node r=m_kato https://hg.mozilla.org/integration/autoland/rev/704c808bbfb0 part 4: Redesign nsIEditActionListener::DidSplitNode() r=m_kato https://hg.mozilla.org/integration/autoland/rev/7cf991358f09 part 5: HTMLEditRules::SplitParagraph() should take EditorRawDOMPoint instead of a set of container node and offset r=m_kato https://hg.mozilla.org/integration/autoland/rev/efc91fe72a26 part 6: Rename mozilla::EditorBase::EmptyContainers enum class to mozilla::SplitAtEdges for making its values clearer r=m_kato https://hg.mozilla.org/integration/autoland/rev/2d0bb2bb96b9 part 7: EditorBase::SplitNodeDeep() should stop splitting orphan node if it meets an orphan node before meeting the most ancestor node to be split r=m_kato https://hg.mozilla.org/integration/autoland/rev/4f14d1405ed6 part 8: Merge two if blocks in the loop of EditorBase::SplitNodeDeep() r=m_kato https://hg.mozilla.org/integration/autoland/rev/d0d42ddd114b part 9: Make EditorBase::SplitNodeDeep() use EditorDOMPoint in the loop r=m_kato https://hg.mozilla.org/integration/autoland/rev/ad09d8d3d8f0 part 10: Redesign EditorBase::SplitNodeDeep() r=m_kato https://hg.mozilla.org/integration/autoland/rev/1053929e8e53 part 11: Create AutoEditorDOMPointOffsetInvalidator stack class for automatically invalidate offset of EditorDOMPoint r=m_kato https://hg.mozilla.org/integration/autoland/rev/27cb72cdc916 part 12: Redesign and rename HTMLEditRules::SplitAsNeeded() r=m_kato https://hg.mozilla.org/integration/autoland/rev/62cd3bf2db56 part 13: HTMLEditRules::MaybeSplitAncestorsForInsert() should be able to return a DOM point in text node r=m_kato

https://hg.mozilla.org/mozilla-central/rev/b1aa0ef250e8 https://hg.mozilla.org/mozilla-central/rev/ad1488c29218 https://hg.mozilla.org/mozilla-central/rev/254338ffb5fa https://hg.mozilla.org/mozilla-central/rev/704c808bbfb0 https://hg.mozilla.org/mozilla-central/rev/7cf991358f09 https://hg.mozilla.org/mozilla-central/rev/efc91fe72a26 https://hg.mozilla.org/mozilla-central/rev/2d0bb2bb96b9 https://hg.mozilla.org/mozilla-central/rev/4f14d1405ed6 https://hg.mozilla.org/mozilla-central/rev/d0d42ddd114b https://hg.mozilla.org/mozilla-central/rev/ad09d8d3d8f0 https://hg.mozilla.org/mozilla-central/rev/1053929e8e53 https://hg.mozilla.org/mozilla-central/rev/27cb72cdc916 https://hg.mozilla.org/mozilla-central/rev/62cd3bf2db56