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

VERIFIED FIXED in Firefox 4.0b11

Status

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

People

(Reporter: George Carstoiu, Assigned: ttaubert)

Tracking

Trunk
Firefox 4.0b11
Bug Flags:
in-testsuite +

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 504405 [details]
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
(Reporter)

Comment 1

8 years ago
Created attachment 504408 [details]
Screenshot 2
(Reporter)

Comment 2

8 years ago
Created attachment 504410 [details]
Screenshot 3
(Reporter)

Comment 3

8 years ago
Screenshot 2 and Screenshot 3 come from the same session. Closed tabs in Panorama still present in tab view.
(Assignee)

Comment 4

8 years ago
Confirming. I'm not quite sure what the behaviour should be. Should the tab be closed when dragging with the middle mouse button?
(Assignee)

Updated

8 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
(Reporter)

Comment 5

8 years ago
Middle mouse dragging is not used in tab view, maybe it shouldn't be used in Panorama either.
(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... :(
(Assignee)

Comment 7

8 years ago
Created attachment 504536 [details] [diff] [review]
patch v1

This patch disables drag/drop via middle click for groups and tabs.
Attachment #504536 - Flags: review?(ian)
(Assignee)

Comment 8

8 years ago
Created attachment 504967 [details] [diff] [review]
patch v1b (unrotted)
Attachment #504536 - Attachment is obsolete: true
Attachment #504967 - Flags: review?(ian)
Attachment #504536 - Flags: review?(ian)
(Assignee)

Comment 9

8 years ago
Created attachment 504968 [details] [diff] [review]
patch v1c (unrotted)
Attachment #504967 - Attachment is obsolete: true
Attachment #504968 - Flags: review?(ian)
Attachment #504967 - Flags: review?(ian)
(Assignee)

Comment 10

8 years ago
Passed try.
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.
Attachment #504968 - Flags: review?(ian) → review+
(Assignee)

Comment 12

8 years ago
Created attachment 505667 [details] [diff] [review]
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.
Attachment #504968 - Attachment is obsolete: true
Attachment #505667 - Flags: approval2.0?
Blocks: 627096
Priority: -- → P3
(Assignee)

Comment 13

8 years ago
Passed try.
Comment on attachment 505667 [details] [diff] [review]
patch v2

a=beltzner, thanks for tests!
Attachment #505667 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 15

8 years ago
Created attachment 506882 [details] [diff] [review]
patch for checkin
Attachment #505667 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/f0f8c5391540
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 17

8 years ago
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 :)
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/908ae37abec8
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b11
(Reporter)

Comment 19

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

Confirming fix.
verified with 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.