<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[198909] trunk/Source/WebCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/198909">198909</a></dd>
<dt>Author</dt> <dd>tonikitoo@webkit.org</dd>
<dt>Date</dt> <dd>2016-03-31 12:25:45 -0700 (Thu, 31 Mar 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>eventMayStartDrag() does not check for shiftKey or isOverLink
https://bugs.webkit.org/show_bug.cgi?id=155746
Reviewed by Darin Adler.
There is currently a mismatch between the logic that checks whether
an event can start a dragging action in EventHandler::handleMousePressEvent
and in EventHandler::eventMayStartDrag
Basically the former checks for event's count, type, button, target and modifier.
The later, on the other hand, only checks for event's button and count.
In order to sync them up again, as per the comment in the code,
patch factors out the logic in ::handleMousePressEvent into a helper function,
and ultimately reuses it in ::eventMayStartDrag.
No new tests for two reasons:
1) ::handleMousePressEvent will not have any behavior change. Code is factored out
into a helper.
2) The callees of ::eventMayStartDrag are currently WebPage::shouldDelayWindowOrderingEvent
and ::acceptsFirstMouse. Based on my understanding of [1], these methods
are called when there is a (say) browser window in background with some text selected,
and user starts to drag the selected content. Dragging happens without bringing the window
foreground. This is called "non-activating click".
Such behavior will not change with the patch.
[1] https://bugs.webkit.org/show_bug.cgi?id=55053 - "Implement non-activating clicks to allow dragging out of a background window"
* page/EventHandler.cpp:
(WebCore::isSingleMouseDownOnLinkOrImage):
(WebCore::canMouseEventStartDragInternal):
(WebCore::EventHandler::handleMousePressEvent):
(WebCore::documentPointForWindowPoint): Moved up in the file to be used elsewhere.
(WebCore::EventHandler::eventMayStartDrag):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorepageEventHandlercpp">trunk/Source/WebCore/page/EventHandler.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (198908 => 198909)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog 2016-03-31 19:12:29 UTC (rev 198908)
+++ trunk/Source/WebCore/ChangeLog 2016-03-31 19:25:45 UTC (rev 198909)
</span><span class="lines">@@ -1,5 +1,43 @@
</span><span class="cx"> 2016-03-31 Antonio Gomes <tonikitoo@webkit.org>
</span><span class="cx">
</span><ins>+ eventMayStartDrag() does not check for shiftKey or isOverLink
+ https://bugs.webkit.org/show_bug.cgi?id=155746
+
+ Reviewed by Darin Adler.
+
+ There is currently a mismatch between the logic that checks whether
+ an event can start a dragging action in EventHandler::handleMousePressEvent
+ and in EventHandler::eventMayStartDrag
+ Basically the former checks for event's count, type, button, target and modifier.
+ The later, on the other hand, only checks for event's button and count.
+
+ In order to sync them up again, as per the comment in the code,
+ patch factors out the logic in ::handleMousePressEvent into a helper function,
+ and ultimately reuses it in ::eventMayStartDrag.
+
+ No new tests for two reasons:
+
+ 1) ::handleMousePressEvent will not have any behavior change. Code is factored out
+ into a helper.
+
+ 2) The callees of ::eventMayStartDrag are currently WebPage::shouldDelayWindowOrderingEvent
+ and ::acceptsFirstMouse. Based on my understanding of [1], these methods
+ are called when there is a (say) browser window in background with some text selected,
+ and user starts to drag the selected content. Dragging happens without bringing the window
+ foreground. This is called "non-activating click".
+ Such behavior will not change with the patch.
+
+ [1] https://bugs.webkit.org/show_bug.cgi?id=55053 - "Implement non-activating clicks to allow dragging out of a background window"
+
+ * page/EventHandler.cpp:
+ (WebCore::isSingleMouseDownOnLinkOrImage):
+ (WebCore::canMouseEventStartDragInternal):
+ (WebCore::EventHandler::handleMousePressEvent):
+ (WebCore::documentPointForWindowPoint): Moved up in the file to be used elsewhere.
+ (WebCore::EventHandler::eventMayStartDrag):
+
+2016-03-31 Antonio Gomes <tonikitoo@webkit.org>
+
</ins><span class="cx"> SelectionController::positionForPlatform should ask EditingBehavior for platform specific behavior
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=41976
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWebCorepageEventHandlercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/EventHandler.cpp (198908 => 198909)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/EventHandler.cpp 2016-03-31 19:12:29 UTC (rev 198908)
+++ trunk/Source/WebCore/page/EventHandler.cpp 2016-03-31 19:25:45 UTC (rev 198909)
</span><span class="lines">@@ -737,6 +737,22 @@
</span><span class="cx"> return node->canStartSelection() || Position::nodeIsUserSelectAll(node);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+#if ENABLE(DRAG_SUPPORT)
+static bool isSingleMouseDownOnLinkOrImage(const MouseEventWithHitTestResults& event)
+{
+ auto& platformEvent = event.event();
+ if (platformEvent.type() != PlatformEvent::MousePressed || platformEvent.button() != LeftButton || platformEvent.clickCount() != 1)
+ return false;
+
+ return event.isOverLink() || event.hitTestResult().image();
+}
+
+static bool eventMayStartDragInternal(const MouseEventWithHitTestResults& event)
+{
+ return !event.event().shiftKey() || isSingleMouseDownOnLinkOrImage(event);
+}
+#endif
+
</ins><span class="cx"> bool EventHandler::handleMousePressEvent(const MouseEventWithHitTestResults& event)
</span><span class="cx"> {
</span><span class="cx"> #if ENABLE(DRAG_SUPPORT)
</span><span class="lines">@@ -762,13 +778,7 @@
</span><span class="cx"> m_mouseDownMayStartSelect = canMouseDownStartSelect(event.targetNode()) && !event.scrollbar();
</span><span class="cx">
</span><span class="cx"> #if ENABLE(DRAG_SUPPORT)
</span><del>- // Careful that the drag starting logic stays in sync with eventMayStartDrag()
- // FIXME: eventMayStartDrag() does not check for shift key press, link or image event targets.
- // Bug: https://bugs.webkit.org/show_bug.cgi?id=155390
-
- // Single mouse down on links or images can always trigger drag-n-drop.
- bool isMouseDownOnLinkOrImage = event.isOverLink() || event.hitTestResult().image();
- m_mouseDownMayStartDrag = singleClick && (!event.event().shiftKey() || isMouseDownOnLinkOrImage);
</del><ins>+ m_mouseDownMayStartDrag = eventMayStartDragInternal(event);
</ins><span class="cx"> #endif
</span><span class="cx">
</span><span class="cx"> m_mouseDownWasSingleClickInSelection = false;
</span><span class="lines">@@ -816,6 +826,14 @@
</span><span class="cx"> return swallowEvent;
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+static LayoutPoint documentPointForWindowPoint(Frame& frame, const IntPoint& windowPoint)
+{
+ FrameView* view = frame.view();
+ // FIXME: Is it really OK to use the wrong coordinates here when view is 0?
+ // Historically the code would just crash; this is clearly no worse than that.
+ return view ? view->windowToContents(windowPoint) : windowPoint;
+}
+
</ins><span class="cx"> #if ENABLE(DRAG_SUPPORT)
</span><span class="cx"> bool EventHandler::handleMouseDraggedEvent(const MouseEventWithHitTestResults& event)
</span><span class="cx"> {
</span><span class="lines">@@ -860,7 +878,7 @@
</span><span class="cx"> updateSelectionForMouseDrag(event.hitTestResult());
</span><span class="cx"> return true;
</span><span class="cx"> }
</span><del>-
</del><ins>+
</ins><span class="cx"> bool EventHandler::eventMayStartDrag(const PlatformMouseEvent& event) const
</span><span class="cx"> {
</span><span class="cx"> // This is a pre-flight check of whether the event might lead to a drag being started. Be careful
</span><span class="lines">@@ -870,9 +888,6 @@
</span><span class="cx"> if (!renderView)
</span><span class="cx"> return false;
</span><span class="cx">
</span><del>- if (event.button() != LeftButton || event.clickCount() != 1)
- return false;
-
</del><span class="cx"> FrameView* view = m_frame.view();
</span><span class="cx"> if (!view)
</span><span class="cx"> return false;
</span><span class="lines">@@ -881,10 +896,15 @@
</span><span class="cx"> if (!page)
</span><span class="cx"> return false;
</span><span class="cx">
</span><ins>+ HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::DisallowShadowContent);
+ LayoutPoint documentPoint = documentPointForWindowPoint(m_frame, event.position());
+ MouseEventWithHitTestResults mouseEvent = m_frame.document()->prepareMouseEvent(request, documentPoint, event);
+ if (!eventMayStartDragInternal(mouseEvent))
+ return false;
+
</ins><span class="cx"> updateDragSourceActionsAllowed();
</span><del>- HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::DisallowShadowContent);
- HitTestResult result(view->windowToContents(event.position()));
- renderView->hitTest(request, result);
</del><ins>+
+ HitTestResult result = mouseEvent.hitTestResult();
</ins><span class="cx"> DragState state;
</span><span class="cx"> return result.innerElement() && page->dragController().draggableElement(&m_frame, result.innerElement(), result.roundedPointInInnerNodeFrame(), state);
</span><span class="cx"> }
</span><span class="lines">@@ -1577,14 +1597,6 @@
</span><span class="cx"> }
</span><span class="cx"> #endif
</span><span class="cx">
</span><del>-static LayoutPoint documentPointForWindowPoint(Frame& frame, const IntPoint& windowPoint)
-{
- FrameView* view = frame.view();
- // FIXME: Is it really OK to use the wrong coordinates here when view is 0?
- // Historically the code would just crash; this is clearly no worse than that.
- return view ? view->windowToContents(windowPoint) : windowPoint;
-}
-
</del><span class="cx"> static Scrollbar* scrollbarForMouseEvent(const MouseEventWithHitTestResults& mouseEvent, FrameView* view)
</span><span class="cx"> {
</span><span class="cx"> if (view) {
</span></span></pre>
</div>
</div>
</body>
</html>