941309 - [Australis] Switching tab groups will not update the tab overflow correctly, and leaving only pinned tabs can create weird overlap between tabs and the navbar

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Tab strip

When switching from a tab group with many tabs to one with few tabs, the overflow indicators remain in the UI. This also means that the new tab button isn't where it is supposed to be (see attachment)

Comment 1

[:Gijs (he/him)]

Can you check if this is reproducible on release/beta/aurora/nightly-before-australis? If it's not, can you mark as blocking australis-merge? Thanks :-)

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

It doesn't happen in release and aurora. It does happen in UX and post-australis nightly. (not sure how I can get a non-australis nightly)

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: NavBar Shift

Related to this; if you have the persistent overflow arrow and app tabs then close all your normal tabs the navbar shifts up.

Comment 4

[:Gijs (he/him)]

Per comment #3.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Looks like the 'underflow' event isn't fired when switching to that groups with few tabs.

Comment 6

[Mike de Boer [:mikedeboer]]

In Australis we're now drawing tabs in the titlebar, which was previously not the case. So, I started flipping the 'browser.tabs.drawInTitlebar' pref (the pref) to 'false', which made this issue not reproducible.
When I comment out the lines at
http://hg.mozilla.org/mozilla-central/file/b2b20bc6576a/browser/components/tabview/ui.js#l473 and http://hg.mozilla.org/mozilla-central/file/b2b20bc6576a/browser/components/tabview/ui.js#l549 and set the pref back to 'true', this is issue is not reproducible.

These lines were introduced in bug 572160, so Dão, I'd like to ask you if removing TabsInTitlebar code from Panorama sounds like a good idea to you? I tested this in fullscreen mode too, both with the pref set to both values, which did not yield any surprising results/ glitches.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> In Australis we're now drawing tabs in the titlebar, which was previously
> not the case.

It was the case in maximized windows on Windows. Did those suffer from this bug before Australis?

> These lines were introduced in bug 572160, so Dão, I'd like to ask you if
> removing TabsInTitlebar code from Panorama sounds like a good idea to you?

Panorama doesn't display the tab bar, so that doesn't sound right to me.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Philipp Sackl [:phlsa] from comment #2)
> It doesn't happen in release and aurora. It does happen in UX and
> post-australis nightly. (not sure how I can get a non-australis nightly)

You can use mozregression (http://mozilla.github.io/mozregression) to track down what day this bug started occurring.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Of course, you have a high chance that it will point to the day that UX got merged to mozilla-central. But you can use this to confirm that, and it is a good tool to use in the future for other bugs you find.

Comment 10

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

@Jared: I used mozregression and as you predicted, the error appeared with the Australis merge.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Philipp Sackl [:phlsa] from comment #10)
> @Jared: I used mozregression and as you predicted, the error appeared with
> the Australis merge.

Cool, good to know. Thanks!

Comment 12

[Justin Dolske [:Dolske]]

Given that this seems to be cosmetic (tabs still work?), and Panorama generally has low-usage, P2 is a better fit here.

Comment 13

[Mike de Boer [:mikedeboer]]

Update: this is not reproduceable on Windows, not on Australis AND non-Australis builds. So this is only occurring on OSX with Australis & drawing in the titlebar.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: work-around.

Markus, using the fastest `setTimeout` possible I can work around this issue.
However, the result is that the OSX titlebar jumps into view briefly when switching between tab groups.
It appears to be that the flow that draws in the titlebar on OSX requires a bit of time, or an additional reflow after it's been obscured from view, like Panorama/ TabView/ Tab Groups does.

Could you spend a few cycles on why this might be the case? When you apply this patch, you should be able to see it rather well.

Comment 15

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] from comment #11)
> (In reply to Philipp Sackl [:phlsa] from comment #10)
> > @Jared: I used mozregression and as you predicted, the error appeared with
> > the Australis merge.
> 
> Cool, good to know. Thanks!

mozregression works on the UX branch too btw ("mozregression -r ux …")

Comment 16

[Markus Stange [:mstange]]

(In reply to Mike de Boer [:mikedeboer] from comment #14)
> Markus, using the fastest `setTimeout` possible I can work around this issue.

Do you know why this fixes it?

> However, the result is that the OSX titlebar jumps into view briefly when
> switching between tab groups.

The titlebar is also visible while Panorama is shown, but during that time its background is made invisible using the activetitlebarcolor attribute (which also gets rid of the separator line below it) in http://dxr.mozilla.org/mozilla-central/source/browser/components/tabview/ui.js#576

So what you see here is a timing difference between setting the "chromemargin" and the "activetitlebarcolor" attributes. Now there is a brief window of time where the titlebar is visible and the activetitlebarcolor attribute is not set - during that time you see a titlebar with regular styling.

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #16)
> Do you know why this fixes it?

No clue... I was hoping you might!

Comment 18

[Christian Ascheberg]

I can reproduce this bug on Win7 with private windows (which reminded me of bug 962803, which also only happens with private windows).

Comment 19

[:Gijs (he/him)]

UX regression window: http://hg.mozilla.org/projects/ux/pushloghtml?fromchange=18fd395752cd&tochange=aa03fbc1149f

I'm guessing this was caused by either of the two tabsintitlebar gradient bugs. This also explains comment #18, because on Windows the privatebrowsing indicator is implemented as a #TabsToolbar::after pseudoelement (see also bug 963512).

Which makes this related to 853415. I don't recall what exactly happened there and there are a lot of comments and it's late here. But maybe this can help in figuring out what is actually *causing* the issue, and fixing that instead of working around it? :-)

Comment 20

[:Gijs (he/him)]

Attachment: strawman backout

So backing out *both* of these changes fixes things...

Comment 21

[Justin Dolske [:Dolske]]

Dao, can you look at this (and, y'know, ideally fix it :) since Mike is out this week?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Dao, any updates here?

Comment 23

[Dão Gottwald [:dao]]

I don't know what exactly is going on here, but I have a plan how to work around it. I'll try to put together a patch tomorrow or on Friday. However, without a Mac I'm flying blind and if I get stuck, it might make sense that somebody else picks this up.

Comment 24

[:Gijs (he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #3)
> Created attachment 8336115 [details]
> NavBar Shift
> 
> Related to this; if you have the persistent overflow arrow and app tabs then
> close all your normal tabs the navbar shifts up.

This should be fixed now that we fixed the general case of only having app tab bugs in a small window (don't have the bug number handy)

(In reply to Christian Ascheberg from comment #18)
> I can reproduce this bug on Win7 with private windows (which reminded me of
> bug 962803, which also only happens with private windows).

I suspect this got fixed with other fixes to overflow and private browsing windows on windows, but I could be wrong.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Unless Dao has any suggestions (or perhaps a patch in his queue! :D), I'm going to try to drive this over the line.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

So I've had multiple people confirm that the glitchiest part of this bug, the overlap thing illustrated in attachment 8336115 [details], is no longer reproducible. I myself can not reproduce this on Aurora on OS X.

That's a good thing.

I was unable to reproduce the the overflow bug on Windows 7 with private browsing windows, so needinfo'ing Christian Ascheberg to see if he can provide STR with latest Nightly or Aurora.

I'm not 100% convinced this is still a P2 with the major glitch no longer reproducible.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Unless anybody can reproduce the glitch in attachment 8336115 [details] on Nightly or Aurora, I think I'm gonna go ahead and downgrade the priority of this to a P3.

Comment 28

[Justin Dolske [:Dolske]]

P3 now, since it appears to be OSX+Panorama only. While the overflow indicators sticking around is mostly harmless, the new tab button moving does tempt me to P3+ it.

Comment 29

[Christian Ascheberg]

(In reply to Mike Conley (:mconley) from comment #26)
> I was unable to reproduce the the overflow bug on Windows 7 with private
> browsing windows

Same for me with latest Nightly, seems to be fixed.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tim Taubert [:ttaubert] (away March 17th-21st) from comment #5)
> Looks like the 'underflow' event isn't fired when switching to that groups
> with few tabs.

Confirmed with the debugger. That's exactly what's happening.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

I had a few minutes to walk through here, and I think what we're hitting is some kind of race. When I step through the hiding of the tabview and the updating of the tabstrip, we underflow just fine.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Today I'm going to try to determine where this race is occurring. I'll keep y'all posted.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

It seems to be a race involving the tabsintitlebar attribute applied to the main window in TabsInTitlebar._update. If I break immediately after that, and then continue, the bug goes away. Breaking just before setting that attribute and continuing causes the bug to show.

I'll be looking at the the changeset range that Gijs pointed to in comment 19 now and how this affects the tabsintitlebar attribute.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

So it looks like we apply those pseudoelements if tabsintitlebar is enabled, and remove them in tabviewmode (since tabsintitlebar is disabled in that mode). Switching back, and re-adding those pseudoelements seems to be the kicker. Something about that is eating up the underflow event. I think I'm going to have to enlist some layout help.

Comment 35

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #34)
> So it looks like we apply those pseudoelements if tabsintitlebar is enabled,
> and remove them in tabviewmode (since tabsintitlebar is disabled in that
> mode). Switching back, and re-adding those pseudoelements seems to be the
> kicker. Something about that is eating up the underflow event. I think I'm
> going to have to enlist some layout help.

Can we keep the pseudoelements around while panorama is on and do something else to not have them affect stuff, as a workaround?

Comment 36

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #35)
> (In reply to Mike Conley (:mconley) from comment #34)
> > So it looks like we apply those pseudoelements if tabsintitlebar is enabled,
> > and remove them in tabviewmode (since tabsintitlebar is disabled in that
> > mode). Switching back, and re-adding those pseudoelements seems to be the
> > kicker. Something about that is eating up the underflow event. I think I'm
> > going to have to enlist some layout help.
> 
> Can we keep the pseudoelements around while panorama is on and do something
> else to not have them affect stuff, as a workaround?

Maybe. We could have tabview/ui.js set some attribute on the main-window when we're in Panorama mode, and key off of those. It's hacky, but it might work.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

So here's what I'm proposing - instead of adding / removing the pseudoelements, we hide or show them depending on whether or not we need them. That seems to sidestep the issue.

I'm going this route because when I tried to keep them around when Panorama was available, I realized I had to somehow get Panorama to realize that even though tabsintitlebar is disabled (because it is disabled when in tabview mode), it'd be going _back_ to a state where tabsintitlebar would be enabled.

Sounded pretty hairy, and I went with Occam's Razor.

My only concern is that permanently introducing these elements might result in more layout running, meaning talos number bumps. I'm going to run this through try and see what we get.

Comment 38

[:Gijs (he/him)]

Comment on attachment 8393221 [details] [diff] [review]
Patch v2

Review of attachment 8393221 [details] [diff] [review]:
-----------------------------------------------------------------

A somewhat reluctant r=me assuming try doesn't show tpaint/ts_paint going through the roof.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Try push:

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=47605bbcee03
Patch: https://tbpl.mozilla.org/?tree=Try&rev=251c24104eb7

Comment 40

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #39)
> Try push:
> 
> Baseline: https://tbpl.mozilla.org/?tree=Try&rev=47605bbcee03
> Patch: https://tbpl.mozilla.org/?tree=Try&rev=251c24104eb7

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=47605bbcee03&newRev=251c24104eb7&submit=true

This actually looks like a tpaint /improvement/, if anything. ts_paint is just weird, TART seems slightly worse but it might not be significant? Done some more retriggers...

Comment 41

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #40)
> (In reply to Mike Conley (:mconley) from comment #39)
> > Try push:
> > 
> > Baseline: https://tbpl.mozilla.org/?tree=Try&rev=47605bbcee03
> > Patch: https://tbpl.mozilla.org/?tree=Try&rev=251c24104eb7
> 
> http://perf.snarkfest.net/compare-talos/index.
> html?oldRevs=47605bbcee03&newRev=251c24104eb7&submit=true
> 
> This actually looks like a tpaint /improvement/, if anything. ts_paint is
> just weird, TART seems slightly worse but it might not be significant? Done
> some more retriggers...

With more retriggers, tpaint, ts_paint and TART all look OK to me. CART, however. :-\

Comment 42

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #41)
> (In reply to :Gijs Kruitbosch from comment #40)
> > (In reply to Mike Conley (:mconley) from comment #39)
> > > Try push:
> > > 
> > > Baseline: https://tbpl.mozilla.org/?tree=Try&rev=47605bbcee03
> > > Patch: https://tbpl.mozilla.org/?tree=Try&rev=251c24104eb7
> > 
> > http://perf.snarkfest.net/compare-talos/index.
> > html?oldRevs=47605bbcee03&newRev=251c24104eb7&submit=true
> > 
> > This actually looks like a tpaint /improvement/, if anything. ts_paint is
> > just weird, TART seems slightly worse but it might not be significant? Done
> > some more retriggers...
> 
> With more retriggers, tpaint, ts_paint and TART all look OK to me. CART,
> however. :-\

Yeah... very strange. We could remove those pseudoelements when the customizing attribute is set, and that might sidestep the regression - though I'm not sure it's worth it.

Avi, what do you think? Do you think this regression is worrying enough that we should find a way around it?

Comment 43

[Avi Halachmi (:avih)]

(In reply to Mike Conley (:mconley) from comment #42)
> Avi, what do you think? Do you think this regression is worrying enough that
> we should find a way around it?

I'm fine with not looking into this. Seems very minor, and possibly within the noise level, and only for osx 10.6.

However, Mike will file a bug to assess overall 10% CART osx 10.6 regressions during march (see also bug 978844 comment 8).

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Filed bug 985575.

remote:   https://hg.mozilla.org/integration/fx-team/rev/742b62803af3

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/742b62803af3

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8393221 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis, and tabs in titlebar on OS X.


User impact if declined: 

If a user has an overflowed tabstrip and then switches into Panorama, if they switch to a tab group that has few tabs (which should underflow), the tab scrollbox goes away. This wouldn't be so bad, except that it moves the new tab button to the far right of the screen, which is not where the user would expect.


Testing completed (on m-c, etc.): 

Plenty of local testing and a day to bake on m-c.


Risk to taking this patch (and alternatives if risky): 

Pretty low risk. This might result in a slight CART bump for OS X 10.6 only, but :avih cleared that.


String or IDL/UUID changes made by this patch:

None.

Comment 48

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/45a9123cb498

https://hg.mozilla.org/releases/mozilla-beta/rev/7daf666421fe

Comment 49

[u279076]

Does this code need tests?

Comment 50

[:Gijs (he/him)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #49)
> Does this code need tests?

Not sure how I feel. I suspect it's enough of an edgecase that it might not be worth writing a regression test for. Mike?

Comment 51

[Dão Gottwald [:dao]]

Not worth it, especially since #TabsToolbar::after is gone (bug 1018582).

Comment 52

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:31.0) Gecko/20100101 Firefox/31.0

Verified issue as fixed on Firefox 30, build ID: 20140605174243 and Firefox 31 beta 5, build ID: 20140626181429.