Permalink
Browse files

Reviewed by Antti.

        - fix <rdar://problem/4889753> REGRESSION: Selection doesn't continue with drag selecting
          when autoscrolling vertically (in Notes as well as Safari)

        The bug doesn't happen inside DumpRenderTree, so I was unable to make an automated
        regression test.

        * manual-tests/autoscroll-when-outside-window.html: Added.

        * rendering/RenderLayer.cpp: (WebCore::RenderLayer::autoscroll): Removed unneeded null
        check for the layer's renderer and the document, neither of which can be null. Call
        the new updateSelectionForMouseDrag instead of doing selection updating here.

        * page/EventHandler.h:
        * page/EventHandler.cpp:
        (WebCore::EventHandler::handleMouseDraggedEvent): Refactored most of the logic
        about updating the selection into updateSelectionForMouseDrag.
        (WebCore::EventHandler::updateSelectionForMouseDrag): Added. The public version of
        this function takes no parameters, and is for use from auto-scrolling code. The
        private version of this function takes node and point parameters and contains the
        shared code, including everything from updateSelectionForMouseDragOverPosition.
        Aside from the code motion, variable name changes, and sharing more code, this
        differs from the old code in RenderLayer::autoscroll in the following ways:

          1) The old code did hit testing only in the layer that was auto-scrolling,
             and the new code instead starts the hit testing at the root layer, which is
             better because it's the same thing we do for mouse moved events. Further,
             the code to do this by calling convertToLayerCoords had a bug  because the
             x and y variables were uninitialized.
          2) The old code passed false for active to HitTestRequest, which was wrong.
             The new code passes true. This flag needs to be true for hit testing done
             while the mouse is down and false for hit testing done while the mouse is up.
          3) The old code did not have the SVG-specific logic to match the mouse moved case.
          4) The old code wouldn't do any selection updating if the return value from hitTest
             was false, which is incorrect. The new code ignores the return value as it should.



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@24957 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information...
1 parent af4695d commit 9a99a71d5bd7680a5d63fd396df22812b511da91 Darin Adler committed Aug 9, 2007
View
@@ -1,3 +1,42 @@
+2007-08-09 Darin Adler <darin@apple.com>
+
+ Reviewed by Antti.
+
+ - fix <rdar://problem/4889753> REGRESSION: Selection doesn't continue with drag selecting
+ when autoscrolling vertically (in Notes as well as Safari)
+
+ The bug doesn't happen inside DumpRenderTree, so I was unable to make an automated
+ regression test.
+
+ * manual-tests/autoscroll-when-outside-window.html: Added.
+
+ * rendering/RenderLayer.cpp: (WebCore::RenderLayer::autoscroll): Removed unneeded null
+ check for the layer's renderer and the document, neither of which can be null. Call
+ the new updateSelectionForMouseDrag instead of doing selection updating here.
+
+ * page/EventHandler.h:
+ * page/EventHandler.cpp:
+ (WebCore::EventHandler::handleMouseDraggedEvent): Refactored most of the logic
+ about updating the selection into updateSelectionForMouseDrag.
+ (WebCore::EventHandler::updateSelectionForMouseDrag): Added. The public version of
+ this function takes no parameters, and is for use from auto-scrolling code. The
+ private version of this function takes node and point parameters and contains the
+ shared code, including everything from updateSelectionForMouseDragOverPosition.
+ Aside from the code motion, variable name changes, and sharing more code, this
+ differs from the old code in RenderLayer::autoscroll in the following ways:
+
+ 1) The old code did hit testing only in the layer that was auto-scrolling,
+ and the new code instead starts the hit testing at the root layer, which is
+ better because it's the same thing we do for mouse moved events. Further,
+ the code to do this by calling convertToLayerCoords had a bug because the
+ x and y variables were uninitialized.
+ 2) The old code passed false for active to HitTestRequest, which was wrong.
+ The new code passes true. This flag needs to be true for hit testing done
+ while the mouse is down and false for hit testing done while the mouse is up.
+ 3) The old code did not have the SVG-specific logic to match the mouse moved case.
+ 4) The old code wouldn't do any selection updating if the return value from hitTest
+ was false, which is incorrect. The new code ignores the return value as it should.
+
2007-08-08 Beth Dakin <bdakin@apple.com>
Reviewed by Geoff Garen.
@@ -0,0 +1,7 @@
+<html>
+ <body>
+ <p>Click somewhere in this text, then quickly drag past the bottom of the window and hold still to allow auto-scrolling to happen.</p>
+ <div style="height:3000px"></div>
+ <p>If the bug occurs, then this text won't be selected until you move the mouse slightly. If the bug does not occur, this text should be selected.</p>
+ </body>
+</html>
@@ -330,13 +330,11 @@ bool EventHandler::handleMouseDraggedEvent(const MouseEventWithHitTestResults& e
if (handleDrag(event))
return true;
- // Mouse not pressed. Do nothing.
if (!m_mousePressed)
return false;
- Node* innerNode = event.targetNode();
-
- if (event.event().button() != LeftButton || !innerNode || !innerNode->renderer())
+ Node* targetNode = event.targetNode();
+ if (event.event().button() != LeftButton || !targetNode || !targetNode->renderer())
return false;
#if PLATFORM(MAC) // FIXME: Why does this assertion fire on other platforms?
@@ -348,30 +346,14 @@ bool EventHandler::handleMouseDraggedEvent(const MouseEventWithHitTestResults& e
if (m_mouseDownMayStartAutoscroll) {
// If the selection is contained in a layer that can scroll, that layer should handle the autoscroll
// Otherwise, let the bridge handle it so the view can scroll itself.
- RenderObject* renderer = innerNode->renderer();
+ RenderObject* renderer = targetNode->renderer();
while (renderer && !renderer->shouldAutoscroll())
renderer = renderer->parent();
if (renderer)
handleAutoscroll(renderer);
}
- if (!(m_mouseDownMayStartSelect && innerNode->renderer()->shouldSelect()))
- return false;
-
-#if ENABLE(SVG)
- Selection curSelection = m_frame->selectionController()->selection();
- if (!curSelection.isNone()
- && curSelection.base().node()->renderer()
- && curSelection.base().node()->renderer()->isSVGText()
- && innerNode->renderer()->containingBlock() != curSelection.base().node()->renderer()->containingBlock())
- return false;
-#endif
-
- // handle making selection
- VisiblePosition pos(innerNode->renderer()->positionForPoint(event.localPoint()));
-
- updateSelectionForMouseDragOverPosition(pos);
-
+ updateSelectionForMouseDrag(targetNode, event.localPoint());
return true;
}
@@ -398,28 +380,71 @@ bool EventHandler::eventMayStartDrag(const PlatformMouseEvent& event) const
return result.innerNode() && result.innerNode()->renderer()->draggableNode(DHTMLFlag, UAFlag, event.x(), event.y(), srcIsDHTML);
}
-void EventHandler::updateSelectionForMouseDragOverPosition(const VisiblePosition& pos)
+void EventHandler::updateSelectionForMouseDrag()
+{
+ FrameView* view = m_frame->view();
+ if (!view)
+ return;
+ RenderObject* renderer = m_frame->renderer();
+ if (!renderer)
+ return;
+ RenderLayer* layer = renderer->layer();
+ if (!layer)
+ return;
+
+ HitTestResult result(view->windowToContents(m_currentMousePosition));
+ layer->hitTest(HitTestRequest(true, true, true), result);
+ updateSelectionForMouseDrag(result.innerNode(), result.localPoint());
+}
+
+void EventHandler::updateSelectionForMouseDrag(Node* targetNode, const IntPoint& localPoint)
{
+ if (!m_mouseDownMayStartSelect)
+ return;
+
+ if (!targetNode)
+ return;
+
+ RenderObject* targetRenderer = targetNode->renderer();
+ if (!targetRenderer)
+ return;
+
+ if (!targetRenderer->shouldSelect())
+ return;
+
+ VisiblePosition targetPosition(targetRenderer->positionForPoint(localPoint));
+
// Don't modify the selection if we're not on a node.
- if (pos.isNull())
+ if (targetPosition.isNull())
return;
// Restart the selection if this is the first mouse move. This work is usually
// done in handleMousePressEvent, but not if the mouse press was on an existing selection.
Selection newSelection = m_frame->selectionController()->selection();
- m_frame->selectionController()->setLastChangeWasHorizontalExtension(false);
-
+
+#if ENABLE(SVG)
+ // Special case to limit selection to the containing block for SVG text.
+ // FIXME: Isn't there a better non-SVG-specific way to do this?
+ if (Node* selectionBaseNode = newSelection.base().node())
+ if (RenderObject* selectionBaseRenderer = selectionBaseNode->renderer())
+ if (selectionBaseRenderer->isSVGText())
+ if (targetNode->renderer()->containingBlock() != selectionBaseRenderer->containingBlock())
+ return;
+#endif
+
if (!m_beganSelectingText) {
m_beganSelectingText = true;
- newSelection = Selection(pos);
+ newSelection = Selection(targetPosition);
}
- newSelection.setExtent(pos);
+ newSelection.setExtent(targetPosition);
if (m_frame->selectionGranularity() != CharacterGranularity)
newSelection.expandUsingGranularity(m_frame->selectionGranularity());
- if (m_frame->shouldChangeSelection(newSelection))
+ if (m_frame->shouldChangeSelection(newSelection)) {
+ m_frame->selectionController()->setLastChangeWasHorizontalExtension(false);
m_frame->selectionController()->setSelection(newSelection);
+ }
}
bool EventHandler::handleMouseUp(const MouseEventWithHitTestResults& event)
@@ -928,6 +953,7 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
return true;
}
#endif
+
if (m_frameSetBeingResized)
return dispatchMouseEvent(mousemoveEvent, m_frameSetBeingResized.get(), false, 0, mouseEvent, false);
@@ -962,9 +988,9 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
m_lastScrollbarUnderMouse = scrollbar;
}
}
-
+
bool swallowEvent = false;
- Node* targetNode = m_capturingMouseEventsNode.get() ? m_capturingMouseEventsNode.get() : mev.targetNode();
+ Node* targetNode = m_capturingMouseEventsNode ? m_capturingMouseEventsNode.get() : mev.targetNode();
RefPtr<Frame> newSubframe = subframeForTargetNode(targetNode);
// We want mouseouts to happen first, from the inside out. First send a move event to the last subframe so that it will fire mouseouts.
@@ -1364,8 +1390,7 @@ void EventHandler::hoverTimerFired(Timer<EventHandler>*)
ASSERT(m_frame->document());
if (RenderObject* renderer = m_frame->renderer()) {
- IntPoint documentPoint = m_frame->view()->windowToContents(m_currentMousePosition);
- HitTestResult result(documentPoint);
+ HitTestResult result(m_frame->view()->windowToContents(m_currentMousePosition));
renderer->layer()->hitTest(HitTestRequest(false, false, true), result);
m_frame->document()->updateRendering();
}
@@ -85,7 +85,7 @@ class EventHandler : Noncopyable {
void clear();
- void updateSelectionForMouseDragOverPosition(const VisiblePosition&);
+ void updateSelectionForMouseDrag();
Node* mousePressNode() const;
void setMousePressNode(PassRefPtr<Node>);
@@ -255,6 +255,8 @@ class EventHandler : Noncopyable {
bool invertSenseOfTabsToLinks(KeyboardEvent*) const;
+ void updateSelectionForMouseDrag(Node* targetNode, const IntPoint& localPoint);
+
Frame* m_frame;
bool m_mousePressed;
@@ -840,25 +840,18 @@ IntRect RenderLayer::getRectToExpose(const IntRect &visibleRect, const IntRect &
void RenderLayer::autoscroll()
{
- if (!renderer() || !renderer()->document() || !renderer()->document()->frame() || !renderer()->document()->frame()->view())
+ Frame* frame = renderer()->document()->frame();
+ if (!frame)
return;
-
- Frame* currentFrame = renderer()->document()->frame();
- IntPoint currentPos = currentFrame->view()->windowToContents(currentFrame->eventHandler()->currentMousePosition());
-
- if (currentFrame->eventHandler()->mouseDownMayStartSelect()) {
- // Convert the mouse position to local layer space.
- int x, y;
- convertToLayerCoords(root(), x, y);
- HitTestRequest request(true, false, true);
- HitTestResult result(currentPos - IntSize(x, y));
- if (hitTest(request, result) && result.innerNode()->renderer() && result.innerNode()->renderer()->shouldSelect()) {
- VisiblePosition pos(result.innerNode()->renderer()->positionForPoint(result.localPoint()));
- currentFrame->eventHandler()->updateSelectionForMouseDragOverPosition(pos);
- }
- }
- scrollRectToVisible(IntRect(currentPos, IntSize(1, 1)), gAlignToEdgeIfNeeded, gAlignToEdgeIfNeeded);
+ FrameView* frameView = frame->view();
+ if (!frameView)
+ return;
+
+ frame->eventHandler()->updateSelectionForMouseDrag();
+
+ IntPoint currentDocumentPosition = frameView->windowToContents(frame->eventHandler()->currentMousePosition());
+ scrollRectToVisible(IntRect(currentDocumentPosition, IntSize(1, 1)), gAlignToEdgeIfNeeded, gAlignToEdgeIfNeeded);
}
void RenderLayer::resize(const PlatformMouseEvent& evt, const IntSize& oldOffset)

0 comments on commit 9a99a71

Please sign in to comment.