Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Clear the dockpanel overlay if a descendant will handle the drop. #300

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

jasongrout
Copy link
Member

@jasongrout jasongrout commented Jul 12, 2017

Our heuristic is that if a descendant of the dock panel will handle the drop, it will also handle drag over events and they won’t bubble up to the dock panel. This makes the dock panel always hide the overlay asynchronously on drag leave, so the hide will be cancelled if a drag over event then immediately bubbles up to the dock panel.

Our heuristic is that if a descendant will handle the drop, it will handle drag over events and they won’t bubble up to us. We hide the overlay asynchronously, so the hide will be cancelled if a p-dragover event bubbles up to us.
@jasongrout
Copy link
Member Author

Discussion around this: https://gitter.im/phosphorjs/phosphor?at=5965688bbc46472974ddd006

Copied here for completeness:

Jason Grout @jasongrout Jul 11 2017 17:24:
@sccolbert - what's the reasoning for turning off the drag overlay for the dock panel only if the new target is not a descendant of the dock panel? https://github.com/phosphorjs/phosphor/blob/master/packages/widgets/src/dockpanel.ts#L477
Why do you check to see if the drag enter target is a descendant?
If I have a drop target inside the dock panel, and start a drag/drop event that has mimetypes that both the dock panel and the child element are drops for, then the dock panel overlay is not cleared when I (a) drag to an area where the dock panel gets a drag start, then (b) drag to the child widget, so now the drop event will be handled by the child. The drop event won't be handled by the dock area, so it makes sense to clear the overlay.

It seems to work well when deleting that check for a descendant element, likely because the leave removes the overlay, but then the drag move event shows the overlay again, so it looks like the overlay never left.
Basically, in your contains check, you're short-circuiting the bubbling of a drop event by giving the visual indication that the drop event will bubble up to the dock panel, when in reality the drop event may not propagate that far.

S. Chris Colbert @sccolbert Jul 11 2017 17:36:
I'm not sure I follow
why do you want the overlay to show for something which is not a descendant of the dock panel?
Ah, I think I understand
your complaint is that the overlay is not cleared if a child widget handles the drag?
Why not use a different mimetype for the child drag?
basically, I need to test if the drag is leaving the dock panel in order to hide the overlay

Jason Grout @jasongrout Jul 11 2017 17:57
yes, my complaint is that the overlay is not cleared if a child widget can handle the drag.
If you always clear on drag leave, then the dragmove command will bubble up where appropriate and set the overlay again.

Why not use a different mimetype for the child drag?

That's one way to work around the problem, but it seems cleaner to fix the assumptions in clearing the overlay. Then I can have a single drag operation that targets both child windows and the drag panel if I want.
you could always do .hide(1), for example, to get an asynchronous hide which should be canceled by the show in the drag move event (if I'm following the code correctly...)

like this: #300

S. Chris Colbert @sccolbert Jul 11 2017 18:47
that logic will fail if you stop moving the mouse exactly when the dragleave is fired.
not saying we can't improve here, just not sure that's the correct fix
you also have the problem of distinguishing between child and parent widgets when you get near a drop zone
in your scenario, the user may get into a situation where they can never get the dock panel zone, because the child is eating all the events.

Jason Grout @jasongrout Jul 11 2017 18:52
dragleave is fired in three situations: on disposal (

Private.dispatchDragLeave(this, this._currentTarget, null, event);
), just before dragover (
Private.dispatchDragLeave(this, prevTarget, currTarget, event);
), and just before dragdrop (
Private.dispatchDragLeave(this, this._currentTarget, null, event);
). I think each of those cases is fine

in your scenario, the user may get into a situation where they can never get the dock panel zone, because the child is eating all the events.

That's a separate design question than the problem of the dockpanel overlay not being cleared when it won't get the drop event. The design does work (sort of) because you can target the borders, but I'm playing with both starting the drag with a modifier and differentiating based on targets.

that logic will fail if you stop moving the mouse exactly when the dragleave is fired.

Since the dragover is fired after dragleave, it should still work.

@jasongrout
Copy link
Member Author

Note also that the spec has the dragover event being fired after the dragleave event: https://html.spec.whatwg.org/multipage/dnd.html#drag-and-drop-processing-model

  1. If the previous step caused the current target element to change, and if the previous target element was not null or a part of a non-DOM document, then fire a DND event named dragleave at the previous target element, with the new current target element as the specific related target.
  2. If the current target element is a DOM element, then fire a DND event named dragover at this current target element.

Copy link
Member

@afshin afshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I couldn't contrive a way to accidentally get the overlay stuck when I played with this build. 👍

I'd like to merge on or before the 2019-AUG-07 JupyterLab weekly call (9AM Pacific).

CC: @sccolbert

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants