Invalidate the previous caret even if the parent text node is removed.

third_party/WebKit/Source/core/editing/FrameSelection.cpp

Index: third_party/WebKit/Source/core/editing/FrameSelection.cpp
diff --git a/third_party/WebKit/Source/core/editing/FrameSelection.cpp b/third_party/WebKit/Source/core/editing/FrameSelection.cpp
index b51173b7bff61bb2ec2393b037a0360637a3abdf..cc2c54f34ed175fcc29b0dcd9799c7753d5c105c 100644
--- a/third_party/WebKit/Source/core/editing/FrameSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/FrameSelection.cpp
@@ -634,6 +634,7 @@ void FrameSelection::prepareForDestruction()
     setSelection(VisibleSelection(), CloseTyping | ClearTypingStyle | DoNotUpdateAppearance);
     m_selectionEditor->dispose();
     m_previousCaretNode.clear();
+    m_parentOfPreviousCaretNode.clear();
 }
 
 void FrameSelection::setStart(const VisiblePosition &pos, EUserTriggered trigger)
@@ -713,11 +714,20 @@ void FrameSelection::invalidateCaretRect()
         return;
 
     LayoutView* view = m_frame->document()->layoutView();
-    if (m_previousCaretNode && (shouldRepaintCaret(*m_previousCaretNode) || shouldRepaintCaret(view)))
+    if (m_previousCaretNode && (shouldRepaintCaret(*m_previousCaretNode) || shouldRepaintCaret(view))) {
         invalidateLocalCaretRect(m_previousCaretNode.get(), m_previousCaretRect);
-    if (newNode && (shouldRepaintCaret(*newNode) || shouldRepaintCaret(view)))
+        m_previousCaretRect = LayoutRect();
+    }
+    if (newNode && (shouldRepaintCaret(*newNode) || shouldRepaintCaret(view))) {
         invalidateLocalCaretRect(newNode, newRect);
+        if (!m_previousCaretRect.isEmpty() && m_previousCaretNode && !m_previousCaretNode->parentNode()
+            && newNode == m_parentOfPreviousCaretNode.get()) {

[dshwang, 2016/02/05 10:25:47]

It means |newNode| is also detached from document. If so,
invalidateLocalCaretRect() do nothing.
the condition checks m_previousCaretNode->parentNode()
Could you review again? I improve little more.

If we decide refactoring wide range of code, I have a idea.
All editable element must has anonymous #text node from creation time. However,
there might be unexpected side effect. e.g. when javascript sets 'editable' to
any element, Blink has to create and add additional #text node for box element,
while inline element become editable by itself. caretBrowsingEnabled is also big
obstacle. To be honest, I'm a bit afraid to touch well-working legacy editor
code without concrete knowledge.

[yosin_UTC9, 2016/02/12 03:53:13]

I would like to have radical change. Current implementation of Caret handling
doesn't match rendering pipeline concept.
We don't have clear idea how caret to be implemented, but we have some idea:
 - paint caret in its own layer; we don't need to remember previous rectangle(?)
 - calculate caret height based on a character will be inserted
 - integrated into rendering pipeline and live with lifecycle == calculation of
caret rectangle just before LayoutClean
 - ... and more ...

It is a time to think about caret re-implementation. I would like to leave
current code as is until we have concrete plan.
 

+            // Must invalidate #text even if it's detached. crbug.com/528922
+            invalidateLocalCaretRect(newNode, m_previousCaretRect);
+        }
+    }
     m_previousCaretNode = newNode;
+    m_parentOfPreviousCaretNode = newNode ? newNode->parentNode() : nullptr;
     m_previousCaretRect = newRect;
     m_previousCaretVisibility = caretVisibility();
 }
@@ -1328,6 +1338,7 @@ DEFINE_TRACE(FrameSelection)
     visitor->trace(m_originalBase);
     visitor->trace(m_originalBaseInComposedTree);
     visitor->trace(m_previousCaretNode);
+    visitor->trace(m_parentOfPreviousCaretNode);
     visitor->trace(m_typingStyle);
 }