Neil Deakin Assignee
	Description • 8 years ago

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.

	Neil Deakin Assignee
	Comment 1 • 8 years ago

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.

	Neil Deakin Assignee
	Comment 2 • 8 years ago

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

	Neil Deakin Assignee
	Comment 3 • 8 years ago

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

	Neil Deakin Assignee
	Comment 4 • 8 years ago

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.

	Neil Deakin Assignee
	Comment 5 • 8 years ago

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.

	Neil Deakin Assignee
	Comment 6 • 8 years ago

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.

	Neil Deakin Assignee
	Comment 7 • 8 years ago

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

	Neil Deakin Assignee
	Comment 8 • 8 years ago

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

	Neil Deakin Assignee
	Comment 9 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 8 years ago

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'

	Timothy Nikkel (:tnikkel)
	Comment 11 • 8 years ago

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?

	Neil Deakin Assignee
	Comment 12 • 8 years ago

Of course, updated to use CSSIntPixels/CSSIntPoint

	Timothy Nikkel (:tnikkel)
	Comment 15 • 8 years ago

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?

	Timothy Nikkel (:tnikkel)
	Comment 16 • 8 years ago

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.

	Neil Deakin Assignee
	Comment 17 • 8 years ago

> >-  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.

	Neil Deakin Assignee
	Comment 19 • 8 years ago

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

	Neil Deakin Assignee
	Comment 20 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 21 • 8 years ago
bugherder

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