Algorithm for deciding if a frame's selection should be hidden

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 c29027dd0a1dd11bf29fa63735e9e3ee60b32b52..6acb2f467adac08e7de60c5c9ea74cbbff48f81a 100644
--- a/third_party/WebKit/Source/core/editing/FrameSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/FrameSelection.cpp
@@ -250,7 +250,7 @@ void FrameSelection::DidSetSelectionDeprecated(SetSelectionOptions options,
   }
 
   frame_caret_->StopCaretBlinkTimer();
-  UpdateAppearance(LayoutSelection::PaintHint::kPaint);
+  UpdateAppearance();
 
   // Always clear the x position used for vertical arrow navigation.
   // It will be restored by the vertical arrow navigation code if necessary.
@@ -335,26 +335,7 @@ void FrameSelection::DidChangeFocus() {
   // Hits in
   // virtual/gpu/compositedscrolling/scrollbars/scrollbar-miss-mousemove-disabled.html
   DisableCompositingQueryAsserts disabler;
-
-  // No focused element means document root has focus.
-  const Element* const focus = GetDocument().FocusedElement()
-                                   ? GetDocument().FocusedElement()
-                                   : GetDocument().documentElement();
-
-  // Protection against LayoutTests/editing/selection/selection-crash.html
-  if (!focus) {
-    frame_caret_->ScheduleVisualUpdateForPaintInvalidationIfNeeded();
-    text_control_focused_ = false;
-    return;
-  }
-
-  // Hide the selection when focus goes away from a text-field and into
-  // something that is not a text-field. When focus enters another text-field we
-  // do not need to update appearance; the appearance is updated when the new
-  // selection is set.
-  if (text_control_focused_ && !focus->IsTextControl())
-    UpdateAppearance(LayoutSelection::PaintHint::kHide);
-  text_control_focused_ = focus->IsTextControl();
+  UpdateAppearance();
 }
 
 static DispatchEventResult DispatchSelectStart(
@@ -444,6 +425,61 @@ void FrameSelection::Clear() {
   SetSelection(SelectionInDOMTree());
 }
 
+bool FrameSelection::IsActive() const {
+  const Node* start = ComputeVisibleSelectionInDOMTreeDeprecated()
+                          .Start()
+                          .ComputeContainerNode();
+  if (!start)
+    return false;
+
+  // No focused element means document root has focus.
+  const Element* const focus_element = GetDocument().FocusedElement()

[Xiaocheng, 2017/05/02 20:05:16]

nit: s/focus_element/focused_element/g

[hugoh_UTC2, 2017/05/03 07:18:55]

OK, I will change this.

+                                           ? GetDocument().FocusedElement()
+                                           : GetDocument().documentElement();
+  if (!focus_element)
+    return false;

[Xiaocheng, 2017/05/02 20:05:17]

Does LayoutTests/editing/selection/selection-crash.html reach here? If so,
please state that here.

[hugoh_UTC2, 2017/05/03 07:18:55]

selection-crash.html doesn't crash right now but so far IsActive is only called
from LayoutSelection. Once others start to call IsActive, we would definitely
need this safety.

+
+  if (focus_element->IsTextControl())
+    return focus_element->ContainsIncludingHostElements(*start);
+
+  bool has_editable_style = HasEditableStyle(*start);
+  const Node* current = start;
+  do {

[Xiaocheng, 2017/05/02 20:05:16]

nit: A |for| loop seems to have better readability:

for (const Node* current = start; current; current =
current->ParentOrShadowHostNode()) {
  ...
}

[hugoh_UTC2, 2017/05/03 07:18:55]

Not sure I agree :) A for-loop would check |current| one extra time (we already
know it is true in the first round).

[Xiaocheng, 2017/05/03 15:03:01]

Compared to the loop body, an extra nullptr check should have almost no cost.

Anyway, this is minor.

+    if (has_editable_style && !HasEditableStyle(*current)) {

[Xiaocheng, 2017/05/02 20:05:16]

I can't understand this loop body. There seem to be multiple factors mixed
together controlling the output:

1) Whether the selection is in text control or not
2) Whether the selection is editable or not
3) Whether the focused element |IsShadowInclduingInclusiveAncestorOf| the
selection
4) Whether the ancestor or shadow host chain of the selection (bounded by the
focused element if it's in the chain) crosses editing boundary

Can some checks be moved out of the loop, so that the loop body becomes lighter
and the relation between the factors and the output becomes clearer?

[hugoh_UTC2, 2017/05/03 07:18:55]

Yes, we have many cases to think of. This logic is the simplest I could come up
with that covers them all.
Hmm. The reason for having the checks inside the loop body is that I wanna walk
the chain of parents only once.

[hugoh_UTC2, 2017/05/03 20:31:00]

I realized that EnclosingTextControl() and the check for text control could be
moved outside the loop body. See PS7.

+      // We exited an editable sub tree.
+      // Only range selections can be active in this case.
+      if (!GetSelectionInDOMTree().IsRange()) {
+        // Example that triggers this:
+        // <div tabindex='-1' id='parent'>
+        //   <!-- Place caret within blabla. -->
+        //   <div contenteditable id='ce'>blabla</div>
+        // </div>
+        // When focusing the outer <div>, we must hide the inner <div>'s caret.
+        return false;
+      }
+
+      // So the selection is a range...
+      // Only hide the selection if its inside a text control.
+      const TextControlElement* in_text_control =
+          EnclosingTextControl(GetSelectionInDOMTree().Base());
+      if (in_text_control) {
+        // Example that triggers this:
+        // <div tabindex="-1">
+        //   <input value="hola"> <!-- Select hola -->
+        // </div>
+        // When focusing the <div>, we must hide the selection [hola].
+        return false;
+      }
+    } else {
+      // Selection is active (= not hidden) if this sub tree got focus.
+      if (current == focus_element)
+        return true;
+    }
+    current = current->ParentOrShadowHostNode();
+  } while (current);
+
+  return true;
+}
 void FrameSelection::DocumentAttached(Document* document) {
   DCHECK(document);
   use_secure_keyboard_entry_when_active_ = false;
@@ -796,15 +832,13 @@ void FrameSelection::CommitAppearanceIfNeeded() {
 }
 
 void FrameSelection::DidLayout() {
-  // Upon relayout, a hidden selection must be kept hidden and a visible
-  // selection must be kept visible.
-  UpdateAppearance(LayoutSelection::PaintHint::kKeep);
+  UpdateAppearance();
 }
 
-void FrameSelection::UpdateAppearance(LayoutSelection::PaintHint hint) {
+void FrameSelection::UpdateAppearance() {
   DCHECK(!frame_->ContentLayoutItem().IsNull());
   frame_caret_->ScheduleVisualUpdateForPaintInvalidationIfNeeded();
-  layout_selection_->SetHasPendingSelection(hint);
+  layout_selection_->SetHasPendingSelection();
 }
 
 void FrameSelection::NotifyLayoutObjectOfSelectionChange(
@@ -993,7 +1027,7 @@ void FrameSelection::RevealSelection(const ScrollAlignment& alignment,
       document_loader->GetInitialScrollState().was_scrolled_by_user = true;
     if (start.AnchorNode()->GetLayoutObject()->ScrollRectToVisible(
             rect, alignment, alignment))
-      UpdateAppearance(LayoutSelection::PaintHint::kPaint);
+      UpdateAppearance();
   }
 }