Add a couple-pixel threshold before tabs start dragging

VERIFIED FIXED in Future

Status

Firefox Graveyard
Panorama
P2
normal
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: aza, Assigned: Sean Dunn)

Tracking

Dependency tree / graph

Details

(Whiteboard: [on-panorama-central])

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

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

Updated

8 years ago
Assignee: nobody → ian

Comment 1

7 years ago
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Component: TabCandy → TabCandy
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Version: unspecified → Trunk

Updated

7 years ago
QA Contact: tabcandy → tabcandy
Blocks: 587503
(Assignee)

Comment 2

7 years ago
Created attachment 472989 [details] [diff] [review]
Proposed patch to fix bug 580412
(Assignee)

Updated

7 years ago
Attachment #472989 - Flags: feedback?(ian)
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.
(Assignee)

Comment 4

7 years ago
Created attachment 473188 [details] [diff] [review]
Proposed patch v2
Attachment #472989 - Attachment is obsolete: true
Attachment #473188 - Flags: feedback?(ian)
Attachment #472989 - Flags: feedback?(ian)
(Assignee)

Updated

7 years ago
Assignee: ian → seanedunn
Attachment #473188 - Attachment is patch: true
Attachment #473188 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 5

7 years ago
Created attachment 473242 [details] [diff] [review]
v3
Attachment #473188 - Attachment is obsolete: true
Attachment #473188 - Flags: feedback?(ian)
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.
Attachment #473242 - Flags: review?(dietrich)
Attachment #473242 - Flags: feedback+
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.
Hardware: ARM → All
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.
Attachment #473242 - Flags: review?(dietrich) → review+
(Assignee)

Comment 10

7 years ago
Created attachment 473432 [details] [diff] [review]
v4
Attachment #473242 - Attachment is obsolete: true
Attachment #473432 - Flags: review?(dietrich)
(Assignee)

Comment 11

7 years ago
Created attachment 473435 [details] [diff] [review]
v4 (redone to fix bad spacing)
Attachment #473432 - Attachment is obsolete: true
Attachment #473435 - Flags: review?(dietrich)
Attachment #473432 - Flags: review?(dietrich)
Duplicate of this bug: 593402
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.
Attachment #473435 - Flags: review?(dietrich)
(Assignee)

Comment 14

7 years ago
Created attachment 473438 [details] [diff] [review]
v5
Attachment #473435 - Attachment is obsolete: true
Attachment #473438 - Flags: review?(dietrich)
Attachment #473438 - Attachment is patch: true
Attachment #473438 - Attachment mime type: application/octet-stream → text/plain
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!
Attachment #473438 - Flags: review?(dietrich) → review+
(Assignee)

Comment 16

7 years ago
Created attachment 473443 [details] [diff] [review]
v6

Alright, I think the agonizing white space is all fixed.
Attachment #473438 - Attachment is obsolete: true
Attachment #473443 - Flags: review?(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.
Attachment #473443 - Flags: review?(dietrich) → review+
(Assignee)

Comment 18

7 years ago
(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.
Assignee: seanedunn → mitcho
Priority: -- → P3
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
Priority: P3 → P2
Target Milestone: --- → Future
(Reporter)

Updated

7 years ago
Blocks: 597043
Created attachment 476706 [details] [diff] [review]
Patch with test

Last patch (v6) already got r+. This is just adding the test. Sending to try now.
Attachment #473443 - Attachment is obsolete: true
Attachment #476706 - Flags: approval2.0?
Blocks: 598167
Botched try run. Try-ing again.
Comment on attachment 476706 [details] [diff] [review]
Patch with test

a=beltzner
Attachment #476706 - Flags: approval2.0? → approval2.0+
blocking2.0: ? → final+
Keywords: polish
Assignee: mitcho → seanedunn
Created attachment 477184 [details] [diff] [review]
Patch for checkin
Attachment #476706 - Attachment is obsolete: true
Keywords: checkin-needed
Status: NEW → ASSIGNED
(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
Keywords: checkin-needed, polish
Whiteboard: [on-panorama-central]
Duplicate of this bug: 598167
(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.
Created attachment 479792 [details] [diff] [review]
Patch for checkin, with test
Attachment #477184 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
verified with recent nightly build of minefield
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.