Improving direction-based selection strategy.

Source/core/editing/GranularityStrategy.cpp

Index: Source/core/editing/GranularityStrategy.cpp
diff --git a/Source/core/editing/GranularityStrategy.cpp b/Source/core/editing/GranularityStrategy.cpp
index 5a5fd9ee30330abf12efdd949c68fff152dc0636..b1751561e7f029f9b9e5d0c9f6921651f1117385 100644
--- a/Source/core/editing/GranularityStrategy.cpp
+++ b/Source/core/editing/GranularityStrategy.cpp
@@ -5,6 +5,7 @@
 #include "config.h"
 #include "core/editing/GranularityStrategy.h"
 
+#include "core/editing/FrameSelection.h"
 #include "core/editing/htmlediting.h"
 
 namespace blink {
@@ -24,14 +25,19 @@ SelectionStrategy CharacterGranularityStrategy::GetType() const
 
 void CharacterGranularityStrategy::Clear() { };
 
-VisibleSelection CharacterGranularityStrategy::updateExtent(const VisiblePosition& extentPosition, const VisibleSelection& selection)
+VisibleSelection CharacterGranularityStrategy::updateExtent(const IntPoint& extentPoint, LocalFrame* frame)
 {
+    const VisiblePosition& extentPosition = visiblePositionForContentsPoint(extentPoint, frame);
+    const VisibleSelection& selection = frame->selection().selection();
+    if (selection.visibleBase() == extentPosition)
+        return selection;
     return VisibleSelection(selection.visibleBase(), extentPosition);
 }
 
 DirectionGranularityStrategy::DirectionGranularityStrategy()
-    : m_granularity(CharacterGranularity)
-    , m_lastMoveShrunkSelection(false) { }
+    : m_state(StrategyState::Cleared)
+    , m_granularity(CharacterGranularity)
+    , m_offset(0) { }
 
 DirectionGranularityStrategy::~DirectionGranularityStrategy() { }
 
@@ -42,10 +48,24 @@ SelectionStrategy DirectionGranularityStrategy::GetType() const
 
 void DirectionGranularityStrategy::Clear()
 {
+    m_state = StrategyState::Cleared;
     m_granularity = CharacterGranularity;
-    m_lastMoveShrunkSelection = false;
+    m_offset = 0;
+    m_extentPoint.zero();
 }
 
+bool DirectionGranularityStrategy::arePositionsInOrder(
+    const VisiblePosition& pos1,
+    const VisiblePosition& pos2,
+    int order)
+{
+    int positionOrder = comparePositions(pos1, pos2);
+    if (order == 0)
+        return positionOrder == 0;
+    return order > 0 ? positionOrder > 0 : positionOrder < 0;
+}
+
+
 VisiblePosition DirectionGranularityStrategy::nextWordBound(
     const VisiblePosition& pos,
     SearchDirection direction,
@@ -60,68 +80,113 @@ VisiblePosition DirectionGranularityStrategy::nextWordBound(
     return startOfWord(pos, wordSide);
 }
 
-VisibleSelection DirectionGranularityStrategy::updateExtent(const VisiblePosition& extentPosition, const VisibleSelection& selection)
+VisibleSelection DirectionGranularityStrategy::updateExtent(const IntPoint& extentPoint, LocalFrame* frame)
 {
-    if (extentPosition == selection.visibleExtent())
-        return selection;
+    const VisibleSelection& selection = frame->selection().selection();
+    // Initialize m_extentPoint if this is the first update after a clear.
+    if (m_state == StrategyState::Cleared) {
+        VisiblePosition pos =  selection.isBaseFirst() ? selection.visibleEnd() : selection.visibleStart();
+        m_extentPoint = IntPoint(pos.absoluteCaretBounds().x(), pos.absoluteCaretBounds().y());
+        m_state = StrategyState::Expanding;
+    }
+
+    IntPoint oldOffsetExtentPoint(m_extentPoint.x() + m_offset, m_extentPoint.y());
+    VisiblePosition oldOffsetExtentPosition = visiblePositionForContentsPoint(oldOffsetExtentPoint, frame);

[leviw_travelin_and_unemployed, 2015/05/20 20:24:37]

As discussed in our meeting, hit testing is expensive.

Ideally we'd cache the old position and convert its visible position into
absolute coordinates to compare. This would have the benefit of tracking the
position if it moves for some reason.

It would also mean we could get into the case where the node we're offset into
gets blown away, so we'd want to make sure we use the same logic as Selection to
blow away all this state if an endpoint is removed.

[mfomitchev, 2015/05/20 21:37:29]

Let me think about/work on this.

+
+    // Apply the offset.
+    IntPoint offsetExtentPoint = extentPoint;
+    int dx = extentPoint.x() - m_extentPoint.x();

[leviw_travelin_and_unemployed, 2015/05/20 20:24:37]

This is just wrong in the presence of transforms or vertical text.

[mfomitchev, 2015/05/20 21:37:29]

Yup.

+    if (m_offset != 0) {
+        if (m_offset > 0 && dx > 0)
+            m_offset = std::max(0, m_offset - dx);
+        else if (m_offset < 0 && dx < 0)
+            m_offset = std::min(0, m_offset - dx);
+        offsetExtentPoint.move(m_offset, 0);
+    }
+    m_extentPoint = extentPoint;
+
+    VisiblePosition offsetExtentPosition = visiblePositionForContentsPoint(offsetExtentPoint, frame);
+
+    if (!inSameLine(offsetExtentPosition, oldOffsetExtentPosition)) {
+        m_offset = 0;
+        offsetExtentPoint = extentPoint;
+        offsetExtentPosition = visiblePositionForContentsPoint(extentPoint, frame);
+    }
 
     const VisiblePosition base = selection.visibleBase();
-    const VisiblePosition oldExtentWithGranularity = selection.isBaseFirst() ? selection.visibleEnd() : selection.visibleStart();
+    int extentBaseOrder = comparePositions(offsetExtentPosition, base);
 
-    int extentBaseOrder = comparePositions(extentPosition, base);
-    int oldExtentBaseOrder = comparePositions(oldExtentWithGranularity, base);
+    // Do not allow selection and extent to be at the same position
+    if (extentBaseOrder == 0)
+        return selection;
 
-    bool extentBaseOrderSwitched = (extentBaseOrder > 0 && oldExtentBaseOrder < 0)
-        || (extentBaseOrder < 0 && oldExtentBaseOrder > 0);
+    int oldExtentBaseOrder = comparePositions(selection.visibleExtent(), base);
+    bool extentBaseOrderSwitched = !arePositionsInOrder(offsetExtentPosition, base, oldExtentBaseOrder);

[leviw_travelin_and_unemployed, 2015/05/20 20:24:37]

Is it possible to avoid three calls to comparePositions? That function can also
be super expensive.

[mfomitchev, 2015/05/20 21:37:29]

Let me think about this as well.

 
     // Determine the boundary of the 'current word', i.e. the boundary extending
     // beyond which should change the granularity to WordGranularity.
-    // If the last move has shrunk the selection and is now exactly on the word
-    // boundary - we need to take the next bound as the bound of the "current
-    // word".
-    VisiblePosition currentWordBoundary = nextWordBound(
-        oldExtentWithGranularity,
-        oldExtentBaseOrder > 0 ? SearchDirection::SearchForward : SearchDirection::SearchBackwards,
-        m_lastMoveShrunkSelection ? BoundAdjust::NextBoundIfOnBound : BoundAdjust::CurrentPosIfOnBound);
-
-    bool thisMoveShrunkSelection = (extentBaseOrder > 0 && comparePositions(extentPosition, selection.visibleExtent()) < 0)
-        || (extentBaseOrder < 0 && comparePositions(extentPosition, selection.visibleExtent()) > 0);
-    // If the extent-base order was switched, then the selection is now
-    // expanding in a different direction than before. Therefore we need to
-    // calculate the boundary of the 'current word' in this new direction in
-    // order to be able to tell if the selection expanded beyond it.
+    VisiblePosition currentWordBoundary;
     if (extentBaseOrderSwitched) {
+        // Special case.
+        // If the extent-base order was switched, then the selection is now
+        // expanding in a different direction than before. Therefore we
+        // calculate the current word boundary in this new direction and
+        // based on the |base| position.
         currentWordBoundary = nextWordBound(
             base,
             extentBaseOrder > 0 ? SearchDirection::SearchForward : SearchDirection::SearchBackwards,
             BoundAdjust::NextBoundIfOnBound);
         m_granularity = CharacterGranularity;
-        // When the base/extent order switches it doesn't count as shrinking selection.
-        thisMoveShrunkSelection = false;
+    } else {
+        // Calculate the current word boundary based on |oldExtentWithGranularity|.
+        // If selection was shrunk in the last update and the extent is now
+        // exactly on the word boundary - we need to take the next bound as the
+        // bound of the current word.
+        currentWordBoundary = nextWordBound(
+            oldOffsetExtentPosition,
+            selection.isBaseFirst() ? SearchDirection::SearchForward : SearchDirection::SearchBackwards,
+            m_state == StrategyState::Shrinking ? BoundAdjust::NextBoundIfOnBound : BoundAdjust::CurrentPosIfOnBound);
     }
 
-    bool expandedBeyondWordBoundary;
-    if (extentBaseOrder > 0)
-        expandedBeyondWordBoundary = comparePositions(extentPosition, currentWordBoundary) > 0;
-    else
-        expandedBeyondWordBoundary = comparePositions(extentPosition, currentWordBoundary) < 0;
-    if (expandedBeyondWordBoundary) {
+    bool expandedBeyondWordBoundary = arePositionsInOrder(offsetExtentPosition, currentWordBoundary, extentBaseOrder);

[leviw_travelin_and_unemployed, 2015/05/20 20:24:37]

More comparePositions...

+
+    // The selection is shrunk if the extent changes position to be closer to
+    // the base, and the extent/base order wasn't switched.
+    bool thisMoveShrunkSelection =
+        !extentBaseOrderSwitched
+        && oldOffsetExtentPosition != offsetExtentPosition
+        && arePositionsInOrder(oldOffsetExtentPosition, offsetExtentPosition, extentBaseOrder);

[leviw_travelin_and_unemployed, 2015/05/20 20:24:37]

And *another* comparePositions. This needs to be cleaned up.

+
+    if (expandedBeyondWordBoundary)
         m_granularity = WordGranularity;
-    } else if (thisMoveShrunkSelection) {
+    else if (thisMoveShrunkSelection)
         m_granularity = CharacterGranularity;
-        m_lastMoveShrunkSelection = true;
-    }
 
-    m_lastMoveShrunkSelection = thisMoveShrunkSelection;
-    VisibleSelection newSelection = selection;
-    newSelection.setExtent(extentPosition);
+    VisiblePosition newSelectionExtent = offsetExtentPosition;
     if (m_granularity == WordGranularity) {
-        if (extentBaseOrder > 0)
-            newSelection.setEndRespectingGranularity(m_granularity, LeftWordIfOnBoundary);
-        else
-            newSelection.setStartRespectingGranularity(m_granularity, RightWordIfOnBoundary);
+        // Determine the bounds of the word where the extent is located.
+        // Set the selection extent to one of the two bounds depending on
+        // whether the extent is passed the middle of the word.
+        VisiblePosition boundBeforeExtent = nextWordBound(offsetExtentPosition, SearchDirection::SearchBackwards, BoundAdjust::CurrentPosIfOnBound);
+        VisiblePosition boundAfterExtent = nextWordBound(offsetExtentPosition, SearchDirection::SearchForward, BoundAdjust::CurrentPosIfOnBound);
+        int xMiddleBetweenBounds = (boundAfterExtent.absoluteCaretBounds().x() + boundBeforeExtent.absoluteCaretBounds().x()) / 2;
+        bool extentBeforeMiddle = offsetExtentPoint.x() < xMiddleBetweenBounds;
+
+        newSelectionExtent = extentBeforeMiddle ? boundBeforeExtent : boundAfterExtent;
+        // Update the offset if needed.
+        if (!inSameLine(newSelectionExtent, selection.visibleExtent()))
+            m_offset = 0;
+        else if ((extentBaseOrder > 0 && !extentBeforeMiddle) || (extentBaseOrder < 0 && extentBeforeMiddle))
+            m_offset = newSelectionExtent.absoluteCaretBounds().x() - extentPoint.x();
     }
 
+    // Only update the state if the selection actually changed as a result of
+    // this move.
+    if (newSelectionExtent != selection.visibleExtent())
+        m_state = thisMoveShrunkSelection ? StrategyState::Shrinking : StrategyState::Expanding;
+
+    VisibleSelection newSelection = selection;
+    newSelection.setExtent(newSelectionExtent);
     return newSelection;
 }