From dd8f12dbc35481cd1502163ec39629552be0d7d1 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Thu, 9 Jul 2020 09:44:41 +0000 Subject: [PATCH] Bug 1647556 - part 9-2: Make `WSRunObject::InsertBreak()` stop using `FindNearestFragment()` r=m_kato It removes some invisible leading and/or trailing white-spaces when it inserts `
` element into the invisible white-space sequence. It currently checks whether the insertion point is in invisible leading and trailing white-spaces or not with `FindNearestFragment()`, but we can do same thing with comparing the insertion point with the result of `TextFragmentData::GetInvisibleLeadingWhiteSpaceRange()` and `TextFragmentData::GetInvisibleLeadingWhiteSpaceRange()`. However, current implementation does not make sense because: - It checks trailing white-spaces with `!IsEndOfHardLine()` and `IsStartOfHardLine()`, but this means that it does ignores invisible white-spaces which are the only content in a line. - It checks leading white-spaces with `!IsStartOfHardLine()` and `IsEndOfHardLine()`, so, this also ignores invisible white-spaces which are the only content in a line. - The important thing of the logic is prevent that invisible leading and trailing white-spaces become visible with new `
` element, but this is done only for trailing white-spaces. Differential Revision: https://phabricator.services.mozilla.com/D82283 --- editor/libeditor/EditorDOMPoint.h | 5 ++ editor/libeditor/WSRunObject.cpp | 137 ++++++++++++++++++------------ editor/libeditor/WSRunObject.h | 106 ++++++++++++++++++++++- 3 files changed, 188 insertions(+), 60 deletions(-) diff --git a/editor/libeditor/EditorDOMPoint.h b/editor/libeditor/EditorDOMPoint.h index bf7eb61aa5c7..dc7a06325f90 100644 --- a/editor/libeditor/EditorDOMPoint.h +++ b/editor/libeditor/EditorDOMPoint.h @@ -1014,6 +1014,11 @@ class EditorDOMRangeBase final { return mStart == mEnd; } bool IsPositioned() const { return mStart.IsSet() && mEnd.IsSet(); } + template + bool Contains(const OtherPointType& aPoint) const { + return IsPositioned() && mStart.EqualsOrIsBefore(aPoint) && + mEnd.IsBefore(aPoint); + } bool InSameContainer() const { MOZ_ASSERT(IsPositioned()); return IsPositioned() && mStart.GetContainer() == mEnd.GetContainer(); diff --git a/editor/libeditor/WSRunObject.cpp b/editor/libeditor/WSRunObject.cpp index a83e5e8ec684..c844ea6caa70 100644 --- a/editor/libeditor/WSRunObject.cpp +++ b/editor/libeditor/WSRunObject.cpp @@ -202,12 +202,18 @@ already_AddRefed WSRunObject::InsertBreak( // meanwhile, the pre case is handled in HandleInsertText() in // HTMLEditSubActionHandler.cpp - const WSFragment* beforeRun = FindNearestFragment(aPointToInsert, false); - const WSFragment* afterRun = FindNearestFragment(aPointToInsert, true); - + TextFragmentData textFragmentData(mStart, mEnd, mNBSPData, mPRE); + const EditorDOMRange invisibleLeadingWhiteSpaceRangeOfNewLine = + textFragmentData.GetNewInvisibleLeadingWhiteSpaceRangeIfSplittingAt( + aPointToInsert); + const EditorDOMRange invisibleTrailingWhiteSpaceRangeOfCurrentLine = + textFragmentData.GetNewInvisibleTrailingWhiteSpaceRangeIfSplittingAt( + aPointToInsert); const Maybe visibleWSFragmentInMiddleOfLine = - TextFragmentData(mStart, mEnd, mNBSPData, mPRE) - .CreateWSFragmentForVisibleAndMiddleOfLine(); + !invisibleLeadingWhiteSpaceRangeOfNewLine.IsPositioned() || + !invisibleTrailingWhiteSpaceRangeOfCurrentLine.IsPositioned() + ? textFragmentData.CreateWSFragmentForVisibleAndMiddleOfLine() + : Nothing(); const PointPosition pointPositionWithVisibleWhiteSpaces = visibleWSFragmentInMiddleOfLine.isSome() ? visibleWSFragmentInMiddleOfLine.ref().ComparePoint(aPointToInsert) @@ -219,20 +225,21 @@ already_AddRefed WSRunObject::InsertBreak( // point while we tweak any surrounding white-space AutoTrackDOMPoint tracker(mHTMLEditor.RangeUpdaterRef(), &pointToInsert); - // Handle any changes needed to ws run after inserted br - if (!afterRun || afterRun->IsEndOfHardLine()) { - // Don't need to do anything. Just insert break. ws won't change. - } else if (afterRun->IsStartOfHardLine()) { - // Delete the leading ws that is after insertion point. We don't - // have to (it would still not be significant after br), but it's - // just more aesthetically pleasing to. - nsresult rv = MOZ_KnownLive(mHTMLEditor) - .DeleteTextAndTextNodesWithTransaction( - pointToInsert, afterRun->EndPoint()); - if (NS_FAILED(rv)) { - NS_WARNING( - "HTMLEditor::DeleteTextAndTextNodesWithTransaction() failed"); - return nullptr; + if (invisibleTrailingWhiteSpaceRangeOfCurrentLine.IsPositioned()) { + if (!invisibleTrailingWhiteSpaceRangeOfCurrentLine.Collapsed()) { + // XXX Why don't we remove all of the invisible white-spaces? + MOZ_ASSERT(invisibleTrailingWhiteSpaceRangeOfCurrentLine.StartRef() == + pointToInsert); + nsresult rv = + MOZ_KnownLive(mHTMLEditor) + .DeleteTextAndTextNodesWithTransaction( + invisibleTrailingWhiteSpaceRangeOfCurrentLine.StartRef(), + invisibleTrailingWhiteSpaceRangeOfCurrentLine.EndRef()); + if (NS_FAILED(rv)) { + NS_WARNING( + "HTMLEditor::DeleteTextAndTextNodesWithTransaction() failed"); + return nullptr; + } } } // If new line will start with visible white-spaces, it needs to be start @@ -265,19 +272,24 @@ already_AddRefed WSRunObject::InsertBreak( } } - // Handle any changes needed to ws run before inserted br - if (!beforeRun || beforeRun->IsStartOfHardLine()) { - // Don't need to do anything. Just insert break. ws won't change. - } else if (beforeRun->IsEndOfHardLine()) { - // Need to delete the trailing ws that is before insertion point, because - // it would become significant after break inserted. - nsresult rv = MOZ_KnownLive(mHTMLEditor) - .DeleteTextAndTextNodesWithTransaction( - beforeRun->StartPoint(), pointToInsert); - if (NS_FAILED(rv)) { - NS_WARNING( - "WSRunObject::DeleteTextAndTextNodesWithTransaction() failed"); - return nullptr; + if (invisibleLeadingWhiteSpaceRangeOfNewLine.IsPositioned()) { + if (!invisibleLeadingWhiteSpaceRangeOfNewLine.Collapsed()) { + // XXX Why don't we remove all of the invisible white-spaces? + MOZ_ASSERT(invisibleLeadingWhiteSpaceRangeOfNewLine.EndRef() == + pointToInsert); + // XXX If the DOM tree has been changed above, + // invisibleLeadingWhiteSpaceRangeOfNewLine may be invalid now. + // So, we may do something wrong here. + nsresult rv = + MOZ_KnownLive(mHTMLEditor) + .DeleteTextAndTextNodesWithTransaction( + invisibleLeadingWhiteSpaceRangeOfNewLine.StartRef(), + invisibleLeadingWhiteSpaceRangeOfNewLine.EndRef()); + if (NS_FAILED(rv)) { + NS_WARNING( + "WSRunObject::DeleteTextAndTextNodesWithTransaction() failed"); + return nullptr; + } } } // If the `
` element is put immediately after an NBSP, it should be @@ -982,41 +994,51 @@ void WSRunScanner::EnsureWSFragments() { textFragmentData.InitializeWSFragmentArray(mFragments); } -template -EditorDOMRangeType +EditorDOMRange WSRunScanner::TextFragmentData::GetInvisibleLeadingWhiteSpaceRange() const { + if (mLeadingWhiteSpaceRange.isSome()) { + return mLeadingWhiteSpaceRange.ref(); + } + // If it's preformatted or not start of line, the range is not invisible // leading white-spaces. // TODO: We should check each text node style rather than WSRunScanner's // scan start position's style. if (mIsPreformatted || !StartsFromHardLineBreak()) { - return EditorDOMRangeType(); + mLeadingWhiteSpaceRange.emplace(); + return mLeadingWhiteSpaceRange.ref(); } // If there is no NBSP, all of the given range is leading white-spaces. // Note that this result may be collapsed if there is no leading white-spaces. if (!mNBSPData.FoundNBSP()) { MOZ_ASSERT(mStart.PointRef().IsSet() || mEnd.PointRef().IsSet()); - return EditorDOMRangeType(mStart.PointRef(), mEnd.PointRef()); + mLeadingWhiteSpaceRange.emplace(mStart.PointRef(), mEnd.PointRef()); + return mLeadingWhiteSpaceRange.ref(); } MOZ_ASSERT(mNBSPData.LastPointRef().IsSetAndValid()); // Even if the first NBSP is the start, i.e., there is no invisible leading // white-space, return collapsed range. - return EditorDOMRangeType(mStart.PointRef(), mNBSPData.FirstPointRef()); + mLeadingWhiteSpaceRange.emplace(mStart.PointRef(), mNBSPData.FirstPointRef()); + return mLeadingWhiteSpaceRange.ref(); } -template -EditorDOMRangeType +EditorDOMRange WSRunScanner::TextFragmentData::GetInvisibleTrailingWhiteSpaceRange() const { + if (mTrailingWhiteSpaceRange.isSome()) { + return mTrailingWhiteSpaceRange.ref(); + } + // If it's preformatted or not immediately before block boundary, the range is // not invisible trailing white-spaces. Note that collapsible white-spaces // before a `
` element is visible. // TODO: We should check each text node style rather than WSRunScanner's // scan start position's style. if (mIsPreformatted || !EndsByBlockBoundary()) { - return EditorDOMRangeType(); + mTrailingWhiteSpaceRange.emplace(); + return mTrailingWhiteSpaceRange.ref(); } // If there is no NBSP, all of the given range is trailing white-spaces. @@ -1024,7 +1046,8 @@ WSRunScanner::TextFragmentData::GetInvisibleTrailingWhiteSpaceRange() const { // spaces. if (!mNBSPData.FoundNBSP()) { MOZ_ASSERT(mStart.PointRef().IsSet() || mEnd.PointRef().IsSet()); - return EditorDOMRangeType(mStart.PointRef(), mEnd.PointRef()); + mTrailingWhiteSpaceRange.emplace(mStart.PointRef(), mEnd.PointRef()); + return mTrailingWhiteSpaceRange.ref(); } MOZ_ASSERT(mNBSPData.LastPointRef().IsSetAndValid()); @@ -1035,13 +1058,15 @@ WSRunScanner::TextFragmentData::GetInvisibleTrailingWhiteSpaceRange() const { mNBSPData.LastPointRef().GetContainer() == mEnd.PointRef().GetContainer() && mNBSPData.LastPointRef().Offset() == mEnd.PointRef().Offset() - 1) { - return EditorDOMRangeType(); + mTrailingWhiteSpaceRange.emplace(); + return mTrailingWhiteSpaceRange.ref(); } // Otherwise, the may be some trailing white-spaces. MOZ_ASSERT(!mNBSPData.LastPointRef().IsEndOfContainer()); - return EditorDOMRangeType(mNBSPData.LastPointRef().NextPoint(), - mEnd.PointRef()); + mTrailingWhiteSpaceRange.emplace(mNBSPData.LastPointRef().NextPoint(), + mEnd.PointRef()); + return mTrailingWhiteSpaceRange.ref(); } Maybe @@ -1067,8 +1092,8 @@ WSRunScanner::TextFragmentData::CreateWSFragmentForVisibleAndMiddleOfLine() // If all of the range is invisible leading or trailing white-spaces, // there is no visible content. - const auto leadingWhiteSpaceRange = - GetInvisibleLeadingWhiteSpaceRange(); + const EditorDOMRange leadingWhiteSpaceRange = + GetInvisibleLeadingWhiteSpaceRange(); const bool maybeHaveLeadingWhiteSpaces = leadingWhiteSpaceRange.StartRef().IsSet() || leadingWhiteSpaceRange.EndRef().IsSet(); @@ -1077,8 +1102,8 @@ WSRunScanner::TextFragmentData::CreateWSFragmentForVisibleAndMiddleOfLine() leadingWhiteSpaceRange.EndRef() == mEnd.PointRef()) { return Nothing(); } - const auto trailingWhiteSpaceRange = - GetInvisibleTrailingWhiteSpaceRange(); + const EditorDOMRange trailingWhiteSpaceRange = + GetInvisibleTrailingWhiteSpaceRange(); const bool maybeHaveTrailingWhiteSpaces = trailingWhiteSpaceRange.StartRef().IsSet() || trailingWhiteSpaceRange.EndRef().IsSet(); @@ -1168,10 +1193,10 @@ void WSRunScanner::TextFragmentData::InitializeWSFragmentArray( return; } - const auto leadingWhiteSpaceRange = - GetInvisibleLeadingWhiteSpaceRange(); - const auto trailingWhiteSpaceRange = - GetInvisibleTrailingWhiteSpaceRange(); + const EditorDOMRange leadingWhiteSpaceRange = + GetInvisibleLeadingWhiteSpaceRange(); + const EditorDOMRange trailingWhiteSpaceRange = + GetInvisibleTrailingWhiteSpaceRange(); // XXX `IsPositioned()` is not availble here because currently it is not // guaranteed that both boundaries are set or unset at same time. const bool maybeHaveLeadingWhiteSpaces = @@ -2142,12 +2167,12 @@ nsresult WSRunObject::MaybeReplaceInclusiveNextNBSPWithASCIIWhiteSpace( nsresult WSRunObject::DeleteInvisibleASCIIWhiteSpacesInternal() { TextFragmentData textFragment(mStart, mEnd, mNBSPData, mPRE); - auto leadingWhiteSpaceRange = - textFragment.GetInvisibleLeadingWhiteSpaceRange(); + EditorDOMRange leadingWhiteSpaceRange = + textFragment.GetInvisibleLeadingWhiteSpaceRange(); // XXX Getting trailing white-space range now must be wrong because // mutation event listener may invalidate it. - auto trailingWhiteSpaceRange = - textFragment.GetInvisibleTrailingWhiteSpaceRange(); + EditorDOMRange trailingWhiteSpaceRange = + textFragment.GetInvisibleTrailingWhiteSpaceRange(); DebugOnly leadingWhiteSpacesDeleted = false; if (leadingWhiteSpaceRange.IsPositioned() && !leadingWhiteSpaceRange.Collapsed()) { diff --git a/editor/libeditor/WSRunObject.h b/editor/libeditor/WSRunObject.h index b06aab304032..2fba4d3b64c7 100644 --- a/editor/libeditor/WSRunObject.h +++ b/editor/libeditor/WSRunObject.h @@ -774,8 +774,7 @@ class MOZ_STACK_CLASS WSRunScanner { * Note that if there are only invisible white-spaces in a hard line, * this returns all of the white-spaces. */ - template - EditorDOMRangeType GetInvisibleLeadingWhiteSpaceRange() const; + EditorDOMRange GetInvisibleLeadingWhiteSpaceRange() const; /** * GetInvisibleTrailingWhiteSpaceRange() returns two DOM points, @@ -785,8 +784,105 @@ class MOZ_STACK_CLASS WSRunScanner { * Note that if there are only invisible white-spaces in a hard line, * this returns all of the white-spaces. */ - template - EditorDOMRangeType GetInvisibleTrailingWhiteSpaceRange() const; + EditorDOMRange GetInvisibleTrailingWhiteSpaceRange() const; + + /** + * GetNewInvisibleLeadingWhiteSpaceRangeIfSplittingAt() returns new + * invisible leading white-space range which should be removed if + * splitting invisible white-space sequence at aPointToSplit creates + * new invisible leading white-spaces in the new line. + * Note that the result may be collapsed range if the point is around + * invisible white-spaces. + */ + template + EditorDOMRange GetNewInvisibleLeadingWhiteSpaceRangeIfSplittingAt( + const EditorDOMPointType& aPointToSplit) const { + // If there are invisible trailing white-spaces and some or all of them + // become invisible leading white-spaces in the new line, although we + // don't need to delete them, but for aesthetically and backward + // compatibility, we should remove them. + EditorDOMRange trailingWhiteSpaceRange = + GetInvisibleTrailingWhiteSpaceRange(); + // XXX Why don't we check leading white-spaces too? + if (!trailingWhiteSpaceRange.IsPositioned()) { + return trailingWhiteSpaceRange; + } + // XXX Why don't we need to treat new trailing white-spaces are invisible + // when the trailing white-spaces are only the content in current + // line? + if (trailingWhiteSpaceRange != GetInvisibleLeadingWhiteSpaceRange()) { + return EditorDOMRange(); + } + // If the point is before the trailing white-spaces, the new line won't + // start with leading white-spaces. + if (aPointToSplit.IsBefore(trailingWhiteSpaceRange.StartRef())) { + return EditorDOMRange(); + } + // If the point is in the trailing white-spaces, the new line may + // start with some leading white-spaces. Returning collapsed range + // is intentional because the caller may want to know whether the + // point is in trailing white-spaces or not. + if (aPointToSplit.EqualsOrIsBefore(trailingWhiteSpaceRange.EndRef())) { + return EditorDOMRange(trailingWhiteSpaceRange.StartRef(), + aPointToSplit); + } + // Otherwise, if the point is after the trailing white-spaces, it may + // be just outside of the text node. E.g., end of parent element. + // This is possible case but the validation cost is not worthwhile + // due to the runtime cost in the worst case. Therefore, we should just + // return collapsed range at the end of trailing white-spaces. Then, + // callers can know the point is immediately after the trailing + // white-spaces. + return EditorDOMRange(trailingWhiteSpaceRange.EndRef()); + } + + /** + * GetNewInvisibleTrailingWhiteSpaceRangeIfSplittingAt() returns new + * invisible trailing white-space range which should be removed if + * splitting invisible white-space sequence at aPointToSplit creates + * new invisible trailing white-spaces in the new line. + * Note that the result may be collapsed range if the point is around + * invisible white-spaces. + */ + template + EditorDOMRange GetNewInvisibleTrailingWhiteSpaceRangeIfSplittingAt( + const EditorDOMPointType& aPointToSplit) const { + // If there are invisible leading white-spaces and some or all of them + // become end of current line, they will become visible. Therefore, we + // need to delete the invisible leading white-spaces before insertion + // point. + EditorDOMRange leadingWhiteSpaceRange = + GetInvisibleLeadingWhiteSpaceRange(); + if (!leadingWhiteSpaceRange.IsPositioned()) { + return leadingWhiteSpaceRange; + } + // XXX Why don't we need to treat new leading white-spaces are invisible + // when the leading white-spaces are only the content in current + // line? + if (leadingWhiteSpaceRange != GetInvisibleTrailingWhiteSpaceRange()) { + return EditorDOMRange(); + } + // If the point equals or is after the leading white-spaces, the line + // will end without trailing white-spaces. + if (leadingWhiteSpaceRange.EndRef().IsBefore(aPointToSplit)) { + return EditorDOMRange(); + } + // If the point is in the leading white-spaces, the line may + // end with some trailing white-spaces. Returning collapsed range + // is intentional because the caller may want to know whether the + // point is in leading white-spaces or not. + if (leadingWhiteSpaceRange.StartRef().EqualsOrIsBefore(aPointToSplit)) { + return EditorDOMRange(aPointToSplit, leadingWhiteSpaceRange.EndRef()); + } + // Otherwise, if the point is before the leading white-spaces, it may + // be just outside of the text node. E.g., start of parent element. + // This is possible case but the validation cost is not worthwhile + // due to the runtime cost in the worst case. Therefore, we should + // just return collapsed range at start of the leading white-spaces. + // Then, callers can know the point is immediately before the leading + // white-spaces. + return EditorDOMRange(leadingWhiteSpaceRange.StartRef()); + } /** * CreateWSFragmentForVisibleAndMiddleOfLine() creates WSFragment whose @@ -798,6 +894,8 @@ class MOZ_STACK_CLASS WSRunScanner { BoundaryData mStart; BoundaryData mEnd; NoBreakingSpaceData mNBSPData; + mutable Maybe mLeadingWhiteSpaceRange; + mutable Maybe mTrailingWhiteSpaceRange; // XXX Currently we set mIsPreformatted to `WSRunScanner::mPRE` value // even if some text nodes between mStart and mEnd are different styled // nodes. This caused some bugs actually, but we now keep traditional