From 45ef18ee30747cfae446a007a12b61b33b6b547d Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Thu, 12 Sep 2019 06:51:26 +0000 Subject: [PATCH] Bug 1574852 - part 100: Get rid of `TextEditRules::DidDeleteSelection()` r=m_kato There are only 3 callers and it does simple but different 2 things. One of the callers is `HTMLEditRules::DidDeleteSelection()` so that if same things are done by `TextEditor::DeleteSelectionAsSubAction()`, it does not need to duplicate the code. Therefore, we need to duplicate the code into `TextEditor::DeleteSelectionAsSubAction()` and `TextEditRules::WillSetText()`. Then, `TextEditRules::WillSetText()` can avoid accessing `Selection` since it still grabs the modified text node. Note that only when it's called by `TextEditRules::DidDoAction()`, `AutoTransactionsConserveSelection` has been set. However, neither `DeleteNodeWithTransaction()` nor `DeleteNodeTransaction::DoTransaction()` changes `Selection`. Therefore, it hasn't do anything. So, we can remove it right now. Differential Revision: https://phabricator.services.mozilla.com/D45294 --HG-- extra : moz-landing-system : lando --- editor/libeditor/HTMLEditRules.cpp | 7 +--- editor/libeditor/TextEditRules.cpp | 65 ++++++++---------------------- editor/libeditor/TextEditRules.h | 7 ---- editor/libeditor/TextEditor.cpp | 50 +++++++++++++++++++---- 4 files changed, 59 insertions(+), 70 deletions(-) diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index 38ea245fc4d2..ddd21723205f 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -4109,12 +4109,7 @@ nsresult HTMLEditRules::DidDeleteSelection() { } } } - - // call through to base class - nsresult rv = TextEditRules::DidDeleteSelection(); - NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), - "TextEditRules::DidDeleteSelection() failed"); - return rv; + return NS_OK; } EditActionResult HTMLEditor::MakeOrChangeListAndListItemAsSubAction( diff --git a/editor/libeditor/TextEditRules.cpp b/editor/libeditor/TextEditRules.cpp index 840e475fac5e..9f344c83f2c5 100644 --- a/editor/libeditor/TextEditRules.cpp +++ b/editor/libeditor/TextEditRules.cpp @@ -259,16 +259,10 @@ nsresult TextEditRules::DidDoAction(EditSubActionInfo& aInfo, return NS_ERROR_EDITOR_DESTROYED; } - AutoSafeEditorData setData(*this, *mTextEditor); - - // don't let any txns in here move the selection around behind our back. - // Note that this won't prevent explicit selection setting from working. - AutoTransactionsConserveSelection dontChangeMySelection(TextEditorRef()); - switch (aInfo.mEditSubAction) { case EditSubAction::eDeleteSelectedContent: MOZ_ASSERT(!mIsHTMLEditRules); - return DidDeleteSelection(); + return NS_OK; case EditSubAction::eInsertElement: case EditSubAction::eUndo: case EditSubAction::eRedo: @@ -869,11 +863,21 @@ nsresult TextEditRules::WillSetText(bool* aCancel, bool* aHandled, // If we replaced non-empty value with empty string, we need to delete the // text node. - if (tString.IsEmpty()) { - DebugOnly rvIgnored = DidDeleteSelection(); - MOZ_ASSERT(rvIgnored != NS_ERROR_EDITOR_DESTROYED); - NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored), - "DidDeleteSelection() failed"); + if (tString.IsEmpty() && !textNode->Length()) { + nsresult rv = + MOZ_KnownLive(TextEditorRef()).DeleteNodeWithTransaction(*textNode); + if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) { + return NS_ERROR_EDITOR_DESTROYED; + } + NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), + "DeleteNodeWithTransaction() failed, but ignored"); + // XXX I don't think this is necessary because the anonymous `
` + // element has now only padding `
` element even if there are + // something. + IgnoredErrorResult ignoredError; + SelectionRefPtr()->SetInterlinePosition(true, ignoredError); + NS_WARNING_ASSERTION(!ignoredError.Failed(), + "Selection::SetInterlinePoisition() failed"); } *aHandled = true; @@ -998,43 +1002,6 @@ nsresult TextEditRules::DeleteSelectionWithTransaction( return NS_OK; } -nsresult TextEditRules::DidDeleteSelection() { - MOZ_ASSERT(IsEditorDataAvailable()); - - EditorDOMPoint selectionStartPoint( - EditorBase::GetStartPoint(*SelectionRefPtr())); - if (NS_WARN_IF(!selectionStartPoint.IsSet())) { - return NS_ERROR_FAILURE; - } - - // Delete empty text nodes at selection. - if (selectionStartPoint.IsInTextNode() && - !selectionStartPoint.GetContainer()->Length()) { - nsresult rv = MOZ_KnownLive(TextEditorRef()) - .DeleteNodeWithTransaction( - MOZ_KnownLive(*selectionStartPoint.GetContainer())); - if (NS_WARN_IF(!CanHandleEditAction())) { - return NS_ERROR_EDITOR_DESTROYED; - } - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - } - - // Note that this may be true only when this is HTMLEditRules. - if (TextEditorRef() - .TopLevelEditSubActionDataRef() - .mDidExplicitlySetInterLine) { - return NS_OK; - } - // We prevent the caret from sticking on the left of prior BR - // (i.e. the end of previous line) after this deletion. Bug 92124 - ErrorResult err; - SelectionRefPtr()->SetInterlinePosition(true, err); - NS_WARNING_ASSERTION(!err.Failed(), "Failed to set interline position"); - return err.StealNSResult(); -} - nsresult TextEditRules::WillOutputText(const nsAString* aOutputFormat, nsAString* aOutString, uint32_t aFlags, bool* aCancel, bool* aHandled) { diff --git a/editor/libeditor/TextEditRules.h b/editor/libeditor/TextEditRules.h index 3765cd38c011..575b5e339636 100644 --- a/editor/libeditor/TextEditRules.h +++ b/editor/libeditor/TextEditRules.h @@ -207,13 +207,6 @@ class TextEditRules { MOZ_MUST_USE nsresult DeleteSelectionWithTransaction( nsIEditor::EDirection aCollapsedAction, bool* aCancel, bool* aHandled); - /** - * Called after deleted selected content. - * This method may remove empty text node and makes guarantee that caret - * is never at left of
element. - */ - MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult DidDeleteSelection(); - nsresult WillSetTextProperty(bool* aCancel, bool* aHandled); nsresult WillRemoveTextProperty(bool* aCancel, bool* aHandled); diff --git a/editor/libeditor/TextEditor.cpp b/editor/libeditor/TextEditor.cpp index 8d23833a49d6..7d05549579f7 100644 --- a/editor/libeditor/TextEditor.cpp +++ b/editor/libeditor/TextEditor.cpp @@ -661,19 +661,53 @@ nsresult TextEditor::DeleteSelectionAsSubAction(EDirection aDirection, subActionInfo.stripWrappers = aStripWrappers; bool cancel, handled; nsresult rv = rules->WillDoAction(subActionInfo, &cancel, &handled); + if (NS_WARN_IF(NS_FAILED(rv)) || cancel) { + return rv; + } + if (!handled) { + rv = DeleteSelectionWithTransaction(aDirection, aStripWrappers); + } + // post-process + rv = rules->DidDoAction(subActionInfo, rv); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } - if (!cancel && !handled) { - rv = DeleteSelectionWithTransaction(aDirection, aStripWrappers); + + // XXX This is odd. We just tries to remove empty text node here but we + // refer `Selection`. It may be modified by mutation event listeners + // so that we should remove the empty text node when we make it empty. + EditorDOMPoint atNewStartOfSelection( + EditorBase::GetStartPoint(*SelectionRefPtr())); + if (NS_WARN_IF(!atNewStartOfSelection.IsSet())) { + // XXX And also it seems that we don't need to return error here. + // Why don't we just ignore? `Selection::RemoveAllRanges()` may + // have been called by mutation event listeners. + return NS_ERROR_FAILURE; } - if (!cancel) { - // post-process - rv = rules->DidDoAction(subActionInfo, rv); - NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), - "TextEditRules::DidDoAction() failed"); + if (atNewStartOfSelection.IsInTextNode() && + !atNewStartOfSelection.GetContainer()->Length()) { + nsresult rv = DeleteNodeWithTransaction( + MOZ_KnownLive(*atNewStartOfSelection.GetContainer())); + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; + } } - return rv; + + // XXX I don't think that this is necessary in anonymous `
` element of + // TextEditor since there should be at most one text node and at most + // one padding `
` element so that `
` element won't be before + // caret. + if (!TopLevelEditSubActionDataRef().mDidExplicitlySetInterLine) { + // We prevent the caret from sticking on the left of previous `
` + // element (i.e. the end of previous line) after this deletion. Bug 92124. + ErrorResult error; + SelectionRefPtr()->SetInterlinePosition(true, error); + if (NS_WARN_IF(error.Failed())) { + return error.StealNSResult(); + } + } + + return NS_OK; } nsresult TextEditor::DeleteSelectionWithTransaction(