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

VERIFIED FIXED in Firefox 4.0b11

Status

defect
P3
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: george.carstoiu, Assigned: ttaubert)

Tracking

Trunk
Firefox 4.0b11
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
Posted image 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
Posted image Screenshot 2
(Reporter)

Comment 2

8 years ago
Posted image 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.
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... :(
Posted patch patch v1 (obsolete) — Splinter Review
This patch disables drag/drop via middle click for groups and tabs.
Attachment #504536 - Flags: review?(ian)
Posted patch patch v1b (unrotted) (obsolete) — Splinter Review
Attachment #504536 - Attachment is obsolete: true
Attachment #504967 - Flags: review?(ian)
Attachment #504536 - Flags: review?(ian)
Posted patch patch v1c (unrotted) (obsolete) — Splinter Review
Attachment #504967 - Attachment is obsolete: true
Attachment #504968 - Flags: review?(ian)
Attachment #504967 - Flags: review?(ian)
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+
Posted patch patch v2 (obsolete) — Splinter Review
(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
Passed try.
Comment on attachment 505667 [details] [diff] [review]
patch v2

a=beltzner, thanks for tests!
Attachment #505667 - Flags: approval2.0? → approval2.0+
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
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 → ---

Comment 18

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