Closed Bug 1176785 Opened 9 years ago Closed 9 years ago

Can't exit 'drag' mode when moving elements without parent in Inspector

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Version:
40 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox43 --- verified

People

(Reporter: arni2033, Assigned: chiragbhatia2006, Mentored)

References

Details

Attachments

(3 files, 4 obsolete files)

Successful performed drag.webm
456.86 KB, video/webm
Details
Cant finish the drag.webm
606.56 KB, video/webm
Details
1176785.5.patch
8.48 KB, patch
pbro
: review+
Details | Diff | Splinter Review 
1. Open page data:text/html,<span>
2. Try to drag <html> element

Result:
Once you enter drag-mode, you can't exit and inspector is unresponsive.
So, you may accidentally start dragging element and lose ability to change attributes etc.

(Please fix the title if needed)
Can you provide a screencast? I can't even drag the <html> element in Nightly (I just see text selection happening), so I'm confused about what you're seeing.
Flags: needinfo?(arni2033)
I knew I should make the screencast. Here's basic drag.
Flags: needinfo?(arni2033)
And here's the 'bad' drag. You can't exit it  Note that this happens with every root element - it could also be <svg>
Can repro now on my win10 tablet. Patrick, can you have a look?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(pbrosset)
So, the problem seems to be happening when the html node has no siblings in the inspector.
For instance, this doesn't happen with this: data:text/html,<!DOCTYPE html><span>
I think we should do 2 things:
- prevent dragging the root node if there's nowhere to drag it to,
- bind escape key press so that instead of bringing the split webconsole up, it cancels the drag.
The first one is the real fix, but it looks to me that the second one would help too, and feels like a natural thing to have (maybe for another bug though).
Flags: needinfo?(pbrosset)
Won't have time to look at it this week (at least), so setting myself as mentor. I can provide information about how to get started on a fix to whoever is interested. This should be a relatively trivial fix.
Mentor: pbrosset
Hi Patrick,

I'm interested in working on this. Can you please provide me some info on how to get started on this?
Flags: needinfo?(pbrosset)
The node dragging mechanism starts here:
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1939

As you can see, on mouse down, we wait for a short delay and then, if the mouse is still down and the node is draggable, we actually start dragging.

I suggest refactoring the first |if| condition in this setTimeout callback into a new method called |isDraggable| or something.
This new method would check everything the |if| does right now plus check that if the node is the <html> tag, it has siblings.

Now, for the second part, where we want the ESCAPE key to stop dragging: there's a keydown listener callback here: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#530
Adding a case in the |switch| that checks if the key was escape *and* if |this.isDragging| is true should be quite simple. I haven't checked the details, but it seems feasible to cancel the dragging from there.
Flags: needinfo?(pbrosset)
Attached patch 1176785.1.patch (obsolete) — — Splinter Review 
Hi Patrick,

I refactored the if condition in a new method (not sure if the placement of the method definition is correct).

I tried adding the Switch case for the Escape key, but realized that when we drag any element in the devtools, pressing the Escape key triggers the console below the devtools. Should we add another event listener to it?
Flags: needinfo?(pbrosset)
Attachment #8632812 - Flags: review?(pbrosset)
Comment on attachment 8632812 [details] [diff] [review]
1176785.1.patch

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

Looks good. I think the function should be inside the class though (see my comments in the code below).
About the ESCAPE key handling, I'm unfortunately not able to test it right now, but I'm pretty sure we should be able to make it work with this:

in the MarkupView.prototype._onKeyDown function,
in the switch statement,
add a new case:

case Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE:
  ... do whatever is needed to stop dragging ...
  break;

because 'handled' is true by default, the 'if (handled)' after the switch will make sure the even doesn't go any further and the split console doesn't open.
Did you try this?
Maybe I missed something, and as I said, I can't test it right now.

The last thing is, we'll need new tests once this is fixed:
- one new test in /browser/devtools/markupview/test that makes sure a <html> node with no siblings can't be dragged
- one new test in /browser/devtools/markupview/test that makes sure pressing ESCAPE cancels the dragging
We already have 5 dragdrop-related tests in this folder: browser_markupview_dragdrop*. I suggest taking a look at those to understand how these tests are written.

Don't hesitate to ask for further help if you need on the #devtools IRC channel as I'm going to be away until beg. of August so I may be slow in answering.

::: browser/devtools/markupview/markup-view.js
@@ +1828,5 @@
>      // Start dragging the container after a delay.
>      this.markup._dragStartEl = target;
>      setTimeout(() => {
>        // Make sure the mouse is still down and on target.
> +      if (!IsDraggable(this, target)) {

I think IsDraggable should be on the prototype of the MarkupContainer class. So this line would instead be:

if (!this.isDraggable(target)) {

@@ +2018,5 @@
>  
>  /**
> + * Check if element is draggable
> + */
> +function IsDraggable(el, target) {

As said above, move this method inside the prototype of MarkupContainer. You could put it just after get isDragging().
And please lowercase the first character, we try and capitalize only contrusctor names.

isDraggable: function(target) {
  ...
},
Attachment #8632812 - Flags: review?(pbrosset)
Sorry for interrupting here, but it was said, that this issue happens not only to <html> element, but to any ROOT element without siblings (e.g. <xul:window>, <svg>, <html>, <show> )
(In reply to arni2033 from comment #11)
> Sorry for interrupting here, but it was said, that this issue happens not
> only to <html> element, but to any ROOT element without siblings (e.g.
> <xul:window>, <svg>, <html>, <show> )
Correct. I did not point this out in the previous review because there are other things to fix first, but you are very correct. The isDraggable method will need to use information from 'this.node' to determine if the node has siblings and if it is a root node.
For info, 'this.node' is a NodeFront type, which is defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#737
Flags: needinfo?(pbrosset)
Wait, we already have a bug for implementing Escape key behavior.
Are you going to implement 2 features in this bug instead?
I'm marking this as blocking, so that people don't try to resolve bug 1168132
Blocks: 1168132
Attached patch 1176785.2.patch (obsolete) — — Splinter Review 
Hi Patrick,

I've made the changes in the code as per your suggestions. I'm however unable to figure out what code to use to trigger the stop of node dragging when the user presses Escape. Can you please suggest what I'm missing here? I tried a lot of trial and error but no success. :(
Attachment #8632812 - Attachment is obsolete: true
Attachment #8649429 - Flags: review?(pbrosset)
Comment on attachment 8649429 [details] [diff] [review]
1176785.2.patch

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

::: browser/devtools/markupview/markup-view.js
@@ +578,5 @@
>          this.beginEditingOuterHTML(this._selectedContainer.node);
>          break;
>        }
> +      case Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE: {
> +        if (this.isDragging) {

So, here you'll need to cancel the dragging. The dragging mechanism is a bit all over the place in this file I'm afraid, some of it is in MarkupContainer (that's the most important part), and some here in MarkupView where you need to unmark the drag and drop targets.
If you look at the _onMouseUp callbacks in these 2 classes, you'll see what needs to be done to cancel the process.
I'll try to put a quick patch together to show you.

@@ +1814,5 @@
> +  isDraggable: function(target) {
> +    return (this._isMouseDown && this.markup._dragStartEl === target &&
> +      !this.node.isPseudoElement && !this.node.isAnonymous &&
> +      this.win.getSelection().isCollapsed &&
> +      this.node._parent.tagName !== null);

This is good, just a few minor comments:
- formatting: we usually format these types of long conditions like so:

return this._isMouseDown &&
       this.markup._dragStartEl === target &&
       !this.node.isPseudoElement &&
       ...

- instead of this.node._parent, please use this.node.parentNode()
Attachment #8649429 - Flags: review?(pbrosset)
Attached patch 1176785.2.patch (obsolete) — — Splinter Review 
This should work. I hope this helps understand how this works a bit better.
Please try and test this thoroughly, I might have missed some buggy corner cases.
Finally, as I mentioned previously, we'll need a new test for this (see comment 10). Let me know if I can be of more help.
Attachment #8649429 - Attachment is obsolete: true
Attached patch 1176785.4.patch (obsolete) — — Splinter Review 
Thanks for the patch, that really helped.

I've added an extra condition in your patch within the switch-case where we need to check if we're dragging any node before trying to cancel dragging, because when we aren't dragging, the break in the case should not get hit. If it does, the console below the devtools will never show up even when we're not dragging any node.

I've also added the two tests that you've mentioned by cloning one of the existing tests. Let me know if you see anything wrong and want me to make any changes.
Attachment #8651208 - Flags: review?(pbrosset)
Blocks: 1198040
Attachment #8650371 - Attachment is obsolete: true
Comment on attachment 8651208 [details] [diff] [review]
1176785.4.patch

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

Just a couple of minor changes to make and we're good to go. Good work on this.
Make sure you run the tests locally too.

::: browser/devtools/markupview/test/browser.ini
@@ +44,5 @@
>  [browser_markupview_dragdrop_autoscroll.js]
>  [browser_markupview_dragdrop_invalidNodes.js]
>  [browser_markupview_dragdrop_isDragging.js]
> +[browser_markupview_dragdrop_dragRootNode.js]
> +[browser_markupview_dragdrop_escapeKeyPress.js]

Please keep the list of tests sorted alphabetically (i.e. these 2 new tests should come just after browser_markupview_dragdrop_autoscroll.js).

::: browser/devtools/markupview/test/browser_markupview_dragdrop_dragRootNode.js
@@ +19,5 @@
> +  el._onMouseDown({
> +    target: el.tagLine,
> +    pageX: rect.x,
> +    pageY: rect.y,
> +    stopPropagation: function() {}

You also need to add this line:
preventDefault: function() {}

I ran the test locally and it failed because preventDefault wasn't defined. That's because in the drag-drop tests, we call the event handler callbacks directly, faking the DOM event by sending a normal object instead. That's fine, but it means we must make sure this DOM event mock object has all the properties and methods that the callback expects to find. In this case, preventDefault was needed.

Note that you can run the tests locally with:
./mach test browser/devtools/markupview/test/browser_markupview_dragdrop_dragRootNode.js
or 
./mach test browser/devtools/markupview/test/
to run all markup-view tests.

It's a good idea to run them all locally once to make sure things work like before, and then, once done, we'll send the patch to our TRY server so all tests are run on all platforms.

@@ +22,5 @@
> +    pageY: rect.y,
> +    stopPropagation: function() {}
> +  });
> +
> +  yield wait(GRAB_DELAY + 1);

Can you add a comment just above this line saying something like:
info("Waiting for a little bit more than the markup-view grab delay");

Otherwise the +1 looks weird for someone who doesn't know the code well.

::: browser/devtools/markupview/test/browser_markupview_dragdrop_escapeKeyPress.js
@@ +19,5 @@
> +  el._onMouseDown({
> +    target: el.tagLine,
> +    pageX: rect.x,
> +    pageY: rect.y,
> +    stopPropagation: function() {}

Same comment about preventDefault than in previous test.

@@ +22,5 @@
> +    pageY: rect.y,
> +    stopPropagation: function() {}
> +  });
> +
> +  yield wait(GRAB_DELAY + 1);

Same comment about adding an info(...) than in previous test.
Attachment #8651208 - Flags: review?(pbrosset)
Blocks: 858038
Attached patch 1176785.5.patch — — Splinter Review 
I've made the changes and sent it to try server for all tests on all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0a7839155f8
Attachment #8651208 - Attachment is obsolete: true
Attachment #8652957 - Flags: review?(pbrosset)
Comment on attachment 8652957 [details] [diff] [review]
1176785.5.patch

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

This looks good to me.
I've pushed the patch to TRY to make sure tests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=463d9985a9f5
Attachment #8652957 - Flags: review?(pbrosset) → review+
Patrick, did I do something wrong during my push to the TRY server? Not sure why all builds failed earlier when I pushed them?
(In reply to Chirag Bhatia from comment #22)
> Patrick, did I do something wrong during my push to the TRY server? Not sure
> why all builds failed earlier when I pushed them?
Oh I didn't even see you had already pushed to TRY, sorry.
Looking at the push URL, I don't see anything wrong, I don't know what happened, but it's probably not related to what you did.
Hi Patrick. Please assign this bug to me. And thanks for all your help and pointers with this :)
My pleasure! You're right, the status and assignee are now correct.
Assignee: nobody → chiragbhatia2006
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/084815f654f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
I have reproduced this bug on Nightly 41.0a1 (2015-06-22) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Beta 43.0b1!

Build ID: 20151103023037
User Agent: Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0

[bugday-20151104]
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
Has STR: --- → yes
Summary: Can't exit 'drag' mode when moving elements in Inspector → Can't exit 'drag' mode when moving elements without parent in Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: