600812 - Dragging tab between groups doesn't work

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Dietrich Ayala (:dietrich)]

sometimes i'll drag a tab from group A to group B.

expected: the tabs in group A rearrange to compensate for the now-missing tab. the tabs in group B rearrange to compensate for the newly-arrived tab.

actual: nothing moves in group A, and there's a hole where the tab used to be. nothing moves in group B, and the tab stays wherever i dropped it.

error in the console when this happens:

GroupItem.add error; tabview assert: shouldn't already be in another groupItem([object MouseEvent])@chrome://browser/content/tabview.js:3351
([object MouseEvent])@chrome://browser/content/tabview.js:1640
([object MouseEvent])@chrome://browser/content/tabview.js:675
@:0

Comment 1

[Gordon P. Hemsley [:GPHemsley]]

If this is the same bug I'm experiencing, it appears to only happen when Panorama is busy loading thumbnail previews. Once all the previews are loaded, there doesn't appear to be a problem.

Though I changed the bug summary, I hesitate to add the part about previews until it is confirmed that these are indeed the same bug.

Dietrich, can you confirm that it is only a problem while tab previews are loading?

Comment 2

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 3

[Kevin Hanes]

bugspam (removing b9)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I experience this bug quite often. This happens even when previews are not loading - on average in 1 of 10 tries. The trick is to quickly drag a tab and drop it so that almost no mouseevents are fired. So this might as well happen even more when firefox or the whole computer is under load.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Pushed to try.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 504196 [details] [diff] [review]
patch v1

>+function showTabView(callback) {
>+function hideTabView(callback) {

Please add an optional win argument, probably after callback, so that we can specify different windows as well. See some of the other head.js functions for examples.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1b (small fixes in head.js)

(In reply to comment #6)
> Please add an optional win argument, probably after callback, so that we can
> specify different windows as well. See some of the other head.js functions for
> examples.

Good catch! Added.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 504210 [details] [diff] [review]
patch v1b (small fixes in head.js)

>+  if (TabView.isVisible()) {

>+  TabView.show();

>+  if (!TabView.isVisible()) {

>+  TabView.hide();

These need to be win.TabView, else they'll just be accessing the main window's TabView.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1c (small fixes in head.js)

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1d (small fixes in head.js)

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Passed try.

Comment 12

[Ian Gilman [:iangilman]]

Comment on attachment 504307 [details] [diff] [review]
patch v1d (small fixes in head.js)

Looking good. 

>+    is(targetGroup.getChildren().length, 2, 'target group has two tabs');

Go ahead and also check that sourceGroup has been closed.

R+ with that

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Approval please.

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Will this fix bug 608405?

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 17

[Kevin Hanes]

bugspam. Moving b10 to b11

Comment 18

[Kevin Hanes]

bugspam. Removing b10

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/979d2ebe8b7e

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> http://hg.mozilla.org/mozilla-central/rev/979d2ebe8b7e

Don't think this will make it into beta 10.

Comment 21

[Tracy Walker [:tracy]]

verified with nightly build of minefield.