626368 - Empty space left between tabs when closing multiple pages in Panorama view

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[George Carstoiu]

Attachment: Screenshot 1

Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110116 Firefox/4.0b10pre

Reproducible: Always

When closing multiple tabs in panorama view using middle click, empty spaces are created which are not filled by following tabs. (See screenshot)

Steps to reproduce:
1. Open multiple tabs
2. Go to panorama
3. Middle click one of the tabs (hold click down and drag the tab a bit in any direction)

Actual results:
 - an empty space is left in the group

Expected results:
 - the tab closes and the following tab takes its place in the group

Additional information:
 - in tab view, the closed tabs from panorama are still present

Comment 1

[George Carstoiu]

Attachment: Screenshot 2

Comment 2

[George Carstoiu]

Attachment: Screenshot 3

Comment 3

[George Carstoiu]

Screenshot 2 and Screenshot 3 come from the same session. Closed tabs in Panorama still present in tab view.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Confirming. I'm not quite sure what the behaviour should be. Should the tab be closed when dragging with the middle mouse button?

Comment 5

[George Carstoiu]

Middle mouse dragging is not used in tab view, maybe it shouldn't be used in Panorama either.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #4)
> I'm not quite sure what the behaviour should be. Should the tab be
> closed when dragging with the middle mouse button?

Indeed, based on our code, I would have expected this behavior to be the same as left-click dragging. The question, then, is how did the tab close. I unfortunately don't have a middle button and don't know how to simulate one easily so it's hard for me to reproduce this and investigate... :(

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

This patch disables drag/drop via middle click for groups and tabs.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1b (unrotted)

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1c (unrotted)

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Passed try.

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 504968 [details] [diff] [review]
patch v1c (unrotted)

Looks good. 

>-        if (Utils.isRightClick(e))
>+        if (Utils.isRightClick(e) || e.button == 1)

Maybe we want a Utils.isMiddleClick()?

For that matter, are there other places we use isRightClick where we should also be checking for middle click? If so, fix here or file follow ups.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #11)
> Maybe we want a Utils.isMiddleClick()?
> 
> For that matter, are there other places we use isRightClick where we should
> also be checking for middle click? If so, fix here or file follow ups.

There is now Utils.isMiddleClick() and Utils.isLeftClick(). I replaced isRightClick() at the places where we also want to check for middle click.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Passed try.

Comment 14

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

Comment on attachment 505667 [details] [diff] [review]
patch v2

a=beltzner, thanks for tests!

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 16

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/f0f8c5391540

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Backed out: http://hg.mozilla.org/mozilla-central/rev/21a7c88123ca

This one was backed out together with bug 625424 but this one does not cause orange. So please feel free to push again :)

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/908ae37abec8

Comment 19

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre

Confirming fix.

Comment 20

[Tracy Walker [:tracy]]

verified with nightly build of minefield.