851436 - pinned tabs sometimes scroll with normal tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Girish Sharma [:Optimizer]]

possibly a regression by bug 837486

STR

1) Have atleast one pinned tab and as many tabs to cause overflow
2) Pin the first unpinned tab on left.
3) Pine another.
4) Viola, overflow arrows jump to left of pinned tabs, and thus pinned tabs can go out of view.

Comment 1

[Frank Yan (:fryn)]

I cannot reproduce this.

Comment 2

[Girish Sharma [:Optimizer]]

I am on windows 7 latest nightly.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

I can't reproduce it with the given STR and couldn't come up with another but I swear I just saw this too (on Linux x64).

Comment 4

[Girish Sharma [:Optimizer]]

okay here are some STR that I can reproduce everytime:

1) Use 1600 X 900 resolution, everything else default firefox nightly windows 7 aero.
2) On the title bar, have the panorama and the downloads button on the extreme right.
3) Have 2 pinned tabs, and 3 normal tabs.
4) Now keep on opening new tabs by hitting Ctrl T until there are 14 non-pinned tabs (making a total of 16 tabs)
5) Scroll the overflowed tabstrip so that the first non-pinned tab is visible.
6) right click on first tab and pin it.
7) Now without changing the position of the overflowed strip, right click on the first visible tab and pin it.
8) Repeat step 7 and you will see the bug.

I will attach screencast soon.

Comment 5

[Girish Sharma [:Optimizer]]

screencast link : http://youtu.be/Mr5GKucPc3I

will be live in around 10 minutes.

Comment 6

[Alex Keybl [:akeybl]]

Thanks for the bug report! While we'd accept a low risk uplift of this issue (if found), it doesn't seem like the user impact is critical enough to track for release.

Comment 7

[Dão Gottwald [:dao]]

This is basically means that pinned tabs aren't pinned (i.e. always visible) anymore. That's significant breakage.

Comment 8

[Frank Yan (:fryn)]

Ah, I can reproduce it now, but it's only triggered by a very narrow combination of pinned tabs, normal tabs, and tab strip widths. I'll fix it.

Comment 9

[Frank Yan (:fryn)]

Given bug 821859 comment 22, I think we should just fix this by backing out the fixes for bug 837486, bug 649654, and bug 821859 in that order. I don't think it's worth all this trouble to try to keep the tab close button under the cursor in overflow mode anymore.

What are your thoughts, Dão?

Comment 10

[Frank Yan (:fryn)]

I just wrote a reply: bug 821859 comment 23.

Comment 11

[Dão Gottwald [:dao]]

Sounds good to me. If someone wants to give bug 649654 another shot later on, avoiding the problems it caused the first time, that's of course always an option.

Comment 12

[Girish Sharma [:Optimizer]]

Is this bug really difficult to solve ?

Comment 13

[Frank Yan (:fryn)]

Attachment: patch: just back out patches for bug 837486, bug 649654, and bug 821859

(In reply to Dão Gottwald [:dao] from comment #11)

Thank you for the quick reply.

> If someone wants to give bug 649654 another shot later
> on, avoiding the problems it caused the first time, that's of course always
> an option.

Absolutely. I'll copy this comment over to that bug when backing its fix out and reopening it.

(In reply to Girish Sharma [:Optimizer] from comment #12)
> Is this bug really difficult to solve ?

This comment is entirely unhelpful. If you have a quick fix for bug 649654 without using bug 821859 and without causing bug 837486 and this bug, patches are welcome.

Comment 14

[Girish Sharma [:Optimizer]]

I was asking the complexity of the bug. It was a question as I had no idea on the complexity and this bug leading to a back-out of 3 bugs seemed a big deal to me. If It felt personal in any way, then I am really sorry.

Comment 15

[Frank Yan (:fryn)]

(In reply to Girish Sharma [:Optimizer] from comment #14)

This bug alone is tricky to solve, because it touches upon the fragility of our tab strip's transition between underflow and overflow states in <xul:scrollbox/>, partially due to the tab strip's scrollable items taking a larger total width (due to the new tab button, etc.) in the underflow state. I would rather not attempt a fix that would have to be back-ported to aurora (like the previous fix for bug 837486), only to have to cause yet another regression.

As I wrote above, the problem that really led me to want to back all this out is dbaron's bug 821859 comment 22, which I didn't know was a problem until he noted it.

Comment 16

[Frank Yan (:fryn)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbba1340a23

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7fbba1340a23

Comment 18

[Frank Yan (:fryn)]

Comment on attachment 725921 [details] [diff] [review]
patch: just back out patches for bug 837486, bug 649654, and bug 821859

[Approval Request Comment]
Bug caused by: the landing of bug 837486, bug 649654, and bug 821859
User impact if declined: visual glitches in primary UI and potentially performance and memory regressions
Testing completed: landed cleanly on mozilla-central 
Risk to taking this patch: very minimal; the patch simply backs out the patches to those three bugs, reverting the tab strip overflow code to Firefox pre-21 state
String or UUID changes made by this patch: none

To clarify, this patch is a "roll-up patch" containing all three relevant and necessary backouts in one.

Comment 19

[bhavana bajaj [:bajaj]]

(In reply to Frank Yan (:fryn) from comment #18)
> Comment on attachment 725921 [details] [diff] [review]
> patch: just back out patches for bug 837486, bug 649654, and bug 821859
> 
> [Approval Request Comment]
> Bug caused by: the landing of bug 837486, bug 649654, and bug 821859
> User impact if declined: visual glitches in primary UI and potentially
> performance and memory regressions
> Testing completed: landed cleanly on mozilla-central 
> Risk to taking this patch: very minimal; the patch simply backs out the
> patches to those three bugs, reverting the tab strip overflow code to
> Firefox pre-21 state
> String or UUID changes made by this patch: none
> 
> To clarify, this patch is a "roll-up patch" containing all three relevant
> and necessary backouts in one.

Thanks for the one roll-up patch. Keeping in mind comment# 15 , backout is safe & worthwhile here . Approving the backout patch for uplift on aurora.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bfb3ef111866

Comment 21

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0

Verified as fixed using STR provided on comment 4 on Firefox 21 beta 1 (buildID: 20130401192816).

Comment 22

[Manuela Muntean [Away]]

Verified fixed with Firefox 22 beta 1 (build ID: 20130514181517) on Mac OSX 10.8.3, Ubuntu 12.10 32bit and Windows 7 64bit.