Changeset 267329 in webkit

Timestamp:

Sep 20, 2020, 12:37:43 PM (6 years ago)

Author:

Darin Adler

Message:

Selection API: Further improvements to VisibleSelection, FrameSelection, and DOMSelection to preserve anchor and focus
​https://bugs.webkit.org/show_bug.cgi?id=216739

Reviewed by Ryosuke Niwa.

Source/WebCore:

If done correctly, these changes should have almost no effect unless live range selection
is enabled. This adds a feature where VisibleSelection tracks the original points,
before the VisiblePosition-style canonicalization. It also starts a transition from
the base/extent terminology to the slightly-easier-to-understand anchor/focus terminology
that is used in the selection specification. New functions name them anchor and focus,
even though we still have some old ones that call them base and extent. In addition,
made some other small related improvements to live range selection mode, without changing
behavior when not in that mode. Have been testing these locally by turning on that mode
and running various tests.

• editing/FrameSelection.cpp:

(WebCore::FrameSelection::isInDocumentTree const): Added. Used by DOMSelection to
correctly handle cases where the selection is in a shadow tree without requiring it
actually create the associated live range just to query.
(WebCore::FrameSelection::associatedLiveRange): Use the new VisibleSelection::range,
which gives us non-canonicalized points, rather than the deprecated
VisibleSelection::firstRange, which we will eventually be renaming or deleting.
(WebCore::FrameSelection::updateFromAssociatedLiveRange): Use
makeContainerOffsetPosition instead of implicitly using makeDeprecatedLegacyPosition.
Also removed the FIXME about normalization of endpoints, which is what this patch
mostly addresses; no need to track with a comment now.
(WebCore::FrameSelection::updateAssociatedLiveRange): Use VisibleSelection::range
for the same reason as above.

• editing/FrameSelection.h: Added isInDocumentTree.

• editing/VisibleSelection.cpp:

(WebCore::VisibleSelection::VisibleSelection): Updated the primary constructor to set
m_anchor/focus and let the validate function set m_base/extent/start/end/baseIsFirst.
Changed most other constructors to call that primary one.
(WebCore::VisibleSelection::anchor const): Added. This is a non-canonicalized position.
(WebCore::VisibleSelection::focus const): Ditto.
(WebCore::VisibleSelection::uncanonicalizedStart const): Added. This non-canonicalized
version of start is used by new DOMSelection code, but eventually should be used
elsewhere as well, and we may eventually eliminate the canonicalized version.
(WebCore::VisibleSelection::uncanonicalizedEnd const): Ditto.
(WebCore::VisibleSelection::range const): Added. This non-canonicalized version of
firstRange has many of the same considerations as the other four functions above.
(WebCore::VisibleSelection::setBase): Set m_anchor instead of m_base. Eventually we
will want to rename this function, too, but there is no need to do that now.
(WebCore::VisibleSelection::setExtent): Ditto.
(WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents): Rewrote to take
m_anchor and m_focus as inputs and now m_base and m_extent are pure outputs.
May want to rename this later, because "deep equivalents" is a funny way to say
that this function sets base and extent to canonicalized values.
(WebCore::VisibleSelection::adjustSelectionRespectingGranularity): Renamed from
setStartAndEndFromBaseAndExtentRespectingGranularity. This now takes m_start/end
as input as well as output, because the validate function needs to check
if the function changes m_start/end so it can update m_anchor/focus. Also did a tiny
bit of refactoring to use std::swap.
(WebCore::VisibleSelection::validate): Refined the code to handle canonicalization
differently from expanding due to granularity. Canonicalization is done preserving
the original m_anchor/focus as it mostly always has been. But expansion due to
granularity now changes m_anchor/focus/base/extent rather than just m_start/end.
This makes sense because granularity changes actually affect what is selected,
while canonicalization just changes how the selection is expressed and tracked.
(WebCore::VisibleSelection::setWithoutValidation): Set m_anchor/focus. In the future,
we can probably get rid of this eventually, because the "validation" talked about
here is the unwanted canonicalization. But that's a refinement for the future.
(WebCore::VisibleSelection::adjustSelectionToAvoidCrossingShadowBoundaries):
Update m_focus, not just m_extent, if we have to adjust. Also removed an unnnecessary
null check from the start of the function.
(WebCore::VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries): Ditto.
Also removed the assertions because these situations can indeed arise and it's not
necessarily an indication of a bug in editing code.

• editing/VisibleSelection.h: Rearranged constructors to make it slightly more clear

which is the main one and that the others are simply convenience shorthands. Might
want to come back later and remove some unused ones. Added uncanonicalizedStart,
uncanonicalizedEnd, anchor, focus, and range. Renamed the private function called
setStartAndEndFromBaseAndExtentRespectingGranularity to the new name
adjustSelectionRespectingGranularity. Added m_anchor and m_focus and refined the
comments about the position data members. We should eventually be able to cut down
on the number of these again, but it's fine to have two more for now.

• page/DOMSelection.cpp:

(WebCore::DOMSelection::range const): Use range rather than firstRange when live
range selection is enabled, for proper round-tripping of selection endpoints,
unaffected by canonicalization. The other changes below are for the same reason.
(WebCore::DOMSelection::anchorPosition const): Use anchor.
(WebCore::DOMSelection::focusPosition const): Use focus.
(WebCore::DOMSelection::basePosition const): Use anchor.
(WebCore::DOMSelection::extentPosition const): Use focus.
(WebCore::DOMSelection::type const): Use isInDocumentTree and range rather than
isNone/isCaret/isRange when live range selection is enabled, because this needs
to report "Caret" when the range is not collapsed, even if the canonicalized
range is collapsed and "None" when the selection is in a shadow tree.
(WebCore::DOMSelection::rangeCount const): Use isInDocumentTree to check if
there is a selection rather than isNone to correctly handle the shadow tree case.
(WebCore::DOMSelection::collapseToEnd): Use uncanonicalizedEnd.
(WebCore::DOMSelection::collapseToStart): Use uncanonicalizedStart.
(WebCore::DOMSelection::containsNode const): Updated the comment for clarity.

LayoutTests:

• editing/execCommand/insert-list-nested-with-orphaned-expected.txt: This test

result reflected incorrrect behavior where the selection changed from a range
to a caret in a certain unusual case. The new behavior of leaving the text
selected is consistent with how the command works in normal cases, and was an
accidental progression from the improvements to the selection code. Was able
to reproduce this all in Safari and after experimenting with different
selections, it's clear this is an improvement.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/execCommand/insert-list-nested-with-orphaned-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/FrameSelection.cpp (modified) (2 diffs)
• Source/WebCore/editing/FrameSelection.h (modified) (1 diff)
• Source/WebCore/editing/VisibleSelection.cpp (modified) (16 diffs)
• Source/WebCore/editing/VisibleSelection.h (modified) (6 diffs)
• Source/WebCore/page/DOMSelection.cpp (modified) (10 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-09-20  Darin Adler  <darin@apple.com>
+
+        Selection API: Further improvements to VisibleSelection, FrameSelection, and DOMSelection to preserve anchor and focus
+        https://bugs.webkit.org/show_bug.cgi?id=216739
+
+        Reviewed by Ryosuke Niwa.
+
+        * editing/execCommand/insert-list-nested-with-orphaned-expected.txt: This test
+        result reflected incorrrect behavior where the selection changed from a range
+        to a caret in a certain unusual case. The new behavior of leaving the text
+        selected is consistent with how the command works in normal cases, and was an
+        accidental progression from the improvements to the selection code. Was able
+        to reproduce this all in Safari and after experimenting with different
+        selections, it's clear this is an improvement.
+
 2020-09-20  Zalan Bujtas  <zalan@apple.com>
 

trunk/LayoutTests/editing/execCommand/insert-list-nested-with-orphaned-expected.txt

@@ -18 +18 @@
 |     <ol>
 |       <li>
-|         "<#selection-caret>because of you"
+|         "because of you"
 |   "
     "

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-09-20  Darin Adler  <darin@apple.com>
+
+        Selection API: Further improvements to VisibleSelection, FrameSelection, and DOMSelection to preserve anchor and focus
+        https://bugs.webkit.org/show_bug.cgi?id=216739
+
+        Reviewed by Ryosuke Niwa.
+
+        If done correctly, these changes should have almost no effect unless live range selection
+        is enabled. This adds a feature where VisibleSelection tracks the original points,
+        before the VisiblePosition-style canonicalization. It also starts a transition from
+        the base/extent terminology to the slightly-easier-to-understand anchor/focus terminology
+        that is used in the selection specification. New functions name them anchor and focus,
+        even though we still have some old ones that call them base and extent. In addition,
+        made some other small related improvements to live range selection mode, without changing
+        behavior when not in that mode. Have been testing these locally by turning on that mode
+        and running various tests.
+
+        * editing/FrameSelection.cpp:
+        (WebCore::FrameSelection::isInDocumentTree const): Added. Used by DOMSelection to
+        correctly handle cases where the selection is in a shadow tree without requiring it
+        actually create the associated live range just to query.
+        (WebCore::FrameSelection::associatedLiveRange): Use the new VisibleSelection::range,
+        which gives us non-canonicalized points, rather than the deprecated
+        VisibleSelection::firstRange, which we will eventually be renaming or deleting.
+        (WebCore::FrameSelection::updateFromAssociatedLiveRange): Use
+        makeContainerOffsetPosition instead of implicitly using makeDeprecatedLegacyPosition.
+        Also removed the FIXME about normalization of endpoints, which is what this patch
+        mostly addresses; no need to track with a comment now.
+        (WebCore::FrameSelection::updateAssociatedLiveRange): Use VisibleSelection::range
+        for the same reason as above.
+
+        * editing/FrameSelection.h: Added isInDocumentTree.
+
+        * editing/VisibleSelection.cpp:
+        (WebCore::VisibleSelection::VisibleSelection): Updated the primary constructor to set
+        m_anchor/focus and let the validate function set m_base/extent/start/end/baseIsFirst.
+        Changed most other constructors to call that primary one.
+        (WebCore::VisibleSelection::anchor const): Added. This is a non-canonicalized position.
+        (WebCore::VisibleSelection::focus const): Ditto.
+        (WebCore::VisibleSelection::uncanonicalizedStart const): Added. This non-canonicalized
+        version of start is used by new DOMSelection code, but eventually should be used
+        elsewhere as well, and we may eventually eliminate the canonicalized version.
+        (WebCore::VisibleSelection::uncanonicalizedEnd const): Ditto.
+        (WebCore::VisibleSelection::range const): Added. This non-canonicalized version of
+        firstRange has many of the same considerations as the other four functions above.
+        (WebCore::VisibleSelection::setBase): Set m_anchor instead of m_base. Eventually we
+        will want to rename this function, too, but there is no need to do that now.
+        (WebCore::VisibleSelection::setExtent): Ditto.
+        (WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents): Rewrote to take
+        m_anchor and m_focus as inputs and now m_base and m_extent are pure outputs.
+        May want to rename this later, because "deep equivalents" is a funny way to say
+        that this function sets base and extent to canonicalized values.
+        (WebCore::VisibleSelection::adjustSelectionRespectingGranularity): Renamed from
+        setStartAndEndFromBaseAndExtentRespectingGranularity. This now takes m_start/end
+        as input as well as output, because the validate function needs to check
+        if the function changes m_start/end so it can update m_anchor/focus. Also did a tiny
+        bit of refactoring to use std::swap.
+        (WebCore::VisibleSelection::validate): Refined the code to handle canonicalization
+        differently from expanding due to granularity. Canonicalization is done preserving
+        the original m_anchor/focus as it mostly always has been. But expansion due to
+        granularity now changes m_anchor/focus/base/extent rather than just m_start/end.
+        This makes sense because granularity changes actually affect what is selected,
+        while canonicalization just changes how the selection is expressed and tracked.
+        (WebCore::VisibleSelection::setWithoutValidation): Set m_anchor/focus. In the future,
+        we can probably get rid of this eventually, because the "validation" talked about
+        here is the unwanted canonicalization. But that's a refinement for the future.
+        (WebCore::VisibleSelection::adjustSelectionToAvoidCrossingShadowBoundaries):
+        Update m_focus, not just m_extent, if we have to adjust. Also removed an unnnecessary
+        null check from the start of the function.
+        (WebCore::VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries): Ditto.
+        Also removed the assertions because these situations can indeed arise and it's not
+        necessarily an indication of a bug in editing code.
+
+        * editing/VisibleSelection.h: Rearranged constructors to make it slightly more clear
+        which is the main one and that the others are simply convenience shorthands. Might
+        want to come back later and remove some unused ones. Added uncanonicalizedStart,
+        uncanonicalizedEnd, anchor, focus, and range. Renamed the private function called
+        setStartAndEndFromBaseAndExtentRespectingGranularity to the new name
+        adjustSelectionRespectingGranularity. Added m_anchor and m_focus and refined the
+        comments about the position data members. We should eventually be able to cut down
+        on the number of these again, but it's fine to have two more for now.
+
+        * page/DOMSelection.cpp:
+        (WebCore::DOMSelection::range const): Use range rather than firstRange when live
+        range selection is enabled, for proper round-tripping of selection endpoints,
+        unaffected by canonicalization. The other changes below are for the same reason.
+        (WebCore::DOMSelection::anchorPosition const): Use anchor.
+        (WebCore::DOMSelection::focusPosition const): Use focus.
+        (WebCore::DOMSelection::basePosition const): Use anchor.
+        (WebCore::DOMSelection::extentPosition const): Use focus.
+        (WebCore::DOMSelection::type const): Use isInDocumentTree and range rather than
+        isNone/isCaret/isRange when live range selection is enabled, because this needs
+        to report "Caret" when the range is not collapsed, even if the canonicalized
+        range is collapsed and "None" when the selection is in a shadow tree.
+        (WebCore::DOMSelection::rangeCount const): Use isInDocumentTree to check if
+        there is a selection rather than isNone to correctly handle the shadow tree case.
+        (WebCore::DOMSelection::collapseToEnd): Use uncanonicalizedEnd.
+        (WebCore::DOMSelection::collapseToStart): Use uncanonicalizedStart.
+        (WebCore::DOMSelection::containsNode const): Updated the comment for clarity.
+
 2020-09-20  Simon Fraser  <simon.fraser@apple.com>
 

trunk/Source/WebCore/editing/FrameSelection.cpp

@@ -2787 +2787 @@
 }
 
+bool FrameSelection::isInDocumentTree() const
+{
+    return containsEndpoints(m_document, m_selection.range());
+}
+
 RefPtr<Range> FrameSelection::associatedLiveRange()
 {
     if (!m_associatedLiveRange) {
-        if (auto range = m_selection.firstRange(); containsEndpoints(m_document, range)) {
+        if (auto range = m_selection.range(); containsEndpoints(m_document, range)) {
             m_associatedLiveRange = createLiveRange(*range);
             m_associatedLiveRange->didAssociateWithSelection();
@@ -2817 +2822 @@
     if (!containsEndpoints(m_document, *m_associatedLiveRange))
         disassociateLiveRange();
-    else
-        setSelection(makeSimpleRange(*m_associatedLiveRange));
-    // FIXME: Normalization done by setSelection will be visible next time updateAssociatedLiveRange is called. Instead the Selection API specification calls for allowing non-normalized selection ranges.
+    else {
+        // Don't use VisibleSelection's constructor that takes a SimpleRange, because it uses makeDeprecatedLegacyPosition instead of makeContainerOffsetPosition.
+        auto start = makeContainerOffsetPosition(&m_associatedLiveRange->startContainer(), m_associatedLiveRange->startOffset());
+        auto end = makeContainerOffsetPosition(&m_associatedLiveRange->endContainer(), m_associatedLiveRange->endOffset());
+        setSelection({ start, end });
+    }
 }
 
 void FrameSelection::updateAssociatedLiveRange()
 {
-    auto range = m_selection.firstRange();
+    auto range = m_selection.range();
     if (!containsEndpoints(m_document, range)) {
         // The selection was cleared or is now within a shadow tree.

trunk/Source/WebCore/editing/FrameSelection.h

@@ -252 +252 @@
     void setShouldShowBlockCursor(bool);
 
+    bool isInDocumentTree() const;
     RefPtr<Range> associatedLiveRange();
     void associateLiveRange(Range&);

trunk/Source/WebCore/editing/VisibleSelection.cpp

@@ -50 +50 @@
 }
 
-VisibleSelection::VisibleSelection(const Position& position, Affinity affinity, bool isDirectional)
-    : m_base(position)
-    , m_extent(position)
+VisibleSelection::VisibleSelection(const Position& anchor, const Position& focus, Affinity affinity, bool isDirectional)
+    : m_anchor(anchor)
+    , m_focus(focus)
     , m_affinity(affinity)
     , m_isDirectional(isDirectional)
@@ -59 +59 @@
 }
 
-VisibleSelection::VisibleSelection(const Position& base, const Position& extent, Affinity affinity, bool isDirectional)
-    : m_base(base)
-    , m_extent(extent)
-    , m_affinity(affinity)
-    , m_isDirectional(isDirectional)
-{
-    validate();
+VisibleSelection::VisibleSelection(const Position& position, Affinity affinity, bool isDirectional)
+    : VisibleSelection(position, position, affinity, isDirectional)
+{
 }
 
 VisibleSelection::VisibleSelection(const VisiblePosition& position, bool isDirectional)
-    : m_base(position.deepEquivalent())
-    , m_extent(position.deepEquivalent())
-    , m_affinity(position.affinity())
-    , m_isDirectional(isDirectional)
-{
-    validate();
-}
-
-VisibleSelection::VisibleSelection(const VisiblePosition& base, const VisiblePosition& extent, bool isDirectional)
-    : m_base(base.deepEquivalent())
-    , m_extent(extent.deepEquivalent())
-    , m_affinity(base.affinity())
-    , m_isDirectional(isDirectional)
-{
-    validate();
+    : VisibleSelection(position.deepEquivalent(), position.affinity(), isDirectional)
+{
+    // FIXME: Wasteful that this re-canonicalizes, but risky to change since the VisiblePosition object could be from before a mutation and its position may no longer be canonical.
+}
+
+VisibleSelection::VisibleSelection(const VisiblePosition& anchor, const VisiblePosition& focus, bool isDirectional)
+    : VisibleSelection(anchor.deepEquivalent(), focus.deepEquivalent(), anchor.affinity(), isDirectional)
+{
+    // FIXME: Wasteful that this re-canonicalizes, but risky to change since the VisiblePosition objects could be from before a mutation and their positions may no longer be canonical.
 }
 
 VisibleSelection::VisibleSelection(const SimpleRange& range, Affinity affinity, bool isDirectional)
-    : m_base(createLegacyEditingPosition(&range.startContainer(), range.startOffset()))
-    , m_extent(createLegacyEditingPosition(&range.endContainer(), range.endOffset()))
-    , m_affinity(affinity)
-    , m_isDirectional(isDirectional)
-{
-    ASSERT(&range.startContainer().treeScope() == &range.endContainer().treeScope());
-    validate();
+    : VisibleSelection(makeDeprecatedLegacyPosition(range.start), makeDeprecatedLegacyPosition(range.end), affinity, isDirectional)
+{
 }
 
@@ -102 +87 @@
 }
 
+Position VisibleSelection::anchor() const
+{
+    return m_anchor;
+}
+
+Position VisibleSelection::focus() const
+{
+    return m_focus;
+}
+
+Position VisibleSelection::uncanonicalizedStart() const
+{
+    return m_baseIsFirst ? m_anchor : m_focus;
+}
+
+Position VisibleSelection::uncanonicalizedEnd() const
+{
+    return m_baseIsFirst ? m_focus : m_anchor;
+}
+
+Optional<SimpleRange> VisibleSelection::range() const
+{
+    return makeSimpleRange(uncanonicalizedStart().parentAnchoredEquivalent(), uncanonicalizedEnd().parentAnchoredEquivalent());
+}
+
 void VisibleSelection::setBase(const Position& position)
 {
-    m_base = position;
+    m_anchor = position;
     validate();
 }
@@ -110 +120 @@
 void VisibleSelection::setBase(const VisiblePosition& visiblePosition)
 {
-    m_base = visiblePosition.deepEquivalent();
+    setBase(visiblePosition.deepEquivalent());
+}
+
+void VisibleSelection::setExtent(const Position& position)
+{
+    m_focus = position;
     validate();
 }
 
-void VisibleSelection::setExtent(const Position& position)
-{
-    m_extent = position;
-    validate();
-}
-
 void VisibleSelection::setExtent(const VisiblePosition& visiblePosition)
 {
-    m_extent = visiblePosition.deepEquivalent();
-    validate();
+    setExtent(visiblePosition.deepEquivalent());
 }
 
@@ -211 +219 @@
 void VisibleSelection::setBaseAndExtentToDeepEquivalents()
 {
-    // Move the selection to rendered positions, if possible.
-    bool baseAndExtentEqual = m_base == m_extent;
-    if (m_base.isNotNull()) {
-        m_base = VisiblePosition(m_base, m_affinity).deepEquivalent();
-        if (baseAndExtentEqual)
-            m_extent = m_base;
-    }
-    if (m_extent.isNotNull() && !baseAndExtentEqual)
-        m_extent = VisiblePosition(m_extent, m_affinity).deepEquivalent();
-
-    // Make sure we do not have a dangling base or extent.
-    if (m_base.isNull() && m_extent.isNull())
-        m_baseIsFirst = true;
-    else if (m_base.isNull()) {
-        m_base = m_extent;
-        m_baseIsFirst = true;
-    } else if (m_extent.isNull()) {
+    // If only one of anchor and focus is null, convert to a caret selection.
+    // FIXME: Seems like a better rule would be to convert to no selection.
+    if (m_anchor.isNull())
+        m_anchor = m_focus;
+    if (m_focus.isNull())
+        m_focus = m_anchor;
+
+    m_base = VisiblePosition(m_anchor, m_affinity).deepEquivalent();
+    if (m_anchor == m_focus)
         m_extent = m_base;
-        m_baseIsFirst = true;
-    } else
-        m_baseIsFirst = m_base <= m_extent;
-}
-
-void VisibleSelection::setStartAndEndFromBaseAndExtentRespectingGranularity(TextGranularity granularity)
-{
-    if (m_baseIsFirst) {
-        m_start = m_base;
-        m_end = m_extent;
-    } else {
-        m_start = m_extent;
-        m_end = m_base;
-    }
-
+    else
+        m_extent = VisiblePosition(m_focus, m_affinity).deepEquivalent();
+
+    m_baseIsFirst = m_base <= m_extent;
+}
+
+void VisibleSelection::adjustSelectionRespectingGranularity(TextGranularity granularity)
+{
     switch (granularity) {
         case TextGranularity::CharacterGranularity:
@@ -287 +280 @@
                 
             m_end = end.deepEquivalent();
+
             // End must not be before start.
-            if (m_start.deprecatedNode() == m_end.deprecatedNode() && m_start.deprecatedEditingOffset() > m_end.deprecatedEditingOffset()) {
-                Position swap(m_start);
-                m_start = m_end;    
-                m_end = swap;    
-            }
+            if (m_start.deprecatedNode() == m_end.deprecatedNode() && m_start.deprecatedEditingOffset() > m_end.deprecatedEditingOffset())
+                std::swap(m_start, m_end);
             break;
         }
-        case TextGranularity::SentenceGranularity: {
+        case TextGranularity::SentenceGranularity:
             m_start = startOfSentence(VisiblePosition(m_start, m_affinity)).deepEquivalent();
             m_end = endOfSentence(VisiblePosition(m_end, m_affinity)).deepEquivalent();
             break;
-        }
         case TextGranularity::LineGranularity: {
             m_start = startOfLine(VisiblePosition(m_start, m_affinity)).deepEquivalent();
@@ -385 +375 @@
 {
     setBaseAndExtentToDeepEquivalents();
-    setStartAndEndFromBaseAndExtentRespectingGranularity(granularity);
+
+    m_start = m_baseIsFirst ? m_base : m_extent;
+    m_end = m_baseIsFirst ? m_extent : m_base;
+
+    auto startBeforeAdjustments = m_start;
+    auto endBeforeAdjustments = m_end;
+
+    adjustSelectionRespectingGranularity(granularity);
     adjustSelectionToAvoidCrossingShadowBoundaries();
     adjustSelectionToAvoidCrossingEditingBoundaries();
     updateSelectionType();
+
+    bool shouldUpdateAnchor = m_start != startBeforeAdjustments;
+    bool shouldUpdateFocus = m_end != endBeforeAdjustments;
 
     if (isRange()) {
@@ -395 +395 @@
         // useful to make to make the selection "canonical" (if only for
         // purposes of comparing selections). This is an ideal point of the code
-        // to do this operation, since all selection changes that result in a RANGE 
+        // to do this operation, since all selection changes that result in a RANGE
         // come through here before anyone uses it.
         // FIXME: Canonicalizing is good, but haven't we already done it (when we
@@ -402 +402 @@
         m_end = m_end.upstream();
 
-        // FIXME: Position::downstream() or Position::upStream() might violate editing boundaries
-        // if an anchor node has a Shadow DOM. So we adjust selection to avoid crossing editing
-        // boundaries again. See https://bugs.webkit.org/show_bug.cgi?id=87463
+        // Position::downstream() or Position::upstream() might violate editing boundaries
+        // if an anchor node has a Shadow DOM even though they should not. But because this
+        // happens in practice, adjust selection to avoid crossing editing boundaries again.
+        // See https://bugs.webkit.org/show_bug.cgi?id=87463.
         adjustSelectionToAvoidCrossingEditingBoundaries();
+    }
+
+    if (shouldUpdateAnchor) {
+        m_anchor = m_baseIsFirst ? m_start : m_end;
+        m_base = m_anchor;
+    }
+    if (shouldUpdateFocus) {
+        m_focus = m_baseIsFirst ? m_end : m_start;
+        m_extent = m_focus;
     }
 }
@@ -417 +427 @@
 void VisibleSelection::setWithoutValidation(const Position& base, const Position& extent)
 {
-    ASSERT(!base.isNull());
-    ASSERT(!extent.isNull());
+    ASSERT(base.isNull() == extent.isNull());
     ASSERT(m_affinity == Affinity::Downstream);
+    m_anchor = base;
+    m_focus = extent;
     m_base = base;
     m_extent = extent;
@@ -487 +498 @@
 void VisibleSelection::adjustSelectionToAvoidCrossingShadowBoundaries()
 {
-    if (m_base.isNull() || m_start.isNull() || m_end.isNull())
+    if (m_start.isNull() || m_end.isNull())
         return;
 
@@ -501 +512 @@
     }
 
+    // Correct the focus if necessary.
     if (m_baseIsFirst) {
         m_extent = adjustPositionForEnd(m_end, m_start.containerNode());
@@ -508 +520 @@
         m_start = m_extent;
     }
+    m_focus = m_extent;
 }
 
 void VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries()
 {
-    if (m_base.isNull() || m_start.isNull() || m_end.isNull())
+    if (m_start.isNull() || m_end.isNull())
         return;
 
@@ -576 +589 @@
 
             if (previous.isNull()) {
-                // The selection crosses an Editing boundary.  This is a
-                // programmer error in the editing code.  Happy debugging!
-                ASSERT_NOT_REACHED();
-                m_base = Position();
-                m_extent = Position();
-                validate();
+                *this = { };
                 return;
             }
@@ -605 +613 @@
             
             if (next.isNull()) {
-                // The selection crosses an Editing boundary.  This is a
-                // programmer error in the editing code.  Happy debugging!
-                ASSERT_NOT_REACHED();
-                m_base = Position();
-                m_extent = Position();
-                validate();
+                *this = { };
                 return;
             }
@@ -617 +620 @@
     }
     
-    // Correct the extent if necessary.
-    if (baseEditableAncestor != lowestEditableAncestor(m_extent.containerNode()))
+    // Correct the focus if necessary.
+    if (baseEditableAncestor != lowestEditableAncestor(m_extent.containerNode())) {
         m_extent = m_baseIsFirst ? m_end : m_start;
+        m_focus = m_extent;
+    }
 }
 

trunk/Source/WebCore/editing/VisibleSelection.h

@@ -40 +40 @@
     static constexpr auto defaultAffinity = VisiblePosition::defaultAffinity;
 
+    VisibleSelection(const Position& anchor, const Position& focus, Affinity = defaultAffinity, bool isDirectional = false);
+
     VisibleSelection(const Position&, Affinity, bool isDirectional = false);
-    VisibleSelection(const Position&, const Position&, Affinity = defaultAffinity, bool isDirectional = false);
     WEBCORE_EXPORT VisibleSelection(const SimpleRange&, Affinity = defaultAffinity, bool isDirectional = false);
     WEBCORE_EXPORT VisibleSelection(const VisiblePosition&, bool isDirectional = false);
-    WEBCORE_EXPORT VisibleSelection(const VisiblePosition&, const VisiblePosition&, bool isDirectional = false);
+    WEBCORE_EXPORT VisibleSelection(const VisiblePosition& anchor, const VisiblePosition& focus, bool isDirectional = false);
 
     WEBCORE_EXPORT static VisibleSelection selectionFromContentsOfNode(Node*);
@@ -51 +52 @@
     Affinity affinity() const { return m_affinity; }
 
+    // FIXME: Move to the terms "focus" and "anchor" instead of "base" and "extent".
+
     void setBase(const Position&);
     void setBase(const VisiblePosition&);
@@ -56 +59 @@
     void setExtent(const VisiblePosition&);
 
+    // These functions return the values that were passed in, without the canonicalization done by VisiblePosition.
+    // FIXME: When we expand granularity, we canonicalize as a side effect, so expanded values have been made canonical.
+    // FIXME: Replace start/range/base/end/firstRange with these, renaming these to the shorter names.
+    Position uncanonicalizedStart() const;
+    Position uncanonicalizedEnd() const;
+    Position anchor() const;
+    Position focus() const;
+    WEBCORE_EXPORT Optional<SimpleRange> range() const;
+
+    // FIXME: Rename these to include the word "canonical" or remove.
     Position base() const { return m_base; }
     Position extent() const { return m_extent; }
     Position start() const { return m_start; }
     Position end() const { return m_end; }
-    
+
     VisiblePosition visibleStart() const { return VisiblePosition(m_start, isRange() ? Affinity::Downstream : affinity()); }
     VisiblePosition visibleEnd() const { return VisiblePosition(m_end, isRange() ? Affinity::Upstream : affinity()); }
@@ -85 +98 @@
     WEBCORE_EXPORT bool expandUsingGranularity(TextGranularity granularity);
 
+    // FIXME: Rename to include the word "canonical" and remove the word "first" or remove.
     // We don't yet support multi-range selections, so we only ever have one range to return.
     WEBCORE_EXPORT Optional<SimpleRange> firstRange() const;
 
-    // FIXME: Most callers probably don't want this function, and should use firstRange instead.
-    // toNormalizedRange is like firstRange, but contracts the range around text and moves the caret upstream before returning the range.
+    // FIXME: Most callers probably don't want this function, and should use range instead.
+    // This function contracts the range around text and moves the caret upstream before returning the range.
     WEBCORE_EXPORT Optional<SimpleRange> toNormalizedRange() const;
 
@@ -96 +110 @@
     WEBCORE_EXPORT bool hasEditableStyle() const;
     WEBCORE_EXPORT bool isContentRichlyEditable() const;
+
     // Returns a shadow tree node for legacy shadow trees, a child of the
     // ShadowRoot node for new shadow trees, or 0 for non-shadow trees.
@@ -118 +133 @@
     // Support methods for validate()
     void setBaseAndExtentToDeepEquivalents();
-    void setStartAndEndFromBaseAndExtentRespectingGranularity(TextGranularity);
+    void adjustSelectionRespectingGranularity(TextGranularity);
     void adjustSelectionToAvoidCrossingShadowBoundaries();
     void adjustSelectionToAvoidCrossingEditingBoundaries();
     void updateSelectionType();
 
-    // We store these as Positions because VisibleSelection is used to store values in
+    // We store only Positions because VisibleSelection is used to store values in
     // editing commands for use when undoing the command. We need to be able to store
     // a selection that, while currently invalid, will be valid once the changes are undone.
 
-    Position m_base; // Where the first click happened.
-    Position m_extent; // Where the end click happened.
-
-    Position m_start; // Leftmost position when expanded to respect granularity.
-    Position m_end; // Rightmost position when expanded to respect granularity.
+    // FIXME: Consider doing canonicalization only as part of editing operations, and keeping all selection endpoints non-canonical outside of that code.
+    // FIXME: Rename m_base to m_canonicalAnchor.
+    // FIXME: Rename m_extent to m_canonicalFocus.
+    Position m_anchor; // Where the first click happened.
+    Position m_focus; // Where the end click/release happened.
+    Position m_base; // Anchor, canonical, not yet expended to respect granularity.
+    Position m_extent; // Focus, canonical, but not expended to respect granularity.
+    Position m_start; // First of anchor and focus, caonicalized and expanded to respect granularity.
+    Position m_end; // Last of anchor and focus, caonicalized and expanded to respect granularity.
 
     Affinity m_affinity { defaultAffinity };

trunk/Source/WebCore/page/DOMSelection.cpp

@@ -70 +70 @@
     if (!frame)
         return WTF::nullopt;
-    auto range = frame->selection().selection().firstRange();
+    auto range = frame->settings().liveRangeSelectionEnabled()
+        ? frame->selection().selection().range()
+        : frame->selection().selection().firstRange();
     if (!range || range->start.container->isInShadowTree())
         return WTF::nullopt;
@@ -81 +83 @@
     if (!frame)
         return { };
+    if (frame->settings().liveRangeSelectionEnabled())
+        return frame->selection().selection().anchor().parentAnchoredEquivalent();
     auto& selection = frame->selection().selection();
     return (selection.isBaseFirst() ? selection.start() : selection.end()).parentAnchoredEquivalent();
@@ -90 +94 @@
     if (!frame)
         return { };
+    if (frame->settings().liveRangeSelectionEnabled())
+        return frame->selection().selection().focus().parentAnchoredEquivalent();
     auto& selection = frame->selection().selection();
     return (selection.isBaseFirst() ? selection.end() : selection.start()).parentAnchoredEquivalent();
@@ -96 +102 @@
 Position DOMSelection::basePosition() const
 {
-    auto frame = this->frame();
-    if (!frame)
-        return { };
+    // FIXME: Remove this once liveRangeSelectionEnabled is always on, since base and anchor should be the same thing.
+    auto frame = this->frame();
+    if (!frame)
+        return { };
+    if (frame->settings().liveRangeSelectionEnabled())
+        return frame->selection().selection().anchor().parentAnchoredEquivalent();
     return frame->selection().selection().base().parentAnchoredEquivalent();
 }
@@ -104 +113 @@
 Position DOMSelection::extentPosition() const
 {
-    auto frame = this->frame();
-    if (!frame)
-        return { };
+    // FIXME: Remove this once liveRangeSelectionEnabled is always on, since extent and focus should be the same thing.
+    auto frame = this->frame();
+    if (!frame)
+        return { };
+    if (frame->settings().liveRangeSelectionEnabled())
+        return frame->selection().selection().focus().parentAnchoredEquivalent();
     return frame->selection().selection().extent().parentAnchoredEquivalent();
 }
@@ -165 +177 @@
         return "None"_s;
     auto& selection = frame->selection();
+    if (frame->settings().liveRangeSelectionEnabled())
+        return !selection.isInDocumentTree() ? "None"_s : range()->collapsed() ? "Caret"_s : "Range"_s;
     if (selection.isNone())
         return "None"_s;
@@ -175 +189 @@
 {
     auto frame = this->frame();
+    if (frame->settings().liveRangeSelectionEnabled())
+        return frame && frame->selection().isInDocumentTree();
     return !frame || frame->selection().isNone() ? 0 : 1;
 }
@@ -211 +227 @@
     if (selection.isNone())
         return Exception { InvalidStateError };
-    selection.disassociateLiveRange();
-    selection.moveTo(selection.selection().end(), Affinity::Downstream);
+    if (frame->settings().liveRangeSelectionEnabled()) {
+        selection.disassociateLiveRange();
+        selection.moveTo(selection.selection().uncanonicalizedEnd(), Affinity::Downstream);
+    } else
+        selection.moveTo(selection.selection().end(), Affinity::Downstream);
     return { };
 }
@@ -224 +243 @@
     if (selection.isNone())
         return Exception { InvalidStateError };
-    selection.disassociateLiveRange();
-    selection.moveTo(selection.selection().start(), Affinity::Downstream);
+    if (frame->settings().liveRangeSelectionEnabled()) {
+        selection.disassociateLiveRange();
+        selection.moveTo(selection.selection().uncanonicalizedStart(), Affinity::Downstream);
+    } else
+        selection.moveTo(selection.selection().start(), Affinity::Downstream);
     return { };
 }
@@ -406 +428 @@
 bool DOMSelection::containsNode(Node& node, bool allowPartial) const
 {
-    // FIXME: This is wrong, and was added to work around anomalies caused when we canonicalize selection endpoints. We should fix that and remove this.
+    // FIXME: The rule implemented here for text nodes is wrong, and was added to work around anomalies caused when we canonicalize selection endpoints.
     if (node.isTextNode() && !node.document().settings().liveRangeSelectionEnabled())
         allowPartial = true;