1202179 - Drag and drop in Inspector should start ONLY after the actual mouse drag, not after "Press and hold left mouse button"

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[arni2033]

Attachment: testpage - Drag and drop in Inspector should start ONLY after the actual mouse drag, not after 'Press and hold left mouse button'.htm

STR:   (Win7_64, Nightly 43, 32bit, ID 20150901030226, new profile, safe mode)
1. Open attached page
2. Right-click gray area, click "Inspect Element".
3. Detach devtools into separate window, maximize that window
4. Hide sidebar   [actually it's super easy to reproduce, I just needed reliable STR]
5. Click the 1st <p> element in markup-view
6. Move mouse to the second <p> element and click it

Result:       Drag and drop started, because you pressed mouse, and 'mouseup' event didn't fire on that element
Expectations: Drag and drop shouldn't start in this case

Note: If you disagree with the Summary (which also is proposed solution), it's not the reason to close this bug. In this case it should be renamed, because the issue is still there.
I filed this bug with such Summary because otherwise I would have to file 2 reports.

Comment 1

[Brian Grinstead [:bgrins]]

Patrick, what do you think?  You can reproduce the same thing by selecting a node and then holding the mouse down on it.  Notice that the UI enters drag mode in that case even though you've never moved the mouse.  I think the proposed behavior is saying the drag shouldn't start until the mouse has been held down for N seconds AND the mouse moves.

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> Patrick, what do you think?  You can reproduce the same thing by selecting a
> node and then holding the mouse down on it.  Notice that the UI enters drag
> mode in that case even though you've never moved the mouse.  I think the
> proposed behavior is saying the drag shouldn't start until the mouse has
> been held down for N seconds AND the mouse moves.
That makes sense.

Comment 3

[arni2033]

(Sorry for spam) Currently right-click also starts the drag... Context menu appears when it ends.
(1) Is that correct behavior? (also middle-clicks also start the drag)
(2) If not, is it going to be fixed in this bug, or it's better to file another one?

Comment 4

[Patrick Brosset <:pbro>]

(In reply to arni2033 from comment #3)
> (Sorry for spam) Currently right-click also starts the drag... Context menu
> appears when it ends.
> (1) Is that correct behavior? (also middle-clicks also start the drag)
> (2) If not, is it going to be fixed in this bug, or it's better to file
> another one?
We should take care of this in this bug. My plan is to work on this bug tomorrow.

Comment 5

[Patrick Brosset <:pbro>]

I'm working on cleaning this mess up a bit, and as a result, will most probably both fix this bug and bug 1198040 at the same time.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: Bug_1202179_-_html_head_body_not_dragdrop-able_and.diff

This patch changes the way drag/drop works in the markup-view. There's no delay for dragging anymore, instead, dragging only start if you actually move the mouse. It overall feels a lot more natural to me.

I also slightly changed the way the drag and drop target indicator looked, I think the big dashed lines looked kind of ugly, but I'm open to feedback.

I had to rewrite much of the existing tests since they were all based on waiting for the delay. I took this opportunity to move some of the common code to head.js.
I removed 2 tests: one was testing the delay only (we don't need it anymore), and one was testing that drag/drop didn't work when text was selected: turns out we disabled text selections some time ago in bug 968241 so this test should have been removed then.

Tests pass locally, I'll provide a try push URL soon.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8678826 [details] [diff] [review]
Bug_1202179_-_html_head_body_not_dragdrop-able_and.diff

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

First round of feedback - the new behavior feels much more natural and I really like it

::: devtools/client/markupview/markup-view.css
@@ +75,5 @@
>   * by adding height: 0 we let surrounding elements to fill the blank space */
>  .child.dragging {
>    position: relative;
>    pointer-events: none;
>    opacity: 0.7;

I know this in a current issue, but the opacity could probably even be a bit lower.   Fine to do a follow up and we can get UX feedback on this.

::: devtools/client/markupview/markup-view.js
@@ -1549,5 @@
> -        this._boundUpdatePreview, true);
> -      this._boundUpdatePreview = null;
> -    }
> -
> -    if (this._boundResizePreview) {

So much cruft here, thanks for cleaning that up

@@ +1944,4 @@
>     */
>    _onMouseMove: function(event) {
> +    // If this is the first move after mousedown, indicate the start position.
> +    if (this._isPreDragging) {

I accidentally started drags a couple of times with this patch applied, maybe we could wait to switch to dragging until the mouse has moved N vertical pixels while the mouse is down?

::: devtools/client/markupview/test/browser_markupview_dragdrop_autoscroll.js
@@ +61,5 @@
> +      }
> +    };
> +
> +    // Start checking if the view scrolled after 500ms.
> +    setTimeout(isDone, 500);

500ms seems like a long time to wait in a test, could contribute to potential timeouts on a slow test server.  May want to start out with a shorter wait, since the isDone function will be resilient to it firing too early.

Comment 9

[arni2033]

I have some thoughts.
>   1) Drag & Drop indicators
They were barely visible and they still are, even if I set opacity to 0.5 [1.1], [1.2]. The first thing that comes to mind is to indeed reduce the opacity of dragging element. Second thing is to set "z-index:2;" to indicators. Third thing is, actually, that old colors were more visible (but still not enough). Black color was completely ok for drop-target indicator on light theme. Now you're suggesting to use yellow which is less visible under (and above) white-text-on-blue of dragging element. What for dark theme... both blue and yellow are not visible enough. I'm not sure, but maybe red/violet color would be the right solution
>   2) Removing element from tree during the drag [also read (3) see also ]
When I'm going to drag a huge element down, I see pointer's coordinates and the target coordinates. So I'm prepared to drag the element by N pixels (let say 60). But when I start dragging, the element is being removed from tree, so position of all elements placed lower also changes. So I can never tell where am I going to drag it (can't specify the target's placement) before I start the actual drag.
// Bug 1202458 is good testcase for this, though in fact that bug isn't about dragging
>   3) Accidental clicks
I was going to file (2) and (3) as separate bugs.
Sometimes when I click an element, I accidentally move mouse by ~1px. I believe user should be protected from accidental drag in this case. (And drag indeed can happen if you look at (2) ).
One solution I see is to check if user moved pointer by >= 5px (or moved pointer out of the element) before starting those drag stuff.
Another solution (the best IMO) is not to remove element from tree [see (2)]
>   Screenshots (for all 3 points):
> [1.1] https://dl.dropboxusercontent.com/s/noy4xb6eu5es2ak/bug%201202179%20-%20discussion%20-%20%5B1.1%5D%20Barely%20visible%20lines%20%28opacity%20set%20to%200.5%29.png?dl=0
> [1.2] https://dl.dropboxusercontent.com/s/6v0md3o1tmjkg8n/bug%201202179%20-%20discussion%20-%20%5B1.2%5D%20Barely%20visible%20lines%20%28opacity%20set%20to%200.5%29.png?dl=0
>  [2.1] https://dl.dropboxusercontent.com/s/20dum9hulhnddre/bug%201202179%20-%20discussion%20-%20%5B2.1%5D%20Before%20drag%20start.png?dl=0
>  [2.2] https://dl.dropboxusercontent.com/s/er5e08xyo5s834m/bug%201202179%20-%20discussion%20-%20%5B2.2%5D%20After%20drag%20start.png?dl=0
>   [3.1] https://dl.dropboxusercontent.com/s/w1oshuln8zhpc8d/bug%201202179%20-%20discussion%20-%20%5B3.1%5D%20Before%20click.png?dl=0
>   [3.2] https://dl.dropboxusercontent.com/s/8es2jswawu2sb7b/bug%201202179%20-%20discussion%20-%20%5B3.2%5D%20After%20click.png?dl=0

Comment 10

[arni2033]

I forgot to mention that bookmarks sidebar does have protection from accidental click & drag by 1px (3)

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to arni2033 from comment #9)
> 3) Accidental clicks
> I was going to file (2) and (3) as separate bugs.
> Sometimes when I click an element, I accidentally move mouse by ~1px. I
> believe user should be protected from accidental drag in this case. (And
> drag indeed can happen if you look at (2) ).
> One solution I see is to check if user moved pointer by >= 5px (or moved
> pointer out of the element) before starting those drag stuff.

I believe this change is going to be added in the next version of the patch

Comment 12

[Patrick Brosset <:pbro>]

(In reply to arni2033 from comment #9)
> I have some thoughts.
> >   1) Drag & Drop indicators
Thanks, filed bug 1218716 for this.
> >   2) Removing element from tree during the drag [also read (3) see also ]
I agree this isn't great. Maybe the solution is to not remove the element from the tree at all. I believe this is something bug 1218716 should fix too.
> >   3) Accidental clicks
As Brian said, the next patch I'll attach will fix this. Thanks.

Comment 13

[Patrick Brosset <:pbro>]

Attachment: Bug_1202179_-_html_head_body_not_dragdrop-able_and.diff

Comment 14

[Patrick Brosset <:pbro>]

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8678826&action=interdiff&newid=8679359&headers=1

You'll notice a seemingly unrelated change in styleinspector/test/head.js
This is because some of the styleinspector tests were failing because inspector.markup._waitForChildren was being called without a scope. I still don't understand why my change caused this, but it's anyway something that needed fixing.

Comment 15

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76448cf0f67a

Comment 16

[arni2033]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #14)

@Patrick:   Apparently, I was a wrong about (3) [but have you really experienced that issue?]
Firefox already has a mechanism for preventing accidental click & drag: the drag starts only after I move poiner by ~4-5px (works for tabs, links, selected text, items in customization palette etc)
Do you think it's a good idea to make another bug for this one change and wait for a while?

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8679359 [details] [diff] [review]
Bug_1202179_-_html_head_body_not_dragdrop-able_and.diff

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

This is great

Comment 18

[Pulsebot]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[arni2033]

Hello, Patrick. This patch has caused a regression:
Now I can't drag comment nodes on the page data:text/html,<body><!--1--><!--2-->
I mean, I can't swap nodes <!--1--> and <!--2--> by dragging. Is this according to the plan?