[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

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: phlsa, Assigned: mconley)

Tracking

(Blocks: 1 bug, {regression})

28 Branch
Firefox 31
x86
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-qa-testsuite ?
in-testsuite -

Firefox Tracking Flags

(firefox29 fixed, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P3])

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 8335615 [details]
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

4 years ago
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 :-)
Flags: needinfo?(philipp)
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)
Flags: needinfo?(philipp)
Blocks: 939862
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.
Whiteboard: [Australis:P1]

Comment 4

4 years ago
Per comment #3.
Summary: [Australis] Switching tab groups will not update the tab overflow correctly → [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
Assignee: nobody → mdeboer
Looks like the 'underflow' event isn't fired when switching to that groups with few tabs.
Status: NEW → ASSIGNED
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.
Flags: needinfo?(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.
Flags: needinfo?(dao)
(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.
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.
@Jared: I used mozregression and as you predicted, the error appeared with the Australis merge.
(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!
Given that this seems to be cosmetic (tabs still work?), and Panorama generally has low-usage, P2 is a better fit here.
Whiteboard: [Australis:P1] → [Australis:P2]
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.
Whiteboard: [Australis:P2] → [Australis:P2][maybe-fix-on-aurora]
Whiteboard: [Australis:P2][maybe-fix-on-aurora] → [Australis:P2][can-fix-on-aurora]
Created attachment 8362954 [details] [diff] [review]
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.
Attachment #8362954 - Flags: feedback?(mstange)
(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 …")
(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.
Attachment #8362954 - Flags: feedback?(mstange)
(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

4 years ago
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

4 years ago
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? :-)
Keywords: regression

Comment 20

4 years ago
Created attachment 8381404 [details] [diff] [review]
strawman backout

So backing out *both* of these changes fixes things...
Dao, can you look at this (and, y'know, ideally fix it :) since Mike is out this week?
Assignee: mdeboer → dao

Updated

3 years ago
Blocks: 978599
Dao, any updates here?
Flags: needinfo?(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.
Flags: needinfo?(dao)

Comment 24

3 years ago
(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.
Unless Dao has any suggestions (or perhaps a patch in his queue! :D), I'm going to try to drive this over the line.
Assignee: dao → 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.
Flags: needinfo?(c.ascheberg)
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.
Whiteboard: [Australis:P2][can-fix-on-aurora] → [Australis:P3][can-fix-on-aurora]
Whiteboard: [Australis:P3][can-fix-on-aurora] → [Australis:P3]
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

3 years ago
(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.
Flags: needinfo?(c.ascheberg)
(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.
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.
Today I'm going to try to determine where this race is occurring. I'll keep y'all posted.
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.
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

3 years ago
(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?
(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.
Created attachment 8393221 [details] [diff] [review]
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.
Attachment #8362954 - Attachment is obsolete: true
Attachment #8381404 - Attachment is obsolete: true
Attachment #8393221 - Flags: review?(gijskruitbosch+bugs)

Comment 38

3 years ago
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.
Attachment #8393221 - Flags: review?(gijskruitbosch+bugs) → review+
Try push:

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

Comment 40

3 years ago
(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

3 years ago
(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. :-\
(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?
Flags: needinfo?(avihpit)
(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).
Flags: needinfo?(avihpit)
Filed bug 985575.

remote:   https://hg.mozilla.org/integration/fx-team/rev/742b62803af3
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/742b62803af3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → fixed
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.
Attachment #8393221 - Flags: approval-mozilla-beta?
Attachment #8393221 - Flags: approval-mozilla-aurora?
Attachment #8393221 - Flags: approval-mozilla-beta?
Attachment #8393221 - Flags: approval-mozilla-beta+
Attachment #8393221 - Flags: approval-mozilla-aurora?
Attachment #8393221 - Flags: approval-mozilla-aurora+

Updated

3 years ago
Duplicate of this bug: 986831
https://hg.mozilla.org/releases/mozilla-aurora/rev/45a9123cb498

https://hg.mozilla.org/releases/mozilla-beta/rev/7daf666421fe
status-firefox29: affected → fixed
status-firefox30: affected → fixed
Does this code need tests?
Flags: in-testsuite?
Keywords: verifyme

Comment 50

3 years ago
(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?
Flags: needinfo?(mconley)
Not worth it, especially since #TabsToolbar::after is gone (bug 1018582).

Updated

3 years ago
Flags: needinfo?(mconley)
Flags: in-testsuite?
Flags: in-testsuite-
Flags: in-qa-testsuite?
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.
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified
status-firefox31: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.