Fix up drag feedback image coordinates

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(8 attachments, 4 obsolete attachments)

2.19 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.48 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
11.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.30 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.49 KB, text/html
Details
18.56 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
19.20 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.25 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8789779 - Flags: review?(tnikkel)
Attachment #8789779 - Attachment description: Part 1 - → Part 1 - use better coordinate types for screen position
mImageOffset is in CSS pixels so don't do computations with it and other values in devpixels.
Attachment #8789782 - Flags: review?(tnikkel)
Use device pixels (LayoutDeviceIntRect) for the output drag rectangle from nsBaseDragService::DrawDrag and update them similarly in nsPresShell.
Attachment #8789784 - Flags: review?(tnikkel)
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.
Attachment #8789785 - Flags: review?(tnikkel)
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.
Attachment #8789786 - Flags: review?(tnikkel)
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.
Attachment #8789787 - Flags: review?(bugs)
This makes it so when someone calls dataTransfer.setDragImage(imageElement, x, y), the image appears at the correct size.
Attachment #8789789 - Flags: review?(tnikkel)
Posted file Testcase
This testcase can be used to test dragging various types of objects. Use the textbox to adjust the offset passed to dataTransfer.setDragImage.
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 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'
Attachment #8789787 - Flags: review?(bugs) → review+
Attachment #8789779 - Attachment is patch: true
Blocks: 1309596
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?
Of course, updated to use CSSIntPixels/CSSIntPoint
Attachment #8789779 - Attachment is obsolete: true
Attachment #8789779 - Flags: review?(tnikkel)
Attachment #8801797 - Flags: review?(tnikkel)
Attachment #8789782 - Attachment is obsolete: true
Attachment #8789782 - Flags: review?(tnikkel)
Attachment #8801799 - Flags: review?(tnikkel)
Attachment #8789784 - Attachment is obsolete: true
Attachment #8789784 - Flags: review?(tnikkel)
Attachment #8801800 - Flags: review?(tnikkel)
Attachment #8801797 - Flags: review?(tnikkel) → review+
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?
Attachment #8801800 - Flags: review?(tnikkel) → review+
Attachment #8789785 - Flags: review?(tnikkel) → review+
Attachment #8789786 - Flags: review?(tnikkel) → review+
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.
Attachment #8789789 - Flags: review?(tnikkel) → review+
> >-  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.
Attachment #8801799 - Attachment is obsolete: true
Attachment #8801799 - Flags: review?(tnikkel)
Attachment #8802172 - Flags: review?(tnikkel)
Attachment #8802172 - Flags: review?(tnikkel) → review+
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
Duplicate of this bug: 933302
You need to log in before you can comment on or make changes to this bug.