Bug 1406726 - HTMLEditRules::WillInsertBreak() should reset mNewNode with caret position r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 13 Feb 2018 19:01:42 +0900
changeset 403917 ac9440a7256caa96064e9a7b259c89630a5aa50e
parent 403916 9c6aa0a5ec8d084a499eb3ff85f676ee29476769
child 403918 a281dd360b0a92a4eb0492a88170ddbf2a30ec74
push id99885
push userapavel@mozilla.com
push dateThu, 15 Feb 2018 10:38:09 +0000
treeherdermozilla-inbound@99495614cba7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1406726
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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 <br> element is visible. However, editable state is not related to this check. If <br> 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 <br> node which is first <br> 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
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditRules.h
editor/libeditor/HTMLEditor.cpp
editor/libeditor/HTMLEditor.h
editor/libeditor/tests/mochitest.ini
editor/libeditor/tests/test_bug1406726.html
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -3566,82 +3566,92 @@ EditorBase::GetNodeLocation(nsINode* aCh
     *aOffset = -1;
   }
   return parent;
 }
 
 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());
   NS_WARNING_ASSERTION(!aPoint.IsInDataNode() || aPoint.IsInTextNode(),
     "GetPreviousNodeInternal() doesn't assume that the start point is a "
     "data node except text node");
 
   // If we are at the beginning of the node, or it is a text node, then just
   // look before it.
   if (aPoint.IsStartOfContainer() || aPoint.IsInTextNode()) {
     if (aNoBlockCrossing && IsBlockNode(aPoint.GetContainer())) {
       // If we aren't allowed to cross blocks, don't look before this block.
       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
   // and want the deep-right child.
   nsIContent* rightMostNode =
     GetRightmostChild(aPoint.GetContainer(), aNoBlockCrossing);
   if (!rightMostNode) {
     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());
   NS_WARNING_ASSERTION(!aPoint.IsInDataNode() || aPoint.IsInTextNode(),
     "GetNextNodeInternal() doesn't assume that the start point is a "
     "data node except text node");
 
   EditorRawDOMPoint point(aPoint);
@@ -3665,34 +3675,37 @@ EditorBase::GetNextNodeInternal(const Ed
     if (!leftMostNode) {
       return point.GetChild();
     }
 
     if (!IsDescendantOfEditorRoot(leftMostNode)) {
       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
   // and want the next one.
   if (aNoBlockCrossing && IsBlockNode(point.GetContainer())) {
     // don't cross out of parent block
     return nullptr;
   }
 
   return GetNextNodeInternal(*point.GetContainer(),
-                             aFindEditableNode, aNoBlockCrossing);
+                             aFindEditableNode, aFindAnyDataNode,
+                             aNoBlockCrossing);
 }
 
 nsIContent*
 EditorBase::FindNextLeafNode(nsINode* aCurrentNode,
                              bool aGoForward,
                              bool bNoBlockCrossing)
 {
   // called only by GetPriorNode so we don't need to check params.
@@ -3741,16 +3754,17 @@ EditorBase::FindNextLeafNode(nsINode* aC
   NS_NOTREACHED("What part of for(;;) do you not understand?");
   return nullptr;
 }
 
 nsIContent*
 EditorBase::FindNode(nsINode* aCurrentNode,
                      bool aGoForward,
                      bool aEditableNode,
+                     bool aFindAnyDataNode,
                      bool bNoBlockCrossing)
 {
   if (IsEditorRoot(aCurrentNode)) {
     // Don't allow traversal above the root node! This helps
     // prevent us from accidentally editing browser content
     // when the editor is in a text widget.
 
     return nullptr;
@@ -3758,21 +3772,23 @@ EditorBase::FindNode(nsINode* aCurrentNo
 
   nsCOMPtr<nsIContent> candidate =
     FindNextLeafNode(aCurrentNode, aGoForward, bNoBlockCrossing);
 
   if (!candidate) {
     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*
 EditorBase::GetRightmostChild(nsINode* aCurrentNode,
                               bool bNoBlockCrossing)
 {
   NS_ENSURE_TRUE(aCurrentNode, nullptr);
   nsIContent *cur = aCurrentNode->GetLastChild();
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -627,60 +627,69 @@ protected:
    * Helper for GetPreviousNodeInternal() and GetNextNodeInternal().
    */
   nsIContent* FindNextLeafNode(nsINode* aCurrentNode,
                                bool aGoForward,
                                bool bNoBlockCrossing);
   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
    *                             the tree, skipping non-editable nodes if
    *                             aFindEditableNode is true.  If there is no
    *                             previous node, returns nullptr.
    */
   nsIContent* GetPreviousNodeInternal(nsINode& aNode,
                                       bool aFindEditableNode,
+                                      bool aFindAnyDataNode,
                                       bool aNoBlockCrossing);
 
   /**
    * And another version that takes a point in DOM tree rather than a node.
    */
   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
    *                             tree, skipping non-editable nodes if
    *                             aFindEditableNode is true.  If there is no
    *                             next node, returns nullptr.
    */
   nsIContent* GetNextNodeInternal(nsINode& aNode,
                                   bool aFindEditableNode,
+                                  bool aFindAnyDataNode,
                                   bool bNoBlockCrossing);
 
   /**
    * And another version that takes a point in DOM tree rather than a node.
    */
   nsIContent* GetNextNodeInternal(const EditorRawDOMPoint& aPoint,
                                   bool aFindEditableNode,
+                                  bool aFindAnyDataNode,
                                   bool aNoBlockCrossing);
 
 
   virtual nsresult InstallEventListeners();
   virtual void CreateEventListeners();
   virtual void RemoveEventListeners();
 
   /**
@@ -802,46 +811,62 @@ public:
                                                       int32_t* outOffset);
   static nsINode* GetNodeLocation(nsINode* aChild, int32_t* aOffset);
 
   /**
    * Get the previous node.
    */
   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);
   }
 
   /**
    * Get the next node.
    *
    * Note that methods taking EditorRawDOMPoint behavior includes the
    * child at offset as search target.  E.g., following code causes infinite
    * loop.
@@ -862,46 +887,62 @@ public:
    * }
    *
    * On the other hand, the methods taking nsINode behavior must be what
    * you want.  They start to search the result from next node of the given
    * node.
    */
   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);
   }
 
   /**
    * Get the rightmost child of aCurrentNode;
    * return nullptr if aCurrentNode has no children.
    */
   nsIContent* GetRightmostChild(nsINode* aCurrentNode,
                                 bool bNoBlockCrossing = false);
@@ -970,27 +1011,41 @@ public:
         // Text nodes are considered to be editable by both typed of editors.
         return true;
       default:
         return false;
     }
   }
 
   /**
+   * 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
    * includes non-editable elements, returns true when one of common ancestors
    * of the range start and the range end is editable.  Otherwise, false.
    */
   bool IsSelectionEditable();
 
   /**
    * 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,
                kMOZEditorBogusNodeAttrAtom, kMOZEditorBogusNodeValue,
                eCaseMatters);
   }
 
   /**
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1747,16 +1747,24 @@ HTMLEditRules::WillInsertBreak(Selection
       // Didn't create a new block for some reason, fall back to <br>
       rv = InsertBRElement(aSelection, atStartOfSelection);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
       *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 <br> 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
   // contains the word "text".  The user selects "text" and types return.
   // "Text" is deleted leaving an empty block.  We want to put in one br to
   // make block have a line.  Then code further below will put in a second br.)
   if (IsEmptyBlockElement(*blockParent, IgnoreSingleBR::eNo)) {
     AutoEditorDOMPointChildInvalidator lockOffset(atStartOfSelection);
@@ -5920,16 +5928,28 @@ HTMLEditRules::GetPromotedPoint(RulesEnd
     point.Set(point.GetContainer());
     DebugOnly<bool> advanced = point.AdvanceOffset();
     NS_WARNING_ASSERTION(advanced,
       "Failed to advance offset to after the text node");
   }
 
   // look ahead through any further inline nodes that aren't across a <br> from
   // us, and that are enclosed in the same block.
+  // XXX Currently, we stop block-extending when finding visible <br> 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:
+  //     * <div contenteditable>foo[]<b contenteditable="false">bar</b></div>
+  //     * <div contenteditable>foo[]<b>bar</b></div>
+  //     * <div contenteditable>foo[]<b contenteditable="false">bar</b>baz</div>
+  //     Only in the first case, after the caret position isn't wrapped with
+  //     new <div> element.
   nsCOMPtr<nsIContent> nextNode =
     htmlEditor->GetNextEditableHTMLNodeInBlock(point.AsRaw());
 
   while (nextNode && !IsBlockNode(*nextNode) && nextNode->GetParentNode()) {
     point.Set(nextNode);
     if (NS_WARN_IF(!point.AdvanceOffset())) {
       break;
     }
--- a/editor/libeditor/HTMLEditRules.h
+++ b/editor/libeditor/HTMLEditRules.h
@@ -415,18 +415,36 @@ protected:
    * GetHiestInlineParent() returns the highest inline node parent between
    * aNode and the editing host.  Even if the editing host is an inline
    * element, this method never returns the editing host as the result.
    */
   nsIContent* GetHighestInlineParent(nsINode& aNode);
   void MakeTransitionList(nsTArray<OwningNonNull<nsINode>>& aNodeArray,
                           nsTArray<bool>& aTransitionArray);
   nsresult RemoveBlockStyle(nsTArray<OwningNonNull<nsINode>>& 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 <br> 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<OwningNonNull<nsINode>>& aNodeArray,
                            nsAtom& aBlockTag);
+
   nsresult MakeBlockquote(nsTArray<OwningNonNull<nsINode>>& aNodeArray);
 
   /**
    * MaybeSplitAncestorsForInsert() does nothing if container of
    * aStartOfDeepestRightNode can have an element whose tag name is aTag.
    * Otherwise, looks for an ancestor node which is or is in active editing
    * host and can have an element whose name is aTag.  If there is such
    * ancestor, its descendants are split.
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -865,18 +865,23 @@ HTMLEditor::IsPrevCharInNodeWhitespace(n
 
 bool
 HTMLEditor::IsVisibleBRElement(nsINode* aNode)
 {
   MOZ_ASSERT(aNode);
   if (!TextEditUtils::IsBreak(aNode)) {
     return false;
   }
-  // Check if there is a later node in block after br
-  nsCOMPtr<nsINode> nextNode = GetNextEditableHTMLNodeInBlock(*aNode);
+  // Check if there is another element or text node in block after current
+  // <br> element.
+  // Note that even if following node is non-editable, it may make the
+  // <br> element visible if it just exists.
+  // E.g., foo<br><button contenteditable="false">button</button>
+  // However, we need to ignore invisible data nodes like comment node.
+  nsCOMPtr<nsINode> nextNode = GetNextHTMLElementOrTextInBlock(*aNode);
   if (nextNode && TextEditUtils::IsBreak(nextNode)) {
     return true;
   }
 
   // A single line break before a block boundary is not displayed, so e.g.
   // foo<p>bar<br></p> and foo<br><p>bar</p> display the same as foo<p>bar</p>.
   // But if there are multiple <br>s in a row, all but the last are visible.
   if (!nextNode) {
@@ -885,17 +890,21 @@ HTMLEditor::IsVisibleBRElement(nsINode* 
   }
   if (IsBlockNode(nextNode)) {
     // Break is right before a block, it's not visible
     return false;
   }
 
   // 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<nsINode> priorNode = GetPreviousEditableHTMLNodeInBlock(*aNode);
+  // Note that even if previous node is non-editable, it may make the
+  // <br> element visible if it just exists.
+  // E.g., <button contenteditable="false"><br>foo
+  // However, we need to ignore invisible data nodes like comment node.
+  nsCOMPtr<nsINode> priorNode = GetPreviousHTMLElementOrTextInBlock(*aNode);
   if (priorNode && TextEditUtils::IsBreak(priorNode)) {
     return true;
   }
 
   // Sigh.  We have to use expensive whitespace calculation code to
   // determine what is going on
   int32_t selOffset;
   nsCOMPtr<nsINode> selNode = GetNodeLocation(aNode, &selOffset);
@@ -3699,16 +3708,39 @@ HTMLEditor::GetNextHTMLSibling(nsINode* 
   while (node && !IsEditable(node)) {
     node = node->GetNextSibling();
   }
 
   return node;
 }
 
 nsIContent*
+HTMLEditor::GetPreviousHTMLElementOrTextInternal(nsINode& aNode,
+                                                 bool aNoBlockCrossing)
+{
+  if (!GetActiveEditingHost()) {
+    return nullptr;
+  }
+  return aNoBlockCrossing ? GetPreviousElementOrTextInBlock(aNode) :
+                            GetPreviousElementOrText(aNode);
+}
+
+nsIContent*
+HTMLEditor::GetPreviousHTMLElementOrTextInternal(
+              const EditorRawDOMPoint& aPoint,
+              bool aNoBlockCrossing)
+{
+  if (!GetActiveEditingHost()) {
+    return nullptr;
+  }
+  return aNoBlockCrossing ? GetPreviousElementOrTextInBlock(aPoint) :
+                            GetPreviousElementOrText(aPoint);
+}
+
+nsIContent*
 HTMLEditor::GetPreviousEditableHTMLNodeInternal(nsINode& aNode,
                                                 bool aNoBlockCrossing)
 {
   if (!GetActiveEditingHost()) {
     return nullptr;
   }
   return aNoBlockCrossing ? GetPreviousEditableNodeInBlock(aNode) :
                             GetPreviousEditableNode(aNode);
@@ -3721,16 +3753,38 @@ HTMLEditor::GetPreviousEditableHTMLNodeI
   if (!GetActiveEditingHost()) {
     return nullptr;
   }
   return aNoBlockCrossing ? GetPreviousEditableNodeInBlock(aPoint) :
                             GetPreviousEditableNode(aPoint);
 }
 
 nsIContent*
+HTMLEditor::GetNextHTMLElementOrTextInternal(nsINode& aNode,
+                                             bool aNoBlockCrossing)
+{
+  if (!GetActiveEditingHost()) {
+    return nullptr;
+  }
+  return aNoBlockCrossing ? GetNextElementOrTextInBlock(aNode) :
+                            GetNextElementOrText(aNode);
+}
+
+nsIContent*
+HTMLEditor::GetNextHTMLElementOrTextInternal(const EditorRawDOMPoint& aPoint,
+                                             bool aNoBlockCrossing)
+{
+  if (!GetActiveEditingHost()) {
+    return nullptr;
+  }
+  return aNoBlockCrossing ? GetNextElementOrTextInBlock(aPoint) :
+                            GetNextElementOrText(aPoint);
+}
+
+nsIContent*
 HTMLEditor::GetNextEditableHTMLNodeInternal(nsINode& aNode,
                                             bool aNoBlockCrossing)
 {
   if (!GetActiveEditingHost()) {
     return nullptr;
   }
   return aNoBlockCrossing ? GetNextEditableNodeInBlock(aNode) :
                             GetNextEditableNode(aNode);
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -887,16 +887,49 @@ protected:
 
   nsresult RemoveBlockContainer(nsIContent& aNode);
 
   nsIContent* GetPriorHTMLSibling(nsINode* aNode);
 
   nsIContent* GetNextHTMLSibling(nsINode* aNode);
 
   /**
+   * GetPreviousHTMLElementOrText*() methods are similar to
+   * EditorBase::GetPreviousElementOrText*() but this won't return nodes
+   * outside active editing host.
+   */
+  nsIContent* GetPreviousHTMLElementOrText(nsINode& aNode)
+  {
+    return GetPreviousHTMLElementOrTextInternal(aNode, false);
+  }
+  nsIContent* GetPreviousHTMLElementOrTextInBlock(nsINode& aNode)
+  {
+    return GetPreviousHTMLElementOrTextInternal(aNode, true);
+  }
+  nsIContent* GetPreviousHTMLElementOrText(const EditorRawDOMPoint& aPoint)
+  {
+    return GetPreviousHTMLElementOrTextInternal(aPoint, false);
+  }
+  nsIContent*
+  GetPreviousHTMLElementOrTextInBlock(const EditorRawDOMPoint& aPoint)
+  {
+    return GetPreviousHTMLElementOrTextInternal(aPoint, true);
+  }
+
+  /**
+   * GetPreviousHTMLElementOrTextInternal() methods are common implementation
+   * of above methods.  Please don't use this method directly.
+   */
+  nsIContent* GetPreviousHTMLElementOrTextInternal(nsINode& aNode,
+                                                   bool aNoBlockCrossing);
+  nsIContent*
+  GetPreviousHTMLElementOrTextInternal(const EditorRawDOMPoint& aPoint,
+                                       bool aNoBlockCrossing);
+
+  /**
    * GetPreviousEditableHTMLNode*() methods are similar to
    * EditorBase::GetPreviousEditableNode() but this won't return nodes outside
    * active editing host.
    */
   nsIContent* GetPreviousEditableHTMLNode(nsINode& aNode)
   {
     return GetPreviousEditableHTMLNodeInternal(aNode, false);
   }
@@ -920,21 +953,57 @@ protected:
    */
   nsIContent* GetPreviousEditableHTMLNodeInternal(nsINode& aNode,
                                                   bool aNoBlockCrossing);
   nsIContent* GetPreviousEditableHTMLNodeInternal(
                 const EditorRawDOMPoint& aPoint,
                 bool aNoBlockCrossing);
 
   /**
+   * GetNextHTMLElementOrText*() methods are similar to
+   * EditorBase::GetNextElementOrText*() but this won't return nodes outside
+   * active editing host.
+   *
+   * Note that same as EditorBase::GetTextEditableNode(), methods which take
+   * |const EditorRawDOMPoint&| start to search from the node pointed by it.
+   * On the other hand, methods which take |nsINode&| start to search from
+   * next node of aNode.
+   */
+  nsIContent* GetNextHTMLElementOrText(nsINode& aNode)
+  {
+    return GetNextHTMLElementOrTextInternal(aNode, false);
+  }
+  nsIContent* GetNextHTMLElementOrTextInBlock(nsINode& aNode)
+  {
+    return GetNextHTMLElementOrTextInternal(aNode, true);
+  }
+  nsIContent* GetNextHTMLElementOrText(const EditorRawDOMPoint& aPoint)
+  {
+    return GetNextHTMLElementOrTextInternal(aPoint, false);
+  }
+  nsIContent* GetNextHTMLElementOrTextInBlock(const EditorRawDOMPoint& aPoint)
+  {
+    return GetNextHTMLElementOrTextInternal(aPoint, true);
+  }
+
+  /**
+   * GetNextHTMLNodeInternal() methods are common implementation
+   * of above methods.  Please don't use this method directly.
+   */
+  nsIContent* GetNextHTMLElementOrTextInternal(nsINode& aNode,
+                                               bool aNoBlockCrossing);
+  nsIContent* GetNextHTMLElementOrTextInternal(const EditorRawDOMPoint& aPoint,
+                                               bool aNoBlockCrossing);
+
+  /**
    * GetNextEditableHTMLNode*() methods are similar to
    * EditorBase::GetNextEditableNode() but this won't return nodes outside
    * active editing host.
    *
-   * Note that same as EditorBaseGetTextEditableNode(), methods which take
+   * Note that same as EditorBase::GetTextEditableNode(), methods which take
    * |const EditorRawDOMPoint&| start to search from the node pointed by it.
    * On the other hand, methods which take |nsINode&| start to search from
    * next node of aNode.
    */
   nsIContent* GetNextEditableHTMLNode(nsINode& aNode)
   {
     return GetNextEditableHTMLNodeInternal(aNode, false);
   }
--- a/editor/libeditor/tests/mochitest.ini
+++ b/editor/libeditor/tests/mochitest.ini
@@ -250,16 +250,17 @@ skip-if = toolkit == 'android' # bug 131
 [test_bug1355792.html]
 [test_bug1358025.html]
 [test_bug1361008.html]
 [test_bug1368544.html]
 [test_bug1385905.html]
 [test_bug1390562.html]
 [test_bug1394758.html]
 [test_bug1399722.html]
+[test_bug1406726.html]
 [test_bug1409520.html]
 [test_bug1425997.html]
 
 [test_CF_HTML_clipboard.html]
 subsuite = clipboard
 [test_composition_event_created_in_chrome.html]
 [test_contenteditable_focus.html]
 [test_documentCharacterSet.html]
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/tests/test_bug1406726.html
@@ -0,0 +1,126 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1406726
+-->
+<head>
+  <title>Test for Bug 1406726</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1406726">Mozilla Bug 1406726</a>
+<p id="display"></p>
+<div id="editor" contenteditable></div>
+
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 1406726 **/
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(() => {
+  let editor = document.getElementById("editor");
+  let selection = window.getSelection();
+
+  editor.focus();
+  for (let paragraphSeparator of ["div", "p"]) {
+    document.execCommand("defaultParagraphSeparator", false, paragraphSeparator);
+
+    // The result of editor.innerHTML may be wrong in this tests.
+    // Currently, editor wraps following elements of <br> element with default
+    // paragraph separator only when there is only non-editable elements.
+    // This behavior should be standardized by execCommand spec.
+
+    editor.innerHTML = "foo<br>bar<br><span contenteditable=\"false\">baz</span>";
+    selection.collapse(editor.childNodes.item(2), "bar".length);
+    document.execCommand("insertParagraph", false);
+    is(editor.innerHTML, "foo<br>" +
+                         "<" + paragraphSeparator + ">bar</" + paragraphSeparator + ">" +
+                         "<" + paragraphSeparator + "><br></" + paragraphSeparator + ">" +
+                         "<" + paragraphSeparator + "><span contenteditable=\"false\">baz</span></" + paragraphSeparator + ">",
+       "All inline nodes including non-editable <span> element should be wrapped with default paragraph separator, <" + paragraphSeparator + ">");
+    ok(selection.isCollapsed, "Selection should be collapsed");
+    is(selection.anchorNode, editor.childNodes.item(3),
+       "Caret should be in the third line");
+    is(selection.anchorOffset, 0,
+       "Caret should be at start of the third line");
+
+    editor.innerHTML = "foo<br>bar<br><span>baz</span>";
+    selection.collapse(editor.childNodes.item(2), "bar".length);
+    document.execCommand("insertParagraph", false);
+    is(editor.innerHTML, "foo<br>" +
+                         "<" + paragraphSeparator + ">bar</" + paragraphSeparator + ">" +
+                         "<" + paragraphSeparator + "><br></" + paragraphSeparator + ">" +
+                         "<span>baz</span>",
+       "All inline nodes in the second line should be wrapped with default paragraph separator, <" + paragraphSeparator + ">");
+    ok(selection.isCollapsed, "Selection should be collapsed");
+    is(selection.anchorNode, editor.childNodes.item(3),
+       "Caret should be in the third line");
+    is(selection.anchorOffset, 0,
+       "Caret should be at start of the third line");
+
+    editor.innerHTML = "foo<br>bar<br><span contenteditable=\"false\">baz</span>qux";
+    selection.collapse(editor.childNodes.item(2), "bar".length);
+    document.execCommand("insertParagraph", false);
+    is(editor.innerHTML, "foo<br>" +
+                         "<" + paragraphSeparator + ">bar</" + paragraphSeparator + ">" +
+                         "<" + paragraphSeparator + "><br></" + paragraphSeparator + ">" +
+                         "<span contenteditable=\"false\">baz</span>qux",
+       "All inline nodes in the second line should be wrapped with default paragraph separator, <" + paragraphSeparator + ">");
+    ok(selection.isCollapsed, "Selection should be collapsed");
+    is(selection.anchorNode, editor.childNodes.item(3),
+       "Caret should be in the third line");
+    is(selection.anchorOffset, 0,
+       "Caret should be at start of the third line");
+
+    editor.innerHTML = "foo<br>bar<br><span contenteditable=\"false\">baz</span>";
+    selection.collapse(editor.childNodes.item(2), "ba".length);
+    document.execCommand("insertParagraph", false);
+    is(editor.innerHTML, "foo<br>" +
+                         "<" + paragraphSeparator + ">ba</" + paragraphSeparator + ">" +
+                         "<" + paragraphSeparator + ">r</" + paragraphSeparator + ">" +
+                         "<" + paragraphSeparator + "><span contenteditable=\"false\">baz</span></" + paragraphSeparator + ">",
+       "All inline nodes including non-editable <span> element should be wrapped with default paragraph separator, <" + paragraphSeparator + ">");
+    ok(selection.isCollapsed, "Selection should be collapsed");
+    is(selection.anchorNode, editor.childNodes.item(3).firstChild,
+       "Caret should be in the text node in the third line");
+    is(selection.anchorOffset, 0,
+       "Caret should be at start of the text node in the third line");
+
+    editor.innerHTML = "foo<br>bar<br><span>baz</span>";
+    selection.collapse(editor.childNodes.item(2), "ba".length);
+    document.execCommand("insertParagraph", false);
+    is(editor.innerHTML, "foo<br>" +
+                         "<" + paragraphSeparator + ">ba</" + paragraphSeparator + ">" +
+                         "<" + paragraphSeparator + ">r</" + paragraphSeparator + ">" +
+                         "<span>baz</span>",
+       "All inline nodes in the second line should be wrapped with default paragraph separator, <" + paragraphSeparator + ">");
+    ok(selection.isCollapsed, "Selection should be collapsed");
+    is(selection.anchorNode, editor.childNodes.item(3).firstChild,
+       "Caret should be in the text node in the third line");
+    is(selection.anchorOffset, 0,
+       "Caret should be at start of the text node in the third line");
+
+    editor.innerHTML = "foo<br>bar<br><span contenteditable=\"false\">baz</span>qux";
+    selection.collapse(editor.childNodes.item(2), "ba".length);
+    document.execCommand("insertParagraph", false);
+    is(editor.innerHTML, "foo<br>" +
+                         "<" + paragraphSeparator + ">ba</" + paragraphSeparator + ">" +
+                         "<" + paragraphSeparator + ">r</" + paragraphSeparator + ">" +
+                         "<span contenteditable=\"false\">baz</span>qux",
+       "All inline nodes in the second line should be wrapped with default paragraph separator, <" + paragraphSeparator + ">");
+    ok(selection.isCollapsed, "Selection should be collapsed");
+    is(selection.anchorNode, editor.childNodes.item(3).firstChild,
+       "Caret should be in the text node in the third line");
+    is(selection.anchorOffset, 0,
+       "Caret should be at start of the text node in the third line");
+  }
+
+  SimpleTest.finish();
+});
+
+</script>
+</pre>
+</body>
+</html>