Bug 173854 - [iOS DnD] Support dragging out of contenteditable areas without a prior selection
Summary: [iOS DnD] Support dragging out of contenteditable areas without a prior selec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-26 15:52 PDT by Wenson Hsieh
Modified: 2017-06-27 17:36 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2017-06-26 15:52:16 PDT
<rdar://problem/32236827>
Comment 1 Wenson Hsieh 2017-06-26 16:41:23 PDT
Created attachment 313873 [details]
Depends on bug #173832
Comment 2 Ryosuke Niwa 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.
Comment 3 Wenson Hsieh 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!
Comment 4 Wenson Hsieh 2017-06-27 15:30:18 PDT
Created attachment 313953 [details]
Patch v2
Comment 5 Wenson Hsieh 2017-06-27 17:36:59 PDT
Committed r218855: <http://trac.webkit.org/changeset/218855>