The default bug view has changed. See this FAQ.

Update drag feedback

RESOLVED FIXED in Firefox 53

Status

()

Core
Drag and Drop
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(5 attachments, 6 obsolete attachments)

(Assignee)

Description

6 months ago
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).
(Assignee)

Comment 1

6 months ago
Created attachment 8800271 [details] [diff] [review]
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 months ago
Created attachment 8800272 [details] [diff] [review]
Part 2 - only use panel for tab drag feedback on linux

Instead use setDragImage on Windows and Mac.
(Assignee)

Comment 3

6 months ago
Created attachment 8800274 [details] [diff] [review]
Part 3 - update drag feedback on Windows
(Assignee)

Comment 4

6 months ago
Created attachment 8800275 [details] [diff] [review]
Part 4 - update drag feedback on Mac
(Assignee)

Comment 5

6 months ago
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.
(Assignee)

Comment 6

6 months ago
Created attachment 8800277 [details] [diff] [review]
Part 4 - update drag feedback on Mac

This is the real Mac parts
Attachment #8800275 - Attachment is obsolete: true
(Assignee)

Comment 7

5 months ago
Created attachment 8807111 [details] [diff] [review]
Part 1 - update drag image when setDragImage is called
Attachment #8800271 - Attachment is obsolete: true
Attachment #8807111 - Flags: review?(bugs)
(Assignee)

Comment 8

5 months ago
Created attachment 8807112 [details] [diff] [review]
Part 2 - use setDragImage on Windows/Mac

This patch uses the same appearance as the existing behaviour.
Attachment #8800272 - Attachment is obsolete: true
Attachment #8807112 - Flags: review?(felipc)
(Assignee)

Comment 9

5 months ago
Created attachment 8807113 [details] [diff] [review]
Part 4 - update drag feedback on Mac
Attachment #8800277 - Attachment is obsolete: true
Attachment #8807113 - Flags: review?(mstange)
(Assignee)

Updated

5 months ago
Attachment #8800274 - Flags: review?(jmathies)
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)
Attachment #8807111 - Flags: review?(bugs) → review-
And there should be a comment in the .webidl that updateDragImage is for parent process only.
(Assignee)

Comment 12

5 months ago
> 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?
Flags: needinfo?(bugs)
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
Flags: needinfo?(bugs)
(Assignee)

Comment 14

5 months ago
(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.

Updated

5 months ago
Attachment #8800274 - Flags: review?(jmathies) → review+
Attachment #8807112 - Flags: review?(felipc) → review+
(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?
(Assignee)

Comment 16

3 months ago
Created attachment 8821212 [details] [diff] [review]
Part 1 - add updateDragImage method

The intent was to not expose drag image updating to non-chrome.
Attachment #8807111 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8821212 - Flags: review?(bugs)
(Assignee)

Comment 17

3 months ago
Created attachment 8821213 [details] [diff] [review]
Part 4 - update drag feedback on Mac

This is updated for the changes from bug 1235162
Attachment #8821213 - Flags: review?(mstange)
(Assignee)

Updated

3 months ago
Attachment #8807113 - Attachment is obsolete: true
Attachment #8807113 - Flags: review?(mstange)
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.
Attachment #8821212 - Flags: review?(bugs) → review+
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.
Attachment #8821213 - Flags: review?(mstange) → review+
(Assignee)

Comment 20

3 months ago
Created attachment 8821528 [details] [diff] [review]
Part 5 - followup to fix zoom case
Attachment #8821528 - Flags: review?(mstange)
Attachment #8821528 - Flags: review?(mstange) → review+
(Assignee)

Comment 21

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74063db1f65c
(Assignee)

Comment 22

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecc3f831f347
(Assignee)

Comment 23

3 months ago
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

3 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

3 months ago
Depends on: 1326472

Updated

3 months ago
Duplicate of this bug: 1250365
Blocks: 1253294
You need to log in before you can comment on or make changes to this bug.