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
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 9726effb..d5a49ac 100644
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -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.
diff --git a/WebCore/manual-tests/autoscroll-when-outside-window.html b/WebCore/manual-tests/autoscroll-when-outside-window.html
new file mode 100644
index 0000000..91b4ecf
--- /dev/null
+++ b/WebCore/manual-tests/autoscroll-when-outside-window.html
@@ -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>
diff --git a/WebCore/page/EventHandler.cpp b/WebCore/page/EventHandler.cpp
index 4e9152e..ae1d1f6 100644
--- a/WebCore/page/EventHandler.cpp
+++ b/WebCore/page/EventHandler.cpp
@@ -330,13 +330,11 @@
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 @@
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 @@
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 @@
return true;
}
#endif
+
if (m_frameSetBeingResized)
return dispatchMouseEvent(mousemoveEvent, m_frameSetBeingResized.get(), false, 0, mouseEvent, false);
@@ -962,9 +988,9 @@
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 @@
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();
}
diff --git a/WebCore/page/EventHandler.h b/WebCore/page/EventHandler.h
index 9528428..98f11e7 100644
--- a/WebCore/page/EventHandler.h
+++ b/WebCore/page/EventHandler.h
@@ -85,7 +85,7 @@
void clear();
- void updateSelectionForMouseDragOverPosition(const VisiblePosition&);
+ void updateSelectionForMouseDrag();
Node* mousePressNode() const;
void setMousePressNode(PassRefPtr<Node>);
@@ -255,6 +255,8 @@
bool invertSenseOfTabsToLinks(KeyboardEvent*) const;
+ void updateSelectionForMouseDrag(Node* targetNode, const IntPoint& localPoint);
+
Frame* m_frame;
bool m_mousePressed;
diff --git a/WebCore/rendering/RenderLayer.cpp b/WebCore/rendering/RenderLayer.cpp
index 75e84be..9009e21 100644
--- a/WebCore/rendering/RenderLayer.cpp
+++ b/WebCore/rendering/RenderLayer.cpp
@@ -840,25 +840,18 @@
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)
