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

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: arni2033, Assigned: nchevobbe)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 affected, firefox48 fixed)

Details

(Whiteboard: [btpp-fix-later], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8698644 [details]
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
(Reporter)

Updated

2 years ago
Summary: It's possible to accidentally end up dragging several nodes in markup → It's possible to accidentally end up dragging several nodes in markup at once
(Reporter)

Updated

2 years ago
Has STR: --- → yes
Triaging (filter on CLIMBING SHOES).
Priority: -- → P2
Whiteboard: [btpp-fix-later]
(Assignee)

Updated

2 years ago
Assignee: nobody → chevobbe.nicolas
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
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 ?
Flags: needinfo?(pbrosset)
(Reporter)

Comment 3

2 years ago
> 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.
(Reporter)

Comment 4

2 years ago
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.
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
Flags: needinfo?(pbrosset)
(Assignee)

Comment 6

2 years ago
Created 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

`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/
Attachment #8742318 - Flags: review?(jdescottes)
Attachment #8742318 - Flags: review?(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
(Assignee)

Comment 8

2 years ago
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/
Attachment #8742318 - Attachment description: MozReview Request: Bug 1232806 - Add `setCapture` to the dragged element in the markup view to track outside target mouse events. r=jdescottes → 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
Attachment #8742318 - Flags: review?(jdescottes)
(Assignee)

Comment 9

2 years ago
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 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!)
Attachment #8742318 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 11

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

Comment 12

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/e2f5cd553b44
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e2f5cd553b44
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

2 years ago
Sebastian, I just tested it on said Nightly and can't reproduce. Could you upload a screencast showing the bug ?
Flags: needinfo?(sebastianzartner)
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
Flags: needinfo?(sebastianzartner)
Flags: needinfo?(chevobbe.nicolas)
(Assignee)

Comment 17

2 years ago
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 ?
Flags: needinfo?(chevobbe.nicolas) → needinfo?(pbrosset)
(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.
Flags: needinfo?(pbrosset)
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Blocks: 1270788
(Reporter)

Updated

11 months ago
Depends on: 1327749
You need to log in before you can comment on or make changes to this bug.