1232806 - It's possible to accidentally end up dragging several nodes in markup at once

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[arni2033]

Attachment: screencast 1 - It's possible to accidentally end up dragging several nodes.webm

>>>   My Info:   Win7_64, Nightly 45, 32bit, ID 20151215030221
STR:
1. Open this "data:" url
>   data:text/html,<a><b><div></div></b></a>
2. Open devtools -> inspector, expand the tree in markup-view.
3. Move mouse pointer over <div> string in markup, press left mouse button to start dragging, then
   move mouse over the center of content area (on the page) and release left mouse button
4. Repeat Step 3 with <b> string in markup
5. Repeat Step 3 with <a> string in markup

Result:       
 After Step 5 you're dragging 3 items at once. Pressing Escape cancels only the last node

Expectations: 
 After Step 3 drag-n-drop operation should be canceled correctly

Comment 1

[Patrick Brosset <:pbro>]

Triaging (filter on CLIMBING SHOES).

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

The problem is that when dragging, we listen to the mouseup element on the markup panel. In the case of this bug, because the mouse is released within the content page, we do not capture it, and so when getting back on the markup panel we still think we are dragging the original element.

There's multiple ways to deal with this : 

1. We can track the mouseup element on the content page too. But if you drag an element outside the Firefox window and release the mouse button, it would triggers the same bug.

2. Use PointerLock, but this seems a little like a hack. PointerLock was designed with video games in mind, so it hide the cursor ( and I couldn't find a way to not do that ). Furthermore, I didn't pushed it too far because of the former problem, but I couldn't get it working, maybe because it require to request the permission to the user and permissions within the devtools work differently ? 

3. Track the mouseenter event on the markup view, and if dragging AND we don't have the mouse button pressed, stop dragging. It works, but it's kind of weird to have the drag mode being stop way after the mouse button is released

4. Quit dragging mode when the mouse leave the markup view ( with mouseleave event ), with a given delay to prevent quitting if the user goes slightly off the markup panel before coming back in right after.


I think 4. is the best choice for this, but I wonder what do you think about this ?

Comment 3

[arni2033]

> 1. We can track the mouseup element on the content page too. But if you drag an element outside the
> Firefox window and release the mouse button, it would triggers the same bug.
Well,... it will not trigger the same bug if you will listen for 'mouseup' event in markup frame.
I can provide a web page using 'mousedown', 'mousemove' and 'mouseup' listeners instead of fancy '*drop*' events, which allows releasing mouse outside of the content area.

Comment 4

[arni2033]

So, huh, I looked at the code and saw that 'mousedown', 'mousemove' and 'mouseup' handlers are already used, but they are attached to 'document.body', not 'window' for some reason.
Here's an example of a page that allows dragging and canceling the drag of an element by 'mouseup' outside of content area (click on the image, and then you'll be able to drag the gray box):
> https://dl.dropboxusercontent.com/s/lh20bovp517u2wq/bug%201232806%20comment%204.zip
That behavior is the best option for markup (IMO), especially in the case of detached toolbox or toolbox docked to the right side of window. Implementing that behavior seems more like a separate "enhancement" bug, but it's possible. Nicolas Chevobbe mentioned this in bug 1195120 comment 1.

Comment 5

[Patrick Brosset <:pbro>]

We had a similar problem in the animation inspector with the scrubber. This has been fixed and works nicely now (you can start dragging then move your mouse out of the animation-inspector window and it continues to track move and mouseup events there). See devtools\client\animationinspector\components\animation-timeline.js

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 1232806 - Change mouse events' target to window in the markup view to keep track of the dragging even when the cursor is outside the window. r=jdescottes

`element.setCapture` allows to get mouse event, including mousemove, when the
cursor gets away from the target. This allow us to keep track of the dragging
state, wherever the cursor is.
We should switch to `element.setPointerEvent`, which is the standardized way of
doing this, when PointerEvent lands.
Add some tests in the `mousemove` handler to keep the dragged node inside the
markup panel.

Review commit: https://reviewboard.mozilla.org/r/47121/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47121/

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8742318 [details]
MozReview Request: Bug 1232806 - Change mouse events' target to window in the markup view to keep track of the dragging even when the cursor is outside the window. r=jdescottes

https://reviewboard.mozilla.org/r/47121/#review43747

Thanks for working on this Nicolas! Code change looks good. Some nits + 1 question below.

Before setting r+, just wanted to check: did you try attaching the mousemove/mouseup events on window instead of the document's body (same as in animation-timeline.js) ? 
I think it should work and avoids the extra complexity of set/releaseCapture. I don't have strong arguments in favor of it other than this, so up to you :)

NB: If you switch to this, pay attention to the destroy method. It is called twice for some MarkupContainer instances, and it nullifies this.win -> can crash easily if we have this.win.removeEventListener inside.

::: devtools/client/inspector/markup/markup.js:2051
(Diff revision 1)
>      let initialDiff = Math.abs(event.pageY - this._dragStartY);
>      if (this._isPreDragging && initialDiff >= DRAG_DROP_MIN_INITIAL_DISTANCE) {
>        this._isPreDragging = false;
>        this.isDragging = true;
>  
> +      // Ensure we get mouse events even if the mouse is outside the window

nit: punctuation : please add "." at the end of the sentence :)

::: devtools/client/inspector/markup/markup.js:2052
(Diff revision 1)
>      if (this._isPreDragging && initialDiff >= DRAG_DROP_MIN_INITIAL_DISTANCE) {
>        this._isPreDragging = false;
>        this.isDragging = true;
>  
> +      // Ensure we get mouse events even if the mouse is outside the window
> +      // Switch to setPointerCapture when PointerEvent is ready ( Bug 1166347)

nit: same as above. Please also remove the extra space between "(" and "Bug"

::: devtools/client/inspector/markup/markup.js:2067
(Diff revision 1)
>  
>      if (this.isDragging) {
> -      let diff = event.pageY - this._dragStartY;
> +      let x = event.pageX - this.win.scrollX;
> +      let y = event.pageY - this.win.scrollY;
> +
> +      // Ensure we keep the dragged element within the markup view

nit : same as above

::: devtools/client/inspector/markup/markup.js:2071
(Diff revision 1)
> +
> +      // Ensure we keep the dragged element within the markup view
> +      if (x < 0) {
> +        x = 0;
> +      } else if (x >= this.markup.doc.body.offsetWidth - this.win.scrollX) {
> +        x = this.markup.doc.body.offsetWidth - this.win.scrollX - 1;

(I assume the goal is to always have a valid drop position here)

If we have a vertical scrollbar and we start dragging on the right of the window, elementFromPoint will hit the scrollbar of the window and will not help find a valid drop target.

We actually don't really care about the x coordinate here. If the goal is to make sure we have a valid one, we could simply always default to 0?

::: devtools/client/inspector/markup/markup.js:2096
(Diff revision 1)
>  
>      this._isPreDragging = false;
>      this.isDragging = false;
>      this.elt.style.removeProperty("top");
> +
> +    // Switch to releasePointerCapture when PointerEvent is ready ( Bug 1166347)

nit : same as above

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8742318 [details]
MozReview Request: Bug 1232806 - Change mouse events' target to window in the markup view to keep track of the dragging even when the cursor is outside the window. r=jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47121/diff/1-2/

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/47121/#review43747

I thought I had, but I tried and it works. Thanks for pointing this, code looks simpler without setCapture.

> nit: punctuation : please add "." at the end of the sentence :)

comment removed

> nit: same as above. Please also remove the extra space between "(" and "Bug"

comment removed

> nit : same as above

comment removed

> (I assume the goal is to always have a valid drop position here)
> 
> If we have a vertical scrollbar and we start dragging on the right of the window, elementFromPoint will hit the scrollbar of the window and will not help find a valid drop target.
> 
> We actually don't really care about the x coordinate here. If the goal is to make sure we have a valid one, we could simply always default to 0?

It works well with `0` indeed

> nit : same as above

comment removed

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8742318 [details]
MozReview Request: Bug 1232806 - Change mouse events' target to window in the markup view to keep track of the dragging even when the cursor is outside the window. r=jdescottes

https://reviewboard.mozilla.org/r/47121/#review44083

Looks good to me, thanks a lot! 

Just pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcdedaff52da
(if you had a pending push, feel free to ignore this one!)

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

TRY ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcdedaff52da ) is over and except a few known intermittent everything looks good

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e2f5cd553b44

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2f5cd553b44

Comment 14

[Sebastian Zartner [:sebo]]

This doesn't seem to be fixed for me using Nightly 2016-05-03.
After step 5 of the test case I am still dragging the three elements and Escape does only cancel the last drag action.

Sebastian

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Sebastian, I just tested it on said Nightly and can't reproduce. Could you upload a screencast showing the bug ?

Comment 16

[Sebastian Zartner [:sebo]]

I just realized that this only happens when e10s is disabled. With e10s enabled it works as expected.
The behavior with e10s disabled looks the same as the screencast attached by arni2033.
Should I create a new bug?

Sebastian

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Okay, I successfully reproduced in Nightly with e10s disabled. I did not know that there were differences on how Firefox handled events with and without e10s.

I don't know if we should keep this bug open or create a new one.
Patrick, what do you think ?

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #17)
> I don't know if we should keep this bug open or create a new one.
> Patrick, what do you think ?
Better to close this bug as fixed and file another one specifically for fixing the issue without e10s, and prioritize it separately.