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.

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

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 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.

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.

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 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 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 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!

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

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.

(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?

(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.)

(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.

I'll write a test for this.

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

Botched try run. Try-ing again.

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

(In reply to comment #23) > Botched try run. Try-ing again. I assume the try was clean?

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

(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!

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.

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

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

verified with recent nightly build of minefield