580412 - Add a couple-pixel threshold before tabs start dragging

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Aza Raskin [:aza]]

One of the problems we see cropping up for mouse-users of Tabcandy is that they try to zoom into a tab, but end up dragging it slightly. We need to add a slight threshold before a tab starts dragging so that slight motion in the cursor when click doesn't defeat the intent of the user.

Comment 1

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 2

[Sean Dunn]

Attachment: Proposed patch to fix bug 580412

Comment 3

[Ian Gilman [:iangilman]]

Comment on attachment 472989 [details] [diff] [review]
Proposed patch to fix bug 580412

First patch! Looking good. 

Please put the rest of the drag actions (calling the drag callback, checking for droppable overs) inside the !startSent check along with the setBounds; basically if we haven't crossed the threshold, we shouldn't act like we're dragging at all.

Comment 4

[Sean Dunn]

Attachment: Proposed patch v2

Comment 5

[Sean Dunn]

Attachment: v3

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 473242 [details] [diff] [review]
v3

Looks good to me. Dietrich: this is not crucial for b6, but good to have whenever we can. 

Pushing to try server shortly.

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

This, I believe, will fix a todo item in my test for bug 591167. I will try to check that test with this patch later today. Hopefully we can push them together.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Sean, it's not pushed yet, but bug 591167 includes a test which you should make sure you don't break: browser_tabview_snapping.js.

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 473242 [details] [diff] [review]
v3

>       stop: function() {
>         drag.info.stop();
>         drag.info = null;
>-      }
>+      },
>+      minDragDistance: 3 // The minimum the mouse must move after mouseDown in order to move an item

nit: 80 char line length

>         var mouse = new Point(e.pageX, e.pageY);
>-        var box = self.getBounds();
>-        box.left = startPos.x + (mouse.x - startMouse.x);
>-        box.top = startPos.y + (mouse.y - startMouse.y);
>+		

remove extra whitespace

>+        if (!startSent){
>+          if(Math.abs(mouse.x-startMouse.x) > self.dragOptions.minDragDistance || 
>+             Math.abs(mouse.y-startMouse.y) > self.dragOptions.minDragDistance){

spaces after end parens

r+ with these fixed.

Comment 10

[Sean Dunn]

Attachment: v4

Comment 11

[Sean Dunn]

Attachment: v4 (redone to fix bad spacing)

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 473435 [details] [diff] [review]
v4 (redone to fix bad spacing)

>+        if (!startSent){
>+          if(Math.abs(mouse.x-startMouse.x)>self.dragOptions.minDragDistance||
>+             Math.abs(mouse.y-startMouse.y)>self.dragOptions.minDragDistance){

still need the spaces after end parens. also, add spaces between operators. code is harder to read when all squished together like this. think of the children.

Comment 14

[Sean Dunn]

Attachment: v5

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 473438 [details] [diff] [review]
v5


>+        if (startSent) {
>+	        // drag events
>+	        var box = self.getBounds();
>+          box.left = startPos.x + (mouse.x - startMouse.x);
>+          box.top = startPos.y + (mouse.y - startMouse.y);

fix indent.

r=me otherwise!

Comment 16

[Sean Dunn]

Attachment: v6

Alright, I think the agonizing white space is all fixed.

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 473443 [details] [diff] [review]
v6

r+ on the code changes. i know this will sound like yet more agony, but this needs a test. sorry for not calling that out sooner. check out the pre-existing tabview tests for examples. i'm pretty sure there are drag-and-drop ones there already that you can use as a template.

Comment 18

[Sean Dunn]

(In reply to comment #17)
> r+ on the code changes. i know this will sound like yet more agony, but this
> needs a test. sorry for not calling that out sooner. check out the pre-existing
> tabview tests for examples. i'm pretty sure there are drag-and-drop ones there
> already that you can use as a template.

I've already run the tabview mochitests including changes with mitcho's patch for bug 591167. All tests passed. Is there anything specifically you think would be good to try?

Comment 19

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #18)
> (In reply to comment #17)
> > r+ on the code changes. i know this will sound like yet more agony, but this
> > needs a test. sorry for not calling that out sooner. check out the pre-existing
> > tabview tests for examples. i'm pretty sure there are drag-and-drop ones there
> > already that you can use as a template.
> 
> I've already run the tabview mochitests including changes with mitcho's patch
> for bug 591167. All tests passed. Is there anything specifically you think
> would be good to try?

We should be able to add a step to my new snapping test which just clicks on a group, moves it one or two pixels over and drops it, and confirm that it didn't move.

I may get around to writing that step later, but otherwise, Sean, find me on IRC and I can give you some pointers.

Dietrich, is adding a step to a separate test file legit, or does it have to be another file? (In which case we can just reuse most of the code from the snapping test instead of modifying it.)

Comment 20

[Ian Gilman [:iangilman]]

(In reply to comment #19)
> Dietrich, is adding a step to a separate test file legit, or does it have to be
> another file? (In which case we can just reuse most of the code from the
> snapping test instead of modifying it.)

I would think updating an existing test would be better, if there's one that's appropriate.

Comment 21

[Michael Yoshitaka Erlewine [:mitcho]]

I'll write a test for this.

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch with test

Last patch (v6) already got r+. This is just adding the test. Sending to try now.

Comment 23

[Michael Yoshitaka Erlewine [:mitcho]]

Botched try run. Try-ing again.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 476706 [details] [diff] [review]
Patch with test

a=beltzner

Comment 25

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 26

[Ian Gilman [:iangilman]]

(In reply to comment #23)
> Botched try run. Try-ing again.

I assume the try was clean?

Comment 27

[Ian Gilman [:iangilman]]

Pushed to panorama-central:

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/55bd5d5174ff

Comment 29

[Raymond Lee [:raymondlee]]

(In reply to comment #27)
> Pushed to panorama-central:
> 
> http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/55bd5d5174ff

Mitcho: the test browser_tabview_bug580412.js file is missing in this patch. Please include it in the patch and also push the test file to the panorama-central.  Thanks!

Comment 30

[Michael Yoshitaka Erlewine [:mitcho]]

I'm sorry, I seem to have messed this up. The test was never checked in, so it was never actually part of a try run. I've pushing to try now again with the test. Hopefully we'll have results soon and it'll have passed.

If we don't get results soon or it doesn't immediately pass, though, we should probably backout the panorama-central push. I'll keep you all updated.

Comment 31

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin, with test

Comment 32

[Michael Yoshitaka Erlewine [:mitcho]]

Passed try, updated patch for m-c checkin, pushed test file to p-c:

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/6d8dd9ed55e6

Comment 33

[Ian Gilman [:iangilman]]

Landed on mozilla-central: 

http://hg.mozilla.org/mozilla-central/rev/55bd5d5174ff
http://hg.mozilla.org/mozilla-central/rev/6d8dd9ed55e6

Comment 34

[Tracy Walker [:tracy]]

verified with recent nightly build of minefield