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

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

 /*
  * Copyright (C) 2004, 2008, 2009, 2010 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 232 matching lines @@
     // "FocusIn", |m_frame| may associate to another document.
     if (!IsAvailable() || GetDocument() != current_document) {
       // Once we get test case to reach here, we should change this
       // if-statement to |DCHECK()|.
       NOTREACHED();
       return;
     }
   }
 
   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.
   x_pos_for_vertical_arrow_navigation_ = NoXPosForVerticalArrowNavigation();
 
   // TODO(yosin): Can we move this to at end of this function?
   // This may dispatch a synchronous focus-related events.
   if (!(options & kDoNotSetFocus)) {
     SelectFrameElementInParentIfFullySelected();
     if (!IsAvailable() || GetDocument() != current_document) {
@@ skipping 64 matching lines @@
   // TODO(yosin): We should move to call |TypingCommand::closeTyping()| to
   // |Editor| class.
   if (!GetDocument().IsRunningExecCommand())
     TypingCommand::CloseTyping(frame_);
 }
 
 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(
     const VisibleSelection& selection) {
   Node* select_start_target = selection.Extent().ComputeContainerNode();
   if (!select_start_target)
     return DispatchEventResult::kNotCanceled;
 
   return select_start_target->DispatchEvent(
       Event::CreateCancelableBubble(EventTypeNames::selectstart));
@@ skipping 69 matching lines @@
   return true;
 }
 
 void FrameSelection::Clear() {
   granularity_ = kCharacterGranularity;
   if (granularity_strategy_)
     granularity_strategy_->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;
   selection_editor_->DocumentAttached(document);
   SetContext(document);
 }
 
 void FrameSelection::ContextDestroyed(Document* document) {
   granularity_ = kCharacterGranularity;
 
@@ skipping 332 matching lines @@
 
 bool FrameSelection::NeedsLayoutSelectionUpdate() const {
   return layout_selection_->HasPendingSelection();
 }
 
 void FrameSelection::CommitAppearanceIfNeeded() {
   return layout_selection_->Commit();
 }
 
 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(
     EUserTriggered user_triggered) {
   TextControlElement* text_control =
       EnclosingTextControl(GetSelectionInDOMTree().Base());
   if (!text_control)
     return;
   text_control->SelectionChanged(user_triggered == kUserTriggered);
 }
@@ skipping 168 matching lines @@
 
   Position start = ComputeVisibleSelectionInDOMTreeDeprecated().Start();
   DCHECK(start.AnchorNode());
   if (start.AnchorNode() && start.AnchorNode()->GetLayoutObject()) {
     // FIXME: This code only handles scrolling the startContainer's layer, but
     // the selection rect could intersect more than just that.
     if (DocumentLoader* document_loader = frame_->Loader().GetDocumentLoader())
       document_loader->GetInitialScrollState().was_scrolled_by_user = true;
     if (start.AnchorNode()->GetLayoutObject()->ScrollRectToVisible(
             rect, alignment, alignment))
-      UpdateAppearance(LayoutSelection::PaintHint::kPaint);
+      UpdateAppearance();
   }
 }
 
 void FrameSelection::SetSelectionFromNone() {
   // Put a caret inside the body if the entire frame is editable (either the
   // entire WebView is editable or designMode is on for this document).
 
   Document* document = frame_->GetDocument();
   if (!ComputeVisibleSelectionInDOMTreeDeprecated().IsNone() ||
       !(blink::HasEditableStyle(*document)))
@@ skipping 178 matching lines @@
 }
 
 void showTree(const blink::FrameSelection* sel) {
   if (sel)
     sel->ShowTreeForThis();
   else
     LOG(INFO) << "Cannot showTree for <null> FrameSelection.";
 }
 
 #endif