Bug 173854

Summary:

[iOS DnD] Support dragging out of contenteditable areas without a prior selection

Product:

WebKit

Reporter:

Wenson Hsieh <wenson_hsieh>

Component:

WebCore Misc.

Assignee:

Wenson Hsieh <wenson_hsieh>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, commit-queue, megan_gardner, rniwa, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Depends on bug #173832	rniwa: review+
Patch v2	thorton: review+

Wenson Hsieh

Reported 2017-06-26 15:52:16 PDT

<rdar://problem/32236827>

Attachments
Depends on bug #173832 (24.89 KB, patch) 2017-06-26 16:41 PDT, Wenson Hsieh	rniwa: review+	Details Formatted Diff Diff
Patch v2 (25.95 KB, patch) 2017-06-27 15:30 PDT, Wenson Hsieh	thorton: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Wenson Hsieh

Comment 1 2017-06-26 16:41:23 PDT

Created attachment 313873 [details] Depends on bug #173832

Ryosuke Niwa

Comment 2 2017-06-27 14:47:12 PDT

Comment on attachment 313873 [details] Depends on bug #173832 View in context: https://bugs.webkit.org/attachment.cgi?id=313873&action=review r=me assuming it passes tests. > Source/WebCore/page/DragController.cpp:726 > + auto* renderer = image.renderer(); > + if (!is<RenderImage>(renderer)) > + return false; > + > + auto* cachedImage = downcast<RenderImage>(*renderer).cachedImage(); > + return cachedImage && !cachedImage->errorOccurred() && cachedImage->imageForRenderer(renderer); We need to be always careful when we rely on the existence of renderer(). Are we sure we've updated the layout / style before getting here? > Source/WebCore/page/ios/EventHandlerIOS.mm:579 > + IntPoint adjustedClientPosition; We do we need to declare this upfront? > Source/WebCore/page/ios/EventHandlerIOS.mm:584 > + { > + FloatPoint adjustedClientPositionAsFloatPoint(clientPosition); > + protectedFrame->nodeRespondingToClickEvents(clientPosition, adjustedClientPositionAsFloatPoint); > + adjustedClientPosition = roundedIntPoint(adjustedClientPositionAsFloatPoint); > + } Why can't we get rid of the curly brackets and declare adjustedClientPosition right in the third line instead? > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/contenteditable-and-target.html:1 > +<head> Missing DOCTYPE.

Wenson Hsieh

Comment 3 2017-06-27 15:13:08 PDT

Comment on attachment 313873 [details] Depends on bug #173832 View in context: https://bugs.webkit.org/attachment.cgi?id=313873&action=review >> Source/WebCore/page/DragController.cpp:726 >> + return cachedImage && !cachedImage->errorOccurred() && cachedImage->imageForRenderer(renderer); > > We need to be always careful when we rely on the existence of renderer(). Are we sure we've updated the layout / style before getting here? I see. We have always accessed the renderer here (see DragController::draggableElement), but it doesn't seem like we update layout/style from all call sites (even on Mac), which seems problematic! I'll fix it for iOS first by making sure that we update layout/style when starting a drag. >> Source/WebCore/page/ios/EventHandlerIOS.mm:579 >> + IntPoint adjustedClientPosition; > > We do we need to declare this upfront? Right -- I cleaned this up as a part of my second patch. >> Source/WebCore/page/ios/EventHandlerIOS.mm:584 >> + } > > Why can't we get rid of the curly brackets and declare adjustedClientPosition right in the third line instead? 👍 >> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/contenteditable-and-target.html:1 >> +<head> > > Missing DOCTYPE. Added!

Wenson Hsieh

Comment 4 2017-06-27 15:30:18 PDT

Created attachment 313953 [details] Patch v2

Wenson Hsieh

Comment 5 2017-06-27 17:36:59 PDT

Committed r218855: <http://trac.webkit.org/changeset/218855>

Note You need to log in before you can comment on or make changes to this bug.