863512 - Fix drag and drop PageThumbs callers in browser

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

May I ask, what's the plan here? I just want to make sure we're not heading in the wrong direction :)

Comment 2

[Tom S [:evilpie]]

Now that you are asking, I probably confused this with the tab icons. So I am going to leave this alone.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Ok :) I might steal this one if I find the time. Should be straight forward.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

The problems I see are:

PageThumbs.jsm has a function captureAndStore() which uses |aBrowser.docShell.currentDocumentChannel;| to see if the channel is an error response or not.

Similarly, browser-thumbnails.js has a function _shouldCapture() which does the same basic thing, although with quite a different implementation.

It seems likely some of this will need to move into a content script.

Comment 6

[Tom S [:evilpie]]

So I just came across this, because somebody told me dragging a tab showed a grey rectangle. There might be a problem, because right now this uses captureToCanvas, which is synchronous. I am not sure if we can even set the drag image after dragstart, when we get the thumbnail sent from the child.

Comment 7

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 11

[Jim Mathies [:jimm]]

stealing this for now, since I'm pretty sure its going to end up being a dupe of bug 698371.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Note that there's another consumer of thumbnails, which is tab drag&dropping. I.e. when you're dragging a tab in the tabstrip we show a small thumbnail of that tab. If that's covered by this bug, bug 698371 or left as a follow up is up for figuring out.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Please note that bug 1059032 added a gMultiProcessBrowser check around d&d thumbnails that should hopefully be removed by this bug

Comment 15

[Chris Peterson [:cpeterson]]

This bug should be fixed for the e10s M2 milestone.

Comment 16

[Chris Peterson [:cpeterson]]

Brad says we don't need to block M2 on page thumbnails.

Comment 17

[Jim Mathies [:jimm]]

Morphing this to address calls to PageThumbs.captureToCanvas for drag and drop. These calls may or may not work once PageThumbs is updated in the parent bug. We won't know until drag and drop is working.

Since d&d support is m4, this is now m4 as well.

Comment 18

[Jim Mathies [:jimm]]

Gere's a stack that shows the issue with async rendering, DrawDrag renders the canvas when the drag starts. At least on Win, I'm not sure if there's a way to update these after the drag operation starts. See nsDragService::InvokeDragSession.

nsBaseDragService::DrawDrag(nsIDOMNode * aDOMNode=0x0ef6dc88, nsIScriptableRegion * aRegion=0x00000000, int aScreenX=273, int aScreenY=142, nsIntRect * aScreenDragRect=0x0033d890, mozilla::RefPtr<mozilla::gfx::SourceSurface> * aSurface=0x0033d8a0, nsPresContext * * aPresContext=0x0033d888) Line 447	C++
nsDragService::CreateDragImage(nsIDOMNode * aDOMNode=0x0ef6dc88, nsIScriptableRegion * aRegion=0x00000000, SHDRAGIMAGE * psdi=0x0033d908) Line 88	C++
nsDragService::InvokeDragSession(nsIDOMNode * aDOMNode=0x0ef6dc88, nsISupportsArray * anArrayTransferables=0x0edce440, nsIScriptableRegion * aRegion=0x00000000, unsigned int aActionType=7) Line 251	C++
nsBaseDragService::InvokeDragSessionWithImage(nsIDOMNode * aDOMNode=0x0ef6dc88, nsISupportsArray * aTransferableArray=0x0edce440, nsIScriptableRegion * aRegion=0x00000000, unsigned int aActionType=7, nsIDOMNode * aImage=0x19e121c8, int aImageX=-16, int aImageY=-16, nsIDOMDragEvent * aDragEvent=0x0efce318, nsIDOMDataTransfer * aDataTransfer=0x0f053790) Line 253	C++
EventStateManager::DoDefaultDragStart(nsPresContext * aPresContext=0x08503400, mozilla::WidgetDragEvent * aDragEvent=0x0033db8c, mozilla::dom::DataTransfer * aDataTransfer=0x0f053790, nsIContent * aDragTarget=0x0ef6dc40, nsISelection * aSelection=0x00000000) Line 1897	C++
EventStateManager::GenerateDragGesture(nsPresContext * aPresContext=0x08503400, mozilla::WidgetMouseEvent * aEvent=0x0033e31c) Line 1686	C++
EventStateManager::PreHandleEvent(nsPresContext * aPresContext=0x08503400, mozilla::WidgetEvent * aEvent=0x0033e31c, nsIFrame * aTargetFrame=0x0ed85400, nsEventStatus * aStatus=0x0033e270) Line 609	C++

Comment 19

[Jim Mathies [:jimm]]

*Here's

Comment 23

[Jim Mathies [:jimm]]

> nsBaseDragService::DrawDrag(nsIDOMNode * aDOMNode=0x0ef6dc88,
> nsDragService::InvokeDragSession(nsIDOMNode * aDOMNode=0x0ef6dc88,
> nsBaseDragService::InvokeDragSessionWithImage(nsIDOMNode *
> EventStateManager::DoDefaultDragStart(nsPresContext *
> EventStateManager::PreHandleEvent(nsPresContext * aPresContext=0x08503400,

I posted this to show that we try to set the drag image synchronously when we start the drag. The problem is that we have to query for the thumbnail image async from the content process. The two systems aren't compatible. My guess is we need to either wait until the thumbnail comes back to start the drag session or figure out some way to set the drag image asynchronously for each platform.

Comment 24

[Gabor Krizsanits (INACTIVE)]

I talked to smaug about this on IRC. He had the idea to use the cached image xul:browser has for the image instead. Since xul:browser is on the parent side, and it gets images from the content all the time to place them on the screen, we could just use that instead of depending on PageThumbs.

(In reply to Jim Mathies [:jimm] from comment #23)
> I posted this to show that we try to set the drag image synchronously when
> we start the drag. The problem is that we have to query for the thumbnail
> image async from the content process. The two systems aren't compatible.

I think at that point we already had to calc the image here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5257
But I'm not sure yet, haven't finished reading all the code yet. Anyway, that does not change a lot, I ended up with the same two approaches you mentioned. I'm a bit hesitant with both approach (delaying the session seems like a bad idea vs I'm not sure it's possible to change the dragged image on all OS's)
but didn't see any alternative until smaug's idea. I'll give it a shot.

Comment 25

[Gabor Krizsanits (INACTIVE)]

You mentioned a drag and drop session with a widget we can pain into during the dnd session during the meeting... could you link me to some code?

Comment 26

[Neil Deakin]

You can't change the drag image after the drag starts on all platforms. (not on Mac anyway)

We do have support for using a <panel type="drag"> as the drag feedback image (by passing one to dataTransfer.setDragImage) though which can be updated after the drag starts with whatever content is desired. It's not a true drag feedback image, but the panel tracks the mouse pointer and hides when the drag ends. It likely needs some polish work as its a feature that was going to be used to implement better UI for tab dragging several years ago, but the improved UI never happened. As far as I know, this feature isn't currently used, so i don't have any sample code.

I mentioned it as one possible thing to explore.

Comment 27

[Gabor Krizsanits (INACTIVE)]

(In reply to Neil Deakin from comment #26)
> 
> We do have support for using a <panel type="drag"> as the drag feedback
> image (by passing one to dataTransfer.setDragImage) though which can be
> updated after the drag starts with whatever content is desired.

I tried my luck with this and did not seem to work, I was wondering if I'm doing
it wrong. I just created a panel with that type and added a canvas to it. Do
I need some special flags for the panel? Do I have to append it to something?

> It's not a
> true drag feedback image, but the panel tracks the mouse pointer and hides
> when the drag ends. It likely needs some polish work as its a feature that
> was going to be used to implement better UI for tab dragging several years
> ago, but the improved UI never happened. As far as I know, this feature
> isn't currently used, so i don't have any sample code.

Yes you have mentioned that, but I'm not looking for sample code more like
looking for where are all the logic for this functionality implemented. So
I can read it through or debug it if something does not work out.

> 
> I mentioned it as one possible thing to explore.

Comment 28

[Gabor Krizsanits (INACTIVE)]

So the panel does show up but only for a fraction of a second, then the drag session ends (on linux at least...). It ends with a failure, more precisely, I get an invisibleSourceDragFailed with a GTK_DRAG_RESULT_GRAB_BROKEN. Which I don't know what means exactly. According to the docs it means: "The pointer or keyboard grab used for the drag operation was broken." For what it's worth...

Anyway, I consider this feature to be broken and I have no idea how to fix it tbh.

Another approach I'm currently experimenting is, that I cancel the native dnd session the moment I get a usable image from the content process (using PageThumbs), and start a new one, while letting the DOM believe that nothing has happened, and the new session is still the old one. It's arguably quite hacky, but the best I've got, since all the other approaches failed so far.

Comment 29

[Neil Deakin]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #28)
> So the panel does show up but only for a fraction of a second, then the drag
> session ends (on linux at least...). It ends with a failure, more precisely,
> I get an invisibleSourceDragFailed with a GTK_DRAG_RESULT_GRAB_BROKEN. Which
> I don't know what means exactly. According to the docs it means: "The
> pointer or keyboard grab used for the drag operation was broken." For what
> it's worth...
> 

Ok so that is bug 782811, which I thought had been fixed. Testing on my machine doesn't cause this, although I tested on gtk2.

The error means that we are adding a grab (calling gtk_grab_add) during drag. A grab is normally added when a popup is opened so that key events get redirected to the popup. This is supposed to be blocked when a drag is in progress, however maybe the popup opens before the drag session officially begins. However, we don't need a grab for type="drag" popups so we may be able to solve this by checking for this within nsWindow::CaptureRollupEvents.

Comment 30

[Gabor Krizsanits (INACTIVE)]

(In reply to Neil Deakin from comment #29)
> begins. However, we don't need a grab for type="drag" popups so we may be
> able to solve this by checking for this within nsWindow::CaptureRollupEvents.

Million thanks! This seem to be fixing it! There is still some polishing work to do, and I guess we will have to test all platforms but this is so much nicer and simple than what I've been trying to do.

Comment 31

[Jim Mathies [:jimm]]

Lets file a follow up UX bug on the "initial white box" problem. Pretty sure we can find something to put in there. On release builds people probably won't see it unless the content process is heavily loaded so not that big of a deal hopefully.

Comment 32

[Gabor Krizsanits (INACTIVE)]

(In reply to Jim Mathies [:jimm] from comment #31)
> Lets file a follow up UX bug on the "initial white box" problem. Pretty sure
> we can find something to put in there. On release builds people probably
> won't see it unless the content process is heavily loaded so not that big of
> a deal hopefully.

Makes sense. I've just tested it on a release build and it looked quite nice. We'll see how much follow-up polishing work will pop up, but at first glance it's pretty decent to land imo.

I have not yet tested it on OSX (I don't have any at reach...) but if someone feels like giving it a try here is a binary for it:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f16d320e814e

it looked OK on try so filing patches for review:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f16d320e814e

Comment 33

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: Fixing xul dnd panels for linux. v1

This seemed like the cleanest approach to me. But if you prefer to check the attribute of the panel element that belongs to this widget instead, I could use some help with it... (not sure how to get to the related xul element)

Comment 34

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: Use xul panels for tab dnd in e10s. v1

Jim, you've been following this bug, do you want to do the review? It should be pretty straightforward.

Comment 35

[Jim Mathies [:jimm]]

Comment on attachment 8641663 [details] [diff] [review]
part2: Use xul panels for tab dnd in e10s. v1

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

Don't forget to file that follow up ux bug! Thanks!

::: browser/base/content/tabbrowser.xml
@@ +5292,5 @@
>          canvas.width = 160 * scale;
>          canvas.height = 90 * scale;
> +        let toDrag;
> +        if (gMultiProcessBrowser) {
> +          // Let's create a panel to use it in setDragImage

s/"Let's create a panel"/"Create a drag panel to use in..."

Comment 36

[Neil Deakin]

Comment on attachment 8641661 [details] [diff] [review]
part1: Fixing xul dnd panels for linux. v1

+            // so let's be extra carefull and skip this operation for dnd popup panels always

carefull -> careful


You could also just cache the value of aInitData->mIsDragPopup passed to Create, but I think this is fine too.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de18634e66ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/884385e6f556

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de18634e66ab
https://hg.mozilla.org/mozilla-central/rev/884385e6f556

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 8641663 [details] [diff] [review]
part2: Use xul panels for tab dnd in e10s. v1

>+        if (gMultiProcessBrowser) {
>+          // Let's create a panel to use it in setDragImage
>+          // which will tell xul to render a panel that follows
>+          // the pointer while a dnd session is on.
>+          var panel = document.createElement("panel");
>+          panel.setAttribute("type", "drag");
>+          panel.appendChild(canvas);
>+          document.documentElement.appendChild(panel);
>+          // PageThumb is async with e10s but that's fine
>+          // since we can update the panel during the dnd.
>           PageThumbs.captureToCanvas(browser, canvas);
>+          toDrag = panel;

It looks like this code litters up the browser window DOM with another panel element for every single tab drag. Am I missing something?

Comment 40

[Gabor Krizsanits (INACTIVE)]

(In reply to Dão Gottwald [:dao] from comment #39)
> It looks like this code litters up the browser window DOM with another panel
> element for every single tab drag. Am I missing something?

I'm afraid you are right, I filed a bug for this issue: bug 1194174. Thanks for spotting it!

Comment 42

[Frederic Bezies]

Looks like bug 1214504 is related to this one.