RESOLVED FIXED 173854
[iOS DnD] Support dragging out of contenteditable areas without a prior selection 
https://bugs.webkit.org/show_bug.cgi?id=173854
Summary [iOS DnD] Support dragging out of contenteditable areas without a prior selec...
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+
DetailsFormatted DiffDiff
Patch v2 (25.95 KB, patch)
2017-06-27 15:30 PDT, Wenson Hsieh 		thorton: review+
DetailsFormatted DiffDiff
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.