1301673 - Fix up drag feedback image coordinates

Status: RESOLVED FIXED

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

Description

[Neil Deakin]

There are a number of issues caused by the mixing of the wrong coordinate types in the drag service when creating drag feedback images. The patches to follow will explain each issue.

Comment 1

[Neil Deakin]

Attachment: Part 1 - use better coordinate types for screen position

1. Use ScreenIntPoint for the cached screen position where the drag starts rather than using ints.
2. Convert ConvertToUnscaledDevPixels to return LayoutDeviceIntPoint
3. On Mac, nsDragService::ConstructDragImage should use the cached screen position as other platforms do.

Comment 2

[Neil Deakin]

Attachment: Part 2 - only do computations with mImageOffset and css pixels

mImageOffset is in CSS pixels so don't do computations with it and other values in devpixels.

Comment 3

[Neil Deakin]

Attachment: Part 3 - use LayoutDeviceIntRect for the output drag rectangle

Use device pixels (LayoutDeviceIntRect) for the output drag rectangle from nsBaseDragService::DrawDrag and update them similarly in nsPresShell.

Comment 4

[Neil Deakin]

Attachment: Part 4 - adjust clip and drag region properly

The region is in CSS pixels. Also, use paths for the clip area since we want to add each to the clip whereas Clip doesn't appear to do this.

This fixes the drag feedback when multiple rows within a tree such as those in the bookmarks window are dragged.

Comment 5

[Neil Deakin]

Attachment: Part 5 - use correct rectangle when drag images are disabled

When drag feedback is disabled, (nglayout.enable_drag_images preference), or if the image fails to draw for some reason, Mac draws a grey rectangle instead. This rectangle is using the wrong coordinates.

Comment 6

[Neil Deakin]

Attachment: Part 6 - handle disabled drag feedback in e10s

The code which handles feedback images from the child process doesn't handle when the feedback image is disabled or fails to draw. The patch switches to pass the rectangle up to the parent process.

Comment 7

[Neil Deakin]

Attachment: Part 7 - convert image size to dev pixels

This makes it so when someone calls dataTransfer.setDragImage(imageElement, x, y), the image appears at the correct size.

Comment 8

[Neil Deakin]

Attachment: Testcase

This testcase can be used to test dragging various types of objects. Use the textbox to adjust the offset passed to dataTransfer.setDragImage.

Comment 9

[Neil Deakin]

That is all the patches. Passing a canvas to setDragImage doesn't get sized properly yet (the last two boxes in the testcase) as I think the canvas as it will either need to be redrawn or a scale adjustment supplied.

Comment 10

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

Comment on attachment 8789787 [details] [diff] [review]
Part 6 - handle disabled drag feedback in e10s

> TabParent::TakeDragVisualization(RefPtr<mozilla::gfx::SourceSurface>& aSurface,
>-                                 int32_t& aDragAreaX, int32_t& aDragAreaY)
>+                                 LayoutDeviceIntRect* aDragRect)
> {
>+  if (!mDragValid)
>+    return false;
Nit, always {} with 'if'

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8789779 [details] [diff] [review]
Part 1 - use better coordinate types for screen position

>-   * aScreenX and aScreenY should be the screen coordinates of the mouse click
>-   * for the drag. These are in desktop pixels.
>+   * aScreenPosition should be the screen coordinates of the mouse click
>+   * for the drag. These are in CSS pixels.

>   nsresult DrawDrag(nsIDOMNode* aDOMNode,
>                     nsIScriptableRegion* aRegion,
>-                    int32_t aScreenX, int32_t aScreenY,
>+                    mozilla::ScreenIntPoint aScreenPosition,

Shouldn't this be CSSIntPoint then if these are CSS pixels?

Comment 12

[Neil Deakin]

Attachment: Part 1 - use better coordinate types for screen position

Of course, updated to use CSSIntPixels/CSSIntPoint

Comment 13

[Neil Deakin]

Attachment: Part 2 - only do computations with mImageOffset and css pixels

Comment 14

[Neil Deakin]

Attachment: Part 3 - use LayoutDeviceIntRect for the output drag rectangle

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8801799 [details] [diff] [review]
Part 2 - only do computations with mImageOffset and css pixels

>@@ -502,20 +502,18 @@ nsBaseDragService::DrawDrag(nsIDOMNode* 
>                             nsIntRect* aScreenDragRect,
>                             RefPtr<SourceSurface>* aSurface,
>                             nsPresContext** aPresContext)
> {
>   *aSurface = nullptr;
>   *aPresContext = nullptr;
> 
>   // use a default size, in case of an error.
>-  aScreenDragRect->x = aScreenPosition.x - mImageOffset.x;
>-  aScreenDragRect->y = aScreenPosition.y - mImageOffset.y;
>-  aScreenDragRect->width = 1;
>-  aScreenDragRect->height = 1;
>+  aScreenDragRect->MoveTo(aScreenPosition.x, aScreenPosition.y);
>+  aScreenDragRect->SizeTo(1, 1);


This should still subtract mImageOffset, shouldn't it? Because it's only used if we early exit before hitting the next chunk of code that overwrites whatever is stored in aScreenDragRect.

Also, shouldn't we converting this to unscaled dev pixels too?

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8789789 [details] [diff] [review]
Part 7 - convert image size to dev pixels

>@@ -679,20 +680,24 @@ nsBaseDragService::DrawDragForImage(nsII
>       return NS_ERROR_NOT_AVAILABLE;
> 
>     rv = imgRequest->GetImage(getter_AddRefs(imgContainer));
>     NS_ENSURE_SUCCESS(rv, rv);
>     if (!imgContainer)
>       return NS_ERROR_NOT_AVAILABLE;
> 
>     // use the size of the image as the size of the drag image
>-    imgContainer->GetWidth(&aScreenDragRect->width);
>-    imgContainer->GetHeight(&aScreenDragRect->height);
>+    int32_t imageWidth, imageHeight;
>+    imgContainer->GetWidth(&imageWidth);
>+    imgContainer->GetHeight(&imageHeight);
>+    aScreenDragRect->width = aPresContext->CSSPixelsToDevPixels(imageWidth);
>+    aScreenDragRect->height = aPresContext->CSSPixelsToDevPixels(imageHeight);

Since you're touching this code anyway, is there anything reasonable you can do if GetWidth/Height returns a failure code? They can return failure sometimes and we've had bugs in the past because of this.

Comment 17

[Neil Deakin]

> >-  aScreenDragRect->x = aScreenPosition.x - mImageOffset.x;
> >-  aScreenDragRect->y = aScreenPosition.y - mImageOffset.y;
> >-  aScreenDragRect->width = 1;
> >-  aScreenDragRect->height = 1;
> >+  aScreenDragRect->MoveTo(aScreenPosition.x, aScreenPosition.y);
> >+  aScreenDragRect->SizeTo(1, 1);
> 
> 
> This should still subtract mImageOffset, shouldn't it? Because it's only
> used if we early exit before hitting the next chunk of code that overwrites
> whatever is stored in aScreenDragRect.

OK.

> Also, shouldn't we converting this to unscaled dev pixels too?

The PresContext is needed to be able to do this.

> Since you're touching this code anyway, is there anything reasonable you can
> do if GetWidth/Height returns a failure code? They can return failure
> sometimes and we've had bugs in the past because of this.

I can just make it return an error.

Comment 18

[Neil Deakin]

Attachment: Part 2 - only do computations with mImageOffset and css pixels

Comment 19

[Neil Deakin]

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

Comment 20

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bef53dbf128cb0571c59cde502538f0a40d7587
Bug 1301673, use more specific coordinates for screen position in drag calculations, r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/05f152da0f3fe524d81001785eaf5f30fc3c5951
Bug 1301673, don't confuse different coordinate types in image offsets, r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/20162ef28859450116f183a6922a6d8c63b68780
Bug 1301673, use device pixels for the supplied drag position and the computed dragrectangle, r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c5656f3e0707a4ef8f885310dad4964a0f2c11c
Bug 1301673, use css pixels for the drag region, add each rectangle to the clip region so that tree drag feedback in drawn properly, r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/4afa256ec14fc12798346eaf423ecc02c7135020
Bug 1301673, use the correct coordinates when drag feedback is disabled or fails; this allows the drag feedback on Mac to appear as a grey rectangle, r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8fd4f51596a460a2a5969329ac5d83eb86fb835
Bug 1301673, properly handle disabled drag feedback image and failed drag feedbacks in content processes, r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f320965d24073a97365a948a87240b640e9762
Bug 1301673, convert the image width and height to device pixels so that custom drag images are drawn at the right size, r=tn

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0bef53dbf128
https://hg.mozilla.org/mozilla-central/rev/05f152da0f3f
https://hg.mozilla.org/mozilla-central/rev/20162ef28859
https://hg.mozilla.org/mozilla-central/rev/4c5656f3e070
https://hg.mozilla.org/mozilla-central/rev/4afa256ec14f
https://hg.mozilla.org/mozilla-central/rev/a8fd4f51596a
https://hg.mozilla.org/mozilla-central/rev/e6f320965d24