597980 - Switching into and out of Panorama mode quickly can cause the wrong tab to be selected on return

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Rain [:rain1] (inactive)]

Windows 7, Minefield 4.0b7pre, September 19.

STR:
1. Open two tabs (let's call them A and B).
2. Focus tab A normally.
3. Enter Panorama mode (Ctrl-Space) and exit it normally. Panorama should believe (indicated by the focus ring) that the focused tab is tab A.
4. Switch to tab B.
5. Press Ctrl-Space twice in succession, entering and exiting before the animation is complete). You'll see tab A on return, not tab B.

What seems to be happening is that Panorama's only updating the tab it believes to be focused after the animation is complete.

Comment 1

[Aza Raskin [:aza]]

Confirmed that this is happening as of the latest m-c on the 12th of Oct. Something we should certainly fix, but not as important as speed or data-loss bugs.

Comment 2

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 3

[Kevin Hanes]

bugspam (removing b9)

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Sean, I'd be happy to take this off your plate.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Trivial patch

Writing a test which is 100% deterministic for this could be a tad tricky... I'd be most inclined for writing a test based on the STR for bug 622870, i.e. clicking on a tab to zoom in on it and immediately (5ms after) checking that that tab and its parent are already set as active.

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 510157 [details] [diff] [review]
Trivial patch

>   setActiveOrphanTab: function GroupItems_setActiveOrphanTab(tabItem) {
>+    this.setActiveOrphanTab(null);
>     this._activeOrphanTab = tabItem;
>   },

I imagine you don't intend this infinite loop. Perhaps the new line is supposed to go in setActiveGroupItem?

The test you propose sounds decent.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #7)
> Comment on attachment 510157 [details] [diff] [review]
> Trivial patch
> 
> >   setActiveOrphanTab: function GroupItems_setActiveOrphanTab(tabItem) {
> >+    this.setActiveOrphanTab(null);
> >     this._activeOrphanTab = tabItem;
> >   },
> 
> I imagine you don't intend this infinite loop. Perhaps the new line is supposed
> to go in setActiveGroupItem?

Sorry, that was supposed to be this.setActiveGroupItem(null). This does not trigger an infinite loop, though, as setActiveGroupItem doesn't call setActiveOrphanTab if setActiveGroupItem's argument is null... Adding the corresponding check to setActiveOrphanTab now.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2, with test

Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d4ba679baaab

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Passed try.

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 510510 [details] [diff] [review]
Patch v2, with test

+ Gavin for quick review while Ian is out.

Comment 13

[Ian Gilman [:iangilman]]

Comment on attachment 510510 [details] [diff] [review]
Patch v2, with test

>+    is(contentWindow.GroupItems.getActiveGroupItem(), originalGroup,
>+       "The new group is active");
>+    is(contentWindow.UI.getActiveTab(), originalTab._tabViewTabItem,
>+       "The new tab is active");

These should be newGroup and newTab, right?

r+ with that fixed.

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #13)
> Comment on attachment 510510 [details] [diff] [review]
> Patch v2, with test
> 
> >+    is(contentWindow.GroupItems.getActiveGroupItem(), originalGroup,
> >+       "The new group is active");
> >+    is(contentWindow.UI.getActiveTab(), originalTab._tabViewTabItem,
> >+       "The new tab is active");
> 
> These should be newGroup and newTab, right?

You're right... the test was wrong, but it was passing... trying to figure out why now.

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2.1

Figured out why the test was failing... I was misusing sendMouseEvent and middle-clicking on it, not left-clicking on it, so it wasn't actually zooming in successfully. Fixed and pushed to try.

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=51b50784d414

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

Passed try cleanly.

Note to approvers: small footprint, fixes a ux glitch, lays groundwork for fixing bug 631934, and passed try with flying colors.

Comment 18

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 19

[(no longer active)]

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

Comment 20

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre

Verified issue and it's no longer reproducible.

Comment 21

[Jesse Ruderman]

I still see this bug or something like it. Filed bug 638273.

Comment 22

[Kevin Hanes]

Bug 638273 claims this is still an issue, anyone else getting this still?

Comment 24

[Jithin Emmanuel]

I can reproduce this issue. Mostly it is triggered when I click 2 tabs next to each other and then select tab view multiple times.
Link to a screen recording showing the issue with latest nightly. http://www.mediafire.com/?en28cyahc572b6e

Comment 25

[Jithin Emmanuel]

To reproduce this bug one needs to switch tabs by clicking on the tab bar, selecting a different tab in panaroma mode is not triggering this. 
Can this bug be reopened ?

Comment 26

[Raymond Lee [:raymondlee]]

(In reply to comment #25)
> To reproduce this bug one needs to switch tabs by clicking on the tab bar,
> selecting a different tab in panaroma mode is not triggering this. 
> Can this bug be reopened ?

It's hard to reproduce but you will hit this

Steps
1) Select tab A in the tabbar 
2) Go into panorama using keyboard shortcut (ctrl/cmd+shift+e)
3) Exit panorama using keyboard shortcut (ctrl/cmd+shift+e)
4) Select tab B in the tabbar
5) Press keyboard shortcut (ctrl/cmd+shift+e) twice in one go

Actual:
You see the zoom out and zoom in animations, and the tab A is selected

Expected:
You see the zoom out and zoom in animations, and the tab B should still be selected

Comment 27

[Raymond Lee [:raymondlee]]

Attachment: Update the test to reproduce the issue

mitcho: I've updated the test to reproduce the issue.  Hope this helps.

Comment 28

[Raymond Lee [:raymondlee]]

mitcho:  if you are not working on this, I can take this one.

Comment 29

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #28)
> mitcho:  if you are not working on this, I can take this one.

Raymond, thanks, that would be great.

Comment 30

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 31

[Ian Gilman [:iangilman]]

Comment on attachment 518004 [details] [diff] [review]
v3

Looks good.

Comment 32

[Raymond Lee [:raymondlee]]

Comment on attachment 518004 [details] [diff] [review]
v3

Very small footprint.  Just added one line of code.

Comment 33

[Raymond Lee [:raymondlee]]

Comment on attachment 518004 [details] [diff] [review]
v3

Passed try.

Comment 34

[Raymond Lee [:raymondlee]]

(In reply to comment #33)
> Comment on attachment 518004 [details] [diff] [review]
> v3
> 
> Passed try.
http://tbpl.mozilla.org/?tree=MozillaTry&rev=31ca6a525b0f

Comment 35

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

Comment on attachment 518004 [details] [diff] [review]
v3

No longer accepting patches for desktop Firefox.

Comment 36

[Johnathan Nightingale [:johnath]]

Comment on attachment 511539 [details] [diff] [review]
Patch v2.1

Clearing obsolete a+

Comment 37

[Jithin Emmanuel]

Will this fix be included in RC2

Comment 38

[Kevin Hanes]

Comment on attachment 518004 [details] [diff] [review]
v3

Re-upping for approval now that the tree is open.

Comment 39

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/72321f30825a

Comment 40

[Ian Gilman [:iangilman]]

Comment on attachment 518004 [details] [diff] [review]
v3

I think we can land without approval. Also, it's already on cedar now.

Comment 41

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/72321f30825a

Comment 42

[(no longer active)]

FWIW, no approval is required to land stuff on trunk any more.

Comment 43

[George Carstoiu]

Verified issue with Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre