From 18912959bd6807fb41bf67c019ea97d475deb443 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Tue, 13 Feb 2018 19:01:42 +0900 Subject: [PATCH] Bug 1406726 - HTMLEditRules::WillInsertBreak() should reset mNewNode with caret position r=m_kato HTMLEditRules::WillInsertBreak() started to use HTMLEditRules::MakeBasicBlock() to wrap existing inline elements with default paragraph separator if inline elements are children of editing host. However, HTMLEditRules::ApplyBlockStyle() called by HTMLEditRules::MakeBasicBlock() sets mNewNode to last new block node. So, if it wraps inline elements after caret position, mNewNode becomes after expected caret position and HTMLEditRules::AfterEditInner() will move caret into it. This patch make HTMLEditRules::WillInsertBreak() reset mNewNode with caret position after calling HTMLEditRules::MakeBasicBlock(). Additionally, this patch fixes a bug of HTMLEditor::IsVisibleBRElement(). That is, it uses only editable nodes to check if given
element is visible. However, editable state is not related to this check. If
element is followed by non-editable inline node (except invisible data nodes), it always visible. This bug caused getting wrong nodes with HTMLEditRules::GetNodesFromSelection() which is used by HTMLEditRules::MakeBasicBlock(). Therefore, this patch also adds lots of EditorBase::Get(Next|Previous)ElementOrText*() and HTMLEditor::Get(Next|Previous)HTMLElementOrText*() to ignore only invisible data nodes. Note that even with this fix, the range of nodes computed by HTMLEditRules::GetNodesFromSelection() is still odd if only non-editable elements follow a
node which is first
element after the caret position. However, what is expected by the execCommand spec is unclear. So, automated test added by this patch checks current inconsistent behavior for now. MozReview-Commit-ID: 2m52StwoEEH --HG-- extra : rebase_source : 6b9b2338e35c4d2e89a405fd8e1ffc7b0873ca1e --- editor/libeditor/EditorBase.cpp | 38 ++++-- editor/libeditor/EditorBase.h | 89 +++++++++++--- editor/libeditor/HTMLEditRules.cpp | 20 ++++ editor/libeditor/HTMLEditRules.h | 18 +++ editor/libeditor/HTMLEditor.cpp | 60 +++++++++- editor/libeditor/HTMLEditor.h | 71 ++++++++++- editor/libeditor/tests/mochitest.ini | 1 + editor/libeditor/tests/test_bug1406726.html | 126 ++++++++++++++++++++ 8 files changed, 391 insertions(+), 32 deletions(-) create mode 100644 editor/libeditor/tests/test_bug1406726.html diff --git a/editor/libeditor/EditorBase.cpp b/editor/libeditor/EditorBase.cpp index 42f81d7b4765..bd4a8ab4e04e 100644 --- a/editor/libeditor/EditorBase.cpp +++ b/editor/libeditor/EditorBase.cpp @@ -3571,17 +3571,20 @@ EditorBase::GetNodeLocation(nsINode* aChild, nsIContent* EditorBase::GetPreviousNodeInternal(nsINode& aNode, bool aFindEditableNode, + bool aFindAnyDataNode, bool aNoBlockCrossing) { if (!IsDescendantOfEditorRoot(&aNode)) { return nullptr; } - return FindNode(&aNode, false, aFindEditableNode, aNoBlockCrossing); + return FindNode(&aNode, false, + aFindEditableNode, aFindAnyDataNode, aNoBlockCrossing); } nsIContent* EditorBase::GetPreviousNodeInternal(const EditorRawDOMPoint& aPoint, bool aFindEditableNode, + bool aFindAnyDataNode, bool aNoBlockCrossing) { MOZ_ASSERT(aPoint.IsSetAndValid()); @@ -3597,13 +3600,15 @@ EditorBase::GetPreviousNodeInternal(const EditorRawDOMPoint& aPoint, return nullptr; } return GetPreviousNodeInternal(*aPoint.GetContainer(), - aFindEditableNode, aNoBlockCrossing); + aFindEditableNode, aFindAnyDataNode, + aNoBlockCrossing); } // else look before the child at 'aOffset' if (aPoint.GetChild()) { return GetPreviousNodeInternal(*aPoint.GetChild(), - aFindEditableNode, aNoBlockCrossing); + aFindEditableNode, aFindAnyDataNode, + aNoBlockCrossing); } // unless there isn't one, in which case we are at the end of the node @@ -3614,29 +3619,34 @@ EditorBase::GetPreviousNodeInternal(const EditorRawDOMPoint& aPoint, return nullptr; } - if (!aFindEditableNode || IsEditable(rightMostNode)) { + if ((!aFindEditableNode || IsEditable(rightMostNode)) && + (aFindAnyDataNode || IsElementOrText(*rightMostNode))) { return rightMostNode; } // restart the search from the non-editable node we just found return GetPreviousNodeInternal(*rightMostNode, - aFindEditableNode, aNoBlockCrossing); + aFindEditableNode, aFindAnyDataNode, + aNoBlockCrossing); } nsIContent* EditorBase::GetNextNodeInternal(nsINode& aNode, bool aFindEditableNode, + bool aFindAnyDataNode, bool aNoBlockCrossing) { if (!IsDescendantOfEditorRoot(&aNode)) { return nullptr; } - return FindNode(&aNode, true, aFindEditableNode, aNoBlockCrossing); + return FindNode(&aNode, true, + aFindEditableNode, aFindAnyDataNode, aNoBlockCrossing); } nsIContent* EditorBase::GetNextNodeInternal(const EditorRawDOMPoint& aPoint, bool aFindEditableNode, + bool aFindAnyDataNode, bool aNoBlockCrossing) { MOZ_ASSERT(aPoint.IsSetAndValid()); @@ -3670,13 +3680,15 @@ EditorBase::GetNextNodeInternal(const EditorRawDOMPoint& aPoint, return nullptr; } - if (!aFindEditableNode || IsEditable(leftMostNode)) { + if ((!aFindEditableNode || IsEditable(leftMostNode)) && + (aFindAnyDataNode || IsElementOrText(*leftMostNode))) { return leftMostNode; } // restart the search from the non-editable node we just found return GetNextNodeInternal(*leftMostNode, - aFindEditableNode, aNoBlockCrossing); + aFindEditableNode, aFindAnyDataNode, + aNoBlockCrossing); } // unless there isn't one, in which case we are at the end of the node @@ -3687,7 +3699,8 @@ EditorBase::GetNextNodeInternal(const EditorRawDOMPoint& aPoint, } return GetNextNodeInternal(*point.GetContainer(), - aFindEditableNode, aNoBlockCrossing); + aFindEditableNode, aFindAnyDataNode, + aNoBlockCrossing); } nsIContent* @@ -3746,6 +3759,7 @@ nsIContent* EditorBase::FindNode(nsINode* aCurrentNode, bool aGoForward, bool aEditableNode, + bool aFindAnyDataNode, bool bNoBlockCrossing) { if (IsEditorRoot(aCurrentNode)) { @@ -3763,11 +3777,13 @@ EditorBase::FindNode(nsINode* aCurrentNode, return nullptr; } - if (!aEditableNode || IsEditable(candidate)) { + if ((!aEditableNode || IsEditable(candidate)) && + (aFindAnyDataNode || IsElementOrText(*candidate))) { return candidate; } - return FindNode(candidate, aGoForward, aEditableNode, bNoBlockCrossing); + return FindNode(candidate, aGoForward, + aEditableNode, aFindAnyDataNode, bNoBlockCrossing); } nsIContent* diff --git a/editor/libeditor/EditorBase.h b/editor/libeditor/EditorBase.h index b0cfaeb32fea..5d97a596eb04 100644 --- a/editor/libeditor/EditorBase.h +++ b/editor/libeditor/EditorBase.h @@ -632,12 +632,15 @@ protected: nsIContent* FindNode(nsINode* aCurrentNode, bool aGoForward, bool aEditableNode, + bool aFindAnyDataNode, bool bNoBlockCrossing); /** * Get the node immediately previous node of aNode. * @param atNode The node from which we start the search. * @param aFindEditableNode If true, only return an editable node. + * @param aFindAnyDataNode If true, may return invisible data node + * like Comment. * @param aNoBlockCrossing If true, don't move across "block" nodes, * whatever that means. * @return The node that occurs before aNode in @@ -647,6 +650,7 @@ protected: */ nsIContent* GetPreviousNodeInternal(nsINode& aNode, bool aFindEditableNode, + bool aFindAnyDataNode, bool aNoBlockCrossing); /** @@ -654,12 +658,15 @@ protected: */ nsIContent* GetPreviousNodeInternal(const EditorRawDOMPoint& aPoint, bool aFindEditableNode, + bool aFindAnyDataNode, bool aNoBlockCrossing); /** * Get the node immediately next node of aNode. * @param aNode The node from which we start the search. * @param aFindEditableNode If true, only return an editable node. + * @param aFindAnyDataNode If true, may return invisible data node + * like Comment. * @param aNoBlockCrossing If true, don't move across "block" nodes, * whatever that means. * @return The node that occurs after aNode in the @@ -669,6 +676,7 @@ protected: */ nsIContent* GetNextNodeInternal(nsINode& aNode, bool aFindEditableNode, + bool aFindAnyDataNode, bool bNoBlockCrossing); /** @@ -676,6 +684,7 @@ protected: */ nsIContent* GetNextNodeInternal(const EditorRawDOMPoint& aPoint, bool aFindEditableNode, + bool aFindAnyDataNode, bool aNoBlockCrossing); @@ -807,36 +816,52 @@ public: */ nsIContent* GetPreviousNode(const EditorRawDOMPoint& aPoint) { - return GetPreviousNodeInternal(aPoint, false, false); + return GetPreviousNodeInternal(aPoint, false, true, false); + } + nsIContent* GetPreviousElementOrText(const EditorRawDOMPoint& aPoint) + { + return GetPreviousNodeInternal(aPoint, false, false, false); } nsIContent* GetPreviousEditableNode(const EditorRawDOMPoint& aPoint) { - return GetPreviousNodeInternal(aPoint, true, false); + return GetPreviousNodeInternal(aPoint, true, true, false); } nsIContent* GetPreviousNodeInBlock(const EditorRawDOMPoint& aPoint) { - return GetPreviousNodeInternal(aPoint, false, true); + return GetPreviousNodeInternal(aPoint, false, true, true); + } + nsIContent* GetPreviousElementOrTextInBlock(const EditorRawDOMPoint& aPoint) + { + return GetPreviousNodeInternal(aPoint, false, false, true); } nsIContent* GetPreviousEditableNodeInBlock( const EditorRawDOMPoint& aPoint) { - return GetPreviousNodeInternal(aPoint, true, true); + return GetPreviousNodeInternal(aPoint, true, true, true); } nsIContent* GetPreviousNode(nsINode& aNode) { - return GetPreviousNodeInternal(aNode, false, false); + return GetPreviousNodeInternal(aNode, false, true, false); + } + nsIContent* GetPreviousElementOrText(nsINode& aNode) + { + return GetPreviousNodeInternal(aNode, false, false, false); } nsIContent* GetPreviousEditableNode(nsINode& aNode) { - return GetPreviousNodeInternal(aNode, true, false); + return GetPreviousNodeInternal(aNode, true, true, false); } nsIContent* GetPreviousNodeInBlock(nsINode& aNode) { - return GetPreviousNodeInternal(aNode, false, true); + return GetPreviousNodeInternal(aNode, false, true, true); + } + nsIContent* GetPreviousElementOrTextInBlock(nsINode& aNode) + { + return GetPreviousNodeInternal(aNode, false, false, true); } nsIContent* GetPreviousEditableNodeInBlock(nsINode& aNode) { - return GetPreviousNodeInternal(aNode, true, true); + return GetPreviousNodeInternal(aNode, true, true, true); } /** @@ -867,36 +892,52 @@ public: */ nsIContent* GetNextNode(const EditorRawDOMPoint& aPoint) { - return GetNextNodeInternal(aPoint, false, false); + return GetNextNodeInternal(aPoint, false, true, false); + } + nsIContent* GetNextElementOrText(const EditorRawDOMPoint& aPoint) + { + return GetNextNodeInternal(aPoint, false, false, false); } nsIContent* GetNextEditableNode(const EditorRawDOMPoint& aPoint) { - return GetNextNodeInternal(aPoint, true, false); + return GetNextNodeInternal(aPoint, true, true, false); } nsIContent* GetNextNodeInBlock(const EditorRawDOMPoint& aPoint) { - return GetNextNodeInternal(aPoint, false, true); + return GetNextNodeInternal(aPoint, false, true, true); + } + nsIContent* GetNextElementOrTextInBlock(const EditorRawDOMPoint& aPoint) + { + return GetNextNodeInternal(aPoint, false, false, true); } nsIContent* GetNextEditableNodeInBlock( const EditorRawDOMPoint& aPoint) { - return GetNextNodeInternal(aPoint, true, true); + return GetNextNodeInternal(aPoint, true, true, true); } nsIContent* GetNextNode(nsINode& aNode) { - return GetNextNodeInternal(aNode, false, false); + return GetNextNodeInternal(aNode, false, true, false); + } + nsIContent* GetNextElementOrText(nsINode& aNode) + { + return GetNextNodeInternal(aNode, false, false, false); } nsIContent* GetNextEditableNode(nsINode& aNode) { - return GetNextNodeInternal(aNode, true, false); + return GetNextNodeInternal(aNode, true, true, false); } nsIContent* GetNextNodeInBlock(nsINode& aNode) { - return GetNextNodeInternal(aNode, false, true); + return GetNextNodeInternal(aNode, false, true, true); + } + nsIContent* GetNextElementOrTextInBlock(nsINode& aNode) + { + return GetNextNodeInternal(aNode, false, false, true); } nsIContent* GetNextEditableNodeInBlock(nsINode& aNode) { - return GetNextNodeInternal(aNode, true, true); + return GetNextNodeInternal(aNode, true, true, true); } /** @@ -974,6 +1015,20 @@ public: } } + /** + * Returns true if aNode is a usual element node (not bogus node) or + * a text node. In other words, returns true if aNode is a usual element + * node or visible data node. + */ + bool IsElementOrText(const nsINode& aNode) const + { + if (!aNode.IsContent() || IsMozEditorBogusNode(&aNode)) { + return false; + } + return aNode.NodeType() == nsINode::ELEMENT_NODE || + aNode.NodeType() == nsINode::TEXT_NODE; + } + /** * Returns true if selection is in an editable element and both the range * start and the range end are editable. E.g., even if the selection range @@ -985,7 +1040,7 @@ public: /** * Returns true if aNode is a MozEditorBogus node. */ - bool IsMozEditorBogusNode(nsINode* aNode) + bool IsMozEditorBogusNode(const nsINode* aNode) const { return aNode && aNode->IsElement() && aNode->AsElement()->AttrValueIs(kNameSpaceID_None, diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index 326b411c1760..f808699829cb 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -1752,6 +1752,14 @@ HTMLEditRules::WillInsertBreak(Selection& aSelection, *aHandled = true; return NS_OK; } + // Now, mNewBlock is last created block element for wrapping inline + // elements around the caret position and AfterEditInner() will move + // caret into it. However, it may be different from block parent of + // the caret position. E.g., MakeBasicBlock() may wrap following + // inline elements of a
element which is next sibling of container + // of the caret. So, we need to adjust mNewBlock here for avoiding + // jumping caret to odd position. + mNewBlock = blockParent; } // If block is empty, populate with br. (For example, imagine a div that @@ -5925,6 +5933,18 @@ HTMLEditRules::GetPromotedPoint(RulesEndpoint aWhere, // look ahead through any further inline nodes that aren't across a
from // us, and that are enclosed in the same block. + // XXX Currently, we stop block-extending when finding visible
element. + // This might be different from "block-extend" of execCommand spec. + // However, the spec is really unclear. + // XXX Probably, scanning only editable nodes is wrong for + // EditAction::makeBasicBlock because it might be better to wrap existing + // inline elements even if it's non-editable. For example, following + // examples with insertParagraph causes different result: + // *
foo[]bar
+ // *
foo[]bar
+ // *
foo[]barbaz
+ // Only in the first case, after the caret position isn't wrapped with + // new
element. nsCOMPtr nextNode = htmlEditor->GetNextEditableHTMLNodeInBlock(point.AsRaw()); diff --git a/editor/libeditor/HTMLEditRules.h b/editor/libeditor/HTMLEditRules.h index 743a0eb9d94a..890e614fc6dd 100644 --- a/editor/libeditor/HTMLEditRules.h +++ b/editor/libeditor/HTMLEditRules.h @@ -420,8 +420,26 @@ protected: void MakeTransitionList(nsTArray>& aNodeArray, nsTArray& aTransitionArray); nsresult RemoveBlockStyle(nsTArray>& aNodeArray); + + /** + * ApplyBlockStyle() formats all nodes in aNodeArray with block elements + * whose name is aBlockTag. + * If aNodeArray has an inline element, a block element is created and the + * inline element and following inline elements are moved into the new block + * element. + * If aNodeArray has
elements, they'll be removed from the DOM tree and + * new block element will be created when there are some remaining inline + * elements. + * If aNodeArray has a block element, this calls itself with children of + * the block element. Then, new block element will be created when there + * are some remaining inline elements. + * + * @param aNodeArray Must be descendants of a node. + * @param aBlockTag The element name of new block elements. + */ nsresult ApplyBlockStyle(nsTArray>& aNodeArray, nsAtom& aBlockTag); + nsresult MakeBlockquote(nsTArray>& aNodeArray); /** diff --git a/editor/libeditor/HTMLEditor.cpp b/editor/libeditor/HTMLEditor.cpp index 017cd13d98cf..3287badda849 100644 --- a/editor/libeditor/HTMLEditor.cpp +++ b/editor/libeditor/HTMLEditor.cpp @@ -870,8 +870,13 @@ HTMLEditor::IsVisibleBRElement(nsINode* aNode) if (!TextEditUtils::IsBreak(aNode)) { return false; } - // Check if there is a later node in block after br - nsCOMPtr nextNode = GetNextEditableHTMLNodeInBlock(*aNode); + // Check if there is another element or text node in block after current + //
element. + // Note that even if following node is non-editable, it may make the + //
element visible if it just exists. + // E.g., foo
+ // However, we need to ignore invisible data nodes like comment node. + nsCOMPtr nextNode = GetNextHTMLElementOrTextInBlock(*aNode); if (nextNode && TextEditUtils::IsBreak(nextNode)) { return true; } @@ -890,7 +895,11 @@ HTMLEditor::IsVisibleBRElement(nsINode* aNode) // If there's an inline node after this one that's not a break, and also a // prior break, this break must be visible. - nsCOMPtr priorNode = GetPreviousEditableHTMLNodeInBlock(*aNode); + // Note that even if previous node is non-editable, it may make the + //
element visible if it just exists. + // E.g.,