1309596 - Update drag feedback

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Neil Deakin]

The tab dragging on e10s uses a panel to be able to update the feedback image after the drag starts and the image has been received from the content process. However this has some issues: it requires us the manually move the popup, it doesn't work well across screens/different dpis and doesn't always interact with some system functions such as hovering over other windows/tasksbars/etc.

Instead, we should use a real drag feedback image when possible. On Mac 10.7+ this capability is available using newer OS DND apis. On Windows, the drag feedback helper component will update the image when DragEnter is called. On Linux, the drag feedback image doesn't seem to allow updating, but this shouldn't be an issue as gtk directly supports using the panel as a drag feedback image so presumably won't have these issues (I haven't test this though).

Comment 1

[Neil Deakin]

Attachment: Part 1 - update drag image when setDragImage is called

Here, I allow changing the drag feedback image within setDragImage during the drag in chrome callers only.

Comment 2

[Neil Deakin]

Attachment: Part 2 - only use panel for tab drag feedback on linux

Instead use setDragImage on Windows and Mac.

Comment 3

[Neil Deakin]

Attachment: Part 3 - update drag feedback on Windows

Comment 4

[Neil Deakin]

Attachment: Part 4 - update drag feedback on Mac

Comment 5

[Neil Deakin]

These patches are dependent on the coordinate fixups in bug 1301673.

Also, on Mac this patch uses a rendering of the tab itself to start which then morphs into the tab feedback image when available. We could also change to use the blank white rectangle as the current code does.

Comment 6

[Neil Deakin]

Attachment: Part 4 - update drag feedback on Mac

This is the real Mac parts

Comment 7

[Neil Deakin]

Attachment: Part 1 - update drag image when setDragImage is called

Comment 8

[Neil Deakin]

Attachment: Part 2 - use setDragImage on Windows/Mac

This patch uses the same appearance as the existing behaviour.

Comment 9

[Neil Deakin]

Attachment: Part 4 - update drag feedback on Mac

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8807111 [details] [diff] [review]
Part 1 - update drag image when setDragImage is called

Always {} with 'if'


> DataTransfer::SetDragImage(Element& aImage, int32_t aX, int32_t aY,
>-                           ErrorResult& aRv)
>+                           nsIPrincipal& aSubjectPrincipal, ErrorResult& aRv)
> {
>-  if (mReadOnly) {
>-    aRv.Throw(NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR);
>+  if (mEventMessage < eDragDropEventFirst || mEventMessage > eDragDropEventLast)
>+    return;
>+
>+  // Allow privileged callers to change the feedback image during the drag.
>+  // Otherise, it can only be changed during the dragstart event.
>+  nsCOMPtr<nsIDragSession> dragSession = nsContentUtils::GetDragSession();
>+  if (dragSession && nsContentUtils::IsSystemPrincipal(&aSubjectPrincipal)) {
>+    dragSession->UpdateDragImage(aImage.AsDOMNode(), aX, aY);
>     return;
>   }

I think this makes the API more confusing - depending on the context API does A or B.
Could you just add
[ChromeOnly]
void updateDragImage(Element image, long x, long y) 
to the webidl and the current ::SetDragImage method would call its implementation if (!mReadOnly)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

And there should be a comment in the .webidl that updateDragImage is for parent process only.

Comment 12

[Neil Deakin]

> I think this makes the API more confusing - depending on the context API
> does A or B.
> Could you just add
> [ChromeOnly]
> void updateDragImage(Element image, long x, long y) 
> to the webidl and the current ::SetDragImage method would call its
> implementation if (!mReadOnly)

I'm not sure how that differs in terms of simplicity. I would still need to have a principal check, and why would you call updateDragImage when setDragImage does everything we need?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

The API wouldn't become so weird that setDragImage works differently depending on the caller.
There would be just two different methods, where the new one wouldn't be exposed in context where it doesn't do anything useful.
And why would you need any principal check? [ChromeOnly] exposes the API to chrome only

Comment 14

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #13)
> The API wouldn't become so weird that setDragImage works differently
> depending on the caller.

What I wanted was that when there was a dragSession, setDragImage would update the image in a chrome caller, and return early (do nothing) for a non-privileged caller. The code should be changed to do this. When there is no dragSession, both would chrome and content callers would set the drag image fields.

> And why would you need any principal check? [ChromeOnly] exposes the API to
> chrome only

You said in comment 10 that SetDragImage should call UpdateDragImage.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Neil Deakin from comment #14)
> What I wanted was that when there was a dragSession, setDragImage would
> update the image in a chrome caller, and return early (do nothing) for a
> non-privileged caller. The code should be changed to do this. When there is
> no dragSession, both would chrome and content callers would set the drag
> image fields.
So why can't there be a method which works always for chrome caller, and another method which always works per HTML spec


> You said in comment 10 that SetDragImage should call UpdateDragImage.

Only in case drag image can be set. So why principal check?

Comment 16

[Neil Deakin]

Attachment: Part 1 - add updateDragImage method

The intent was to not expose drag image updating to non-chrome.

Comment 17

[Neil Deakin]

Attachment: Part 4 - update drag feedback on Mac

This is updated for the changes from bug 1235162

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8821212 [details] [diff] [review]
Part 1 - add updateDragImage method

>-DataTransfer::SetDragImage(Element& aImage, int32_t aX, int32_t aY,
>-                           ErrorResult& aRv)
>+DataTransfer::SetDragImage(Element& aImage, int32_t aX, int32_t aY)
> {
>-  if (mReadOnly) {
>-    aRv.Throw(NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR);
>-    return;
>+  if (mEventMessage == eDragStart && !mReadOnly) {
Please don't add mEventMessage == eDragStart check. Spec doesn't have such, and I don't see how that would be related to this bug.

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8821213 [details] [diff] [review]
Part 4 - update drag feedback on Mac

Review of attachment 8821213 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: widget/cocoa/nsDragService.mm
@@ +114,5 @@
> +                                  CSSIntPoint aPoint,
> +                                  LayoutDeviceIntRect* aDragRect)
> + {
> +   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> + 

stray space

@@ +650,5 @@
> +nsDragService::DragMovedWithView(NSDraggingSession* aSession, NSPoint aPoint)
> +{
> +  aPoint.y = nsCocoaUtils::FlippedScreenY(aPoint.y);
> +
> +  // XXX It feels like we should be using the backing scale factor at aPoint

My opinion is that having screen coordinates in device pixels or CSS int pixels is not meaningful anyway. But anything that mostly works is fine with me at the moment until we get around to cleaning all of this up to be consistent. (IMO: all screen coordinates should be in ScreenPixels, only once you have a window you can convert them to CSSIntPixels or LayoutDevicePixels *relative to that window*. But that's unrelated to this patch.)

Have you tested this patch in non-e10s on a document with a fullZoom != 1?

@@ +680,5 @@
> +        }
> +
> +        CSSIntPoint screenPoint =
> +          CSSIntPoint(pc->DevPixelsToIntCSSPixels(devPoint.x),
> +                         pc->DevPixelsToIntCSSPixels(devPoint.y));

strange indent

@@ +695,5 @@
> +      [aSession enumerateDraggingItemsWithOptions:NSDraggingItemEnumerationConcurrent
> +                                          forView: nil
> +                                          classes: [NSArray arrayWithObject:[NSPasteboardItem class]]
> +                                    searchOptions: nil
> +                                       usingBlock: changeImageBlock];

We usually don't leave spaces behind the colons here.

Comment 20

[Neil Deakin]

Attachment: Part 5 - followup to fix zoom case

Comment 21

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74063db1f65c

Comment 22

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecc3f831f347

Comment 23

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03fd23542c4fb4503e635f4b185f2795a3c61a97
Bug 1309596, add an updateDragImage method to modify the drag feedback image during the drag, r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/096a4d18d3bc46b6be9b9753a94a7d4e48b31ae5
Bug 1309596, use updateDragImage for tab dragging on Windows and Mac so that a proper drag feedback image is used rather than the panel, r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3173d98e40c97cb4feed319a999930fa08802b0
Bug 1309596, implementation of UpdateDragImage by adjusting image and calling DragEnter of the drag feedback helper, r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c52b95f73fdebffa2c871f259b54b9816b39bae
Bug 1309596,implementation of UpdateDragImage on mac, r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/243cf7829d19447e20b6b73b39940615794cdf9a
Bug 1309596, fix changed drag feedback image when the page is zoomed, r=mstange

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/03fd23542c4f
https://hg.mozilla.org/mozilla-central/rev/096a4d18d3bc
https://hg.mozilla.org/mozilla-central/rev/a3173d98e40c
https://hg.mozilla.org/mozilla-central/rev/0c52b95f73fd
https://hg.mozilla.org/mozilla-central/rev/243cf7829d19