Selection API: Mutating a Range object after adding it to Selection should update Selection attribu…

third_party/WebKit/Source/core/dom/Range.cpp

Index: third_party/WebKit/Source/core/dom/Range.cpp
diff --git a/third_party/WebKit/Source/core/dom/Range.cpp b/third_party/WebKit/Source/core/dom/Range.cpp
index 3187a1e9c746f3eef49a70065a484b4e3f5c92da..8ee35a6d8c63fe8858d9fc72dc8f7b96cae11e2a 100644
--- a/third_party/WebKit/Source/core/dom/Range.cpp
+++ b/third_party/WebKit/Source/core/dom/Range.cpp
@@ -36,6 +36,8 @@
 #include "core/dom/ProcessingInstruction.h"
 #include "core/dom/Text.h"
 #include "core/editing/EditingUtilities.h"
+#include "core/editing/EphemeralRange.h"
+#include "core/editing/FrameSelection.h"
 #include "core/editing/VisiblePosition.h"
 #include "core/editing/VisibleUnits.h"
 #include "core/editing/iterators/TextIterator.h"
@@ -184,6 +186,8 @@ void Range::setStart(Node* refNode,
 
   if (didMoveDocument || checkForDifferentRootContainer(m_start, m_end))

[yosin_UTC9, 2017/02/10 10:26:27]

nit: Can we use early-return style?

if (didMoveDocument || checkForDifferentRootContainer(m_start, m_end)) {
  collapse(true);
  return;
}
updateSelectionIfAddedToSelection();

Should we detach Range from FrameSelection when Range move to another Document?

[tkent, 2017/02/13 04:04:13]

Good point.  I'll investigate Firefox/Edge behavior.

[tkent, 2017/02/13 05:45:29]

Both of Firefox and Edge keep Selection.getRangeAt(0) even if the Range selects
detached Node or a Node owned by a different Document, and IDL attributes such
as Selection.anchorNode return values based on the Range.

Firefox clears visible text selection, and Edge keeps visible text selection
base on the previous value.  I think Firefox behavior is reasonable because
there are no ways to operate the visible text selection.
I'll update this CL.

[tkent, 2017/02/13 07:31:40]

We discussed offline, and agreed that we should unregister the Range from the
Selection at this moment.

     collapse(true);
+  else
+    updateSelectionIfAddedToSelection();
 }
 
 void Range::setEnd(Node* refNode, int offset, ExceptionState& exceptionState) {
@@ -208,6 +212,8 @@ void Range::setEnd(Node* refNode, int offset, ExceptionState& exceptionState) {
 
   if (didMoveDocument || checkForDifferentRootContainer(m_start, m_end))

[yosin_UTC9, 2017/02/10 10:26:27]

Similar to L187 in Range::setStart()

[tkent, 2017/02/13 07:31:40]

Done.

     collapse(false);
+  else
+    updateSelectionIfAddedToSelection();
 }
 
 void Range::setStart(const Position& start, ExceptionState& exceptionState) {
@@ -227,6 +233,7 @@ void Range::collapse(bool toStart) {
     m_end = m_start;
   else
     m_start = m_end;
+  updateSelectionIfAddedToSelection();
 }
 
 bool Range::isNodeFullyContained(Node& node) const {
@@ -1218,6 +1225,7 @@ void Range::selectNodeContents(Node* refNode, ExceptionState& exceptionState) {
 
   m_start.setToStartOfNode(*refNode);
   m_end.setToEndOfNode(*refNode);
+  updateSelectionIfAddedToSelection();
 }
 
 bool Range::selectNodeContents(Node* refNode, Position& start, Position& end) {
@@ -1700,6 +1708,16 @@ FloatRect Range::boundingRect() const {
   return result;
 }
 
+void Range::updateSelectionIfAddedToSelection() {
+  if (!ownerDocument().frame())
+    return;
+  FrameSelection& selection = ownerDocument().frame()->selection();
+  if (this != selection.documentCachedRange())
+    return;
+  selection.setSelectedRange(EphemeralRange(this), VP_DEFAULT_AFFINITY);
+  selection.cacheRangeOfDocument(this);
+}
+
 DEFINE_TRACE(Range) {
   visitor->trace(m_ownerDocument);
   visitor->trace(m_start);