Bug 176721

Summary: [iOS DnD] Support DataTransfer.setDragImage when starting a drag on iOS
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, kangil.han, megan_gardner, thorton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
thorton: review+
Patch for landing none

Description Wenson Hsieh 2017-09-11 12:39:22 PDT
<rdar://problem/34373660>
Comment 1 Wenson Hsieh 2017-09-11 22:07:58 PDT
Created attachment 320517 [details]
Patch
Comment 2 Tim Horton 2017-09-11 22:38:27 PDT
Comment on attachment 320517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320517&action=review

> Source/WebCore/page/DragController.cpp:1248
> +                if (auto* page = frame.page())
> +                    dragPreviewSize.scale(1 / page->deviceScaleFactor());

This seems a little odd. Did you test with page scale too?

> Source/WebCore/page/DragController.cpp:1252
> +            // We can simply position the preview simply using the bounds of the drag source element.

Too many simplys
Comment 3 Wenson Hsieh 2017-09-11 22:49:52 PDT
(In reply to Tim Horton from comment #2)
> Comment on attachment 320517 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320517&action=review
> 
> > Source/WebCore/page/DragController.cpp:1248
> > +                if (auto* page = frame.page())
> > +                    dragPreviewSize.scale(1 / page->deviceScaleFactor());
> 
> This seems a little odd. Did you test with page scale too?

Yes -- what'll happen is that the image will also appear scaled up, to match what the rest of the page looks like. I thought this would be better than having a detached drag image be a constant size, regardless of page scale, which is why I didn't also scale by 1 / pageScaleFactor() here, though I'm definitely open to that option as well!

> 
> > Source/WebCore/page/DragController.cpp:1252
> > +            // We can simply position the preview simply using the bounds of the drag source element.
> 
> Too many simplys

Good catch! I should stop spamming this word 🙃
Comment 4 Tim Horton 2017-09-11 22:54:31 PDT
(In reply to Wenson Hsieh from comment #3)
> (In reply to Tim Horton from comment #2)
> > Comment on attachment 320517 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=320517&action=review
> > 
> > > Source/WebCore/page/DragController.cpp:1248
> > > +                if (auto* page = frame.page())
> > > +                    dragPreviewSize.scale(1 / page->deviceScaleFactor());
> > 
> > This seems a little odd. Did you test with page scale too?
> 
> Yes -- what'll happen is that the image will also appear scaled up, to match
> what the rest of the page looks like. I thought this would be better than
> having a detached drag image be a constant size, regardless of page scale,
> which is why I didn't also scale by 1 / pageScaleFactor() here, though I'm
> definitely open to that option as well!

Ah, got it. I think your plan makes sense.
Comment 5 Wenson Hsieh 2017-09-11 22:57:05 PDT
Created attachment 320520 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2017-09-11 23:59:51 PDT
Comment on attachment 320520 [details]
Patch for landing

Clearing flags on attachment: 320520

Committed r221907: <http://trac.webkit.org/changeset/221907>