1273094 - Tabs not in Title-bar after bug 1269462

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Testing with m-c hourly builds. on Win10 x64 32 bit builds.

Last good:
https://hg.mozilla.org/mozilla-central/rev/403912ca555eb65f814b18ecf38ad8e8e98569f5
Bad build: 
https://hg.mozilla.org/mozilla-central/rev/d0be57e84807ce0853b2406de7ff6abb195ac898

Mozregress found https://bugzilla.mozilla.org/show_bug.cgi?id=1269462 as the regressor

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Other items I forgot to include:
Clean Profile: yes
Maximized window 
Resize does not change, tabs still not in title-bar
Same with e10s on/off
Single Monitor

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

Attachment: titlebar.jpg

See attachment for image of issue

Comment 3

[:Gijs (he/him)]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #1)
> Other items I forgot to include:
> Clean Profile: yes
> Maximized window 
> Resize does not change, tabs still not in title-bar

I just reproduced the original problem, but if I restore and re-maximize (or, if I started with a restored window, I maximize and re-restore), the problem goes away. Does it not for you?

Comment 4

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #1)
> > Other items I forgot to include:
> > Clean Profile: yes
> > Maximized window 
> > Resize does not change, tabs still not in title-bar
> 
> I just reproduced the original problem, but if I restore and re-maximize
> (or, if I started with a restored window, I maximize and re-restore), the
> problem goes away. Does it not for you?

yes, indeed the issue goes away if I toggle minimize/re-maximize.

Comment 5

[:Gijs (he/him)]

So the reason this isn't working on Windows 10 but it works on earlier versions of Windows (which is why I didn't catch it originally), is because the tablet mode detection code (which is windows-10-only), runs and always calls TabsInTitlebar.updateAppearance.

That's unnecessary in this case, and it wouldn't be a bad thing on its own, but the second bug is that when TabsInTitlebar.init is called from the browser's onLoad method, the tabbar still has 0 height. I don't know why that is - in my understanding, when the load event fires, the initial layout should have happened and it shouldn't be the case that the tabstrip has no height... so that's puzzling. When the constructor of the tabstrip XBL binding fires, this is also still the case (ie the tabstrip still has 0 height).

I'm effectively looking for a way of determining when it's "safe" to run TabsInTitlebar.update, and then I'll gate both the current check for _initialized on that, and ensure that we run the update when it's set...

Comment 6

[:Gijs (he/him)]

Dan or Neil, can you help? Steps to reproduce on Windows versions earlier than Windows 10 are:

copy this line:

https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/browser/base/content/browser.js#5093
that is,

    TabsInTitlebar.updateAppearance(true);

into the init() method a few lines up:

https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/browser/base/content/browser.js#5070-5075

outside the if() block.

("./mach build faster" will suffice to rebuild)

Any diagnostic JS code you want to add can go in browser-tabsintitlebar.js:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabsintitlebar.js#145-147

AIUI the element's height should be defined at this stage because "load" has fired - and if I read e.g. a background-color style set in tabs.inc.css from the #TabsToolbar, it works, so CSS has clearly been loaded and applied, but for some reason the tabstrip doesn't have its usual height, which makes no sense to me...

A few dozens of milliseconds later, when the "activate" event triggers ToolbarColor.inferFromText, this line:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7734
that is,

      let [r, g, b] = parseRGB(getComputedStyle(toolbar).color);

apparently triggers the "resize" event listener in tabbrowser.xml:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5368-5372

which I can only assume is due to a style/layout flush, after which the tabstrip height *is* defined (this is the path into browser-tabsintitlebar.js we take on non-win10 / without the adjustment I suggested at the top.

Comment 7

[Neil Deakin]

I don't see this on Windows 7. Can you provide a patch so I understand what it is I am supposed to be changing and seeing?

Comment 8

[:Gijs (he/him)]

Attachment: Patch to reproduce on Windows 8

This reproduces the issue for me on Windows 8. When opening Firefox with a window that used to be maximized (so the window is restored maximized) and tabs in the titlebar, there is suddenly (a lot of) space above the tabs. After restoring and re-maximizing the window, there is less of this space.

Tested with an opt build - as it's a bit timing-dependent, I wouldn't be surprised if it doesn't reproduce on a debug one...

Comment 10

[:Gijs (he/him)]

Re-needinfo'ing after comment #8 because without more of a clue of what's breaking here I don't know how to address it beyond piling on more hacks, which I really don't think we should do with this code.

Comment 12

[Marcia Knous [:marcia]]

Tracking for 49.

Comment 13

[Neil Deakin]

When I examine #TabsToolbar at that point in browser-tabsintitlebar.js it has collapsed="true"

Comment 14

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #13)
> When I examine #TabsToolbar at that point in browser-tabsintitlebar.js it
> has collapsed="true"

D'oh. That would help explain what's going on. I don't know off-hand where that gets unset, though...

Jared, were you looking for bugs to work on still? If so, wanna steal this one? ("No" is acceptable!)

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Neil Deakin from comment #13)
> > When I examine #TabsToolbar at that point in browser-tabsintitlebar.js it
> > has collapsed="true"
> 
> D'oh. That would help explain what's going on. I don't know off-hand where
> that gets unset, though...

So the answer here is that it gets set when tabbrowser-tabs gets the same "load" event, which calls into its own binding, which (un)sets collapsed on the container (#TabsToolbar), which explains why this is all a race... it depends whether browser.js or the tabbrowser binding adds its load event handler first. We should change TabsInTitlebar to cope with this.

Jared, if you have time, feel free to take this, otherwise I'll write a patch (my) tomorrow morning.

Comment 16

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1273094 - only trigger TabsInTitlebar.init() after the tabbrowser has updated the visibility of the tabs toolbar, r?mikedeboer

Review commit: https://reviewboard.mozilla.org/r/53508/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53508/

Comment 17

[Ryan VanderMeulen [:RyanVM]]

FWIW, looks like this issue actually reproduces on Win10 on Try:
https://treeherder.mozilla.org/logviewer.html#?job_id=20978447&repo=try#L15739

I guess it's comforting that if we had Win10 running in CI, it would have caught the regression before it shipped.

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8753820 [details]
MozReview Request: Bug 1273094 - only trigger TabsInTitlebar.init() after the tabbrowser has updated the visibility of the tabs toolbar, r?mikedeboer

https://reviewboard.mozilla.org/r/53508/#review50402

::: browser/base/content/browser.js:5087
(Diff revision 1)
>    observe(subject, topic, data) {
>      this.update(data == "tablet-mode");
>    },
>  
>    update(isInTabletMode) {
> +    let wasInTabletMode =

The other consumers use `hasAttribute("tabletmode")`, which is nice and boolean to start with... perhaps good to change it here to be consistent?

::: browser/base/content/tabbrowser.xml:5367
(Diff revision 1)
>          <parameter name="aEvent"/>
>          <body><![CDATA[
>            switch (aEvent.type) {
>              case "load":
>                this.updateVisibility();
> +              TabsInTitlebar.init();

This made me look at the TabsInTitlebar#init function to see if there was a double-init guard there... and it is, but not used in the init function.
Perhaps put it there for good measure? It super unlikely to have two tabbrowser-tabs in the same window, but still...

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5a4cdb6dfb19

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a4cdb6dfb19

Comment 21

[:Gijs (he/him)]

Comment on attachment 8753820 [details]
MozReview Request: Bug 1273094 - only trigger TabsInTitlebar.init() after the tabbrowser has updated the visibility of the tabs toolbar, r?mikedeboer

Approval Request Comment
[Feature/regressing bug #]: bug 1269462
[User impact if declined]: titlebar broken on win10
[Describe test coverage new/current, TreeHerder]: nope...
[Risks and why]: reasonably low... this together with bug 1269462 (where I just requested uplift) makes our tabsintitlebar story arguably more sane than it was before. At this stage it's had enough baking that I'm fairly sure there aren't glaringly obvious issues left
[String/UUID change made/needed]: no.

Comment 22

[Edouard Oger [:eoger]]

I'm not sure if you're supposed to care or not but this line removal https://hg.mozilla.org/mozilla-central/rev/5a4cdb6dfb19#l2.12 breaks this addon https://addons.mozilla.org/en-US/firefox/addon/vertical-tabs-simplified/

Comment 23

[:Gijs (he/him)]

(In reply to Edouard Oger [:eoger] from comment #22)
> I'm not sure if you're supposed to care or not but this line removal
> https://hg.mozilla.org/mozilla-central/rev/5a4cdb6dfb19#l2.12 breaks this
> addon
> https://addons.mozilla.org/en-US/firefox/addon/vertical-tabs-simplified/

What does "breaks" mean in this sentence? I just checked and I don't see any breakage on OS X - I see the titlebar not being as narrow in height as one might wish. That seems like something that should be relatively easy for the add-on to fix.

Ultimately the interaction here is always going to be weird. It makes sense to initialize tabs-in-the-titlebar when the tabstrip has been initialized (and not before, or somewhere else) because ultimately that's what "tabs in the titlebar" is about: tabs, in the titlebar. When an add-on moves tabs out of the titlebar, the whole thing gets pretty murky. What should be in the titlebar at that stage? The tabs aren't there...

Looking at the add-on code, it seems it overrides the tabbrowser-tabs binding. At that stage, there is very little we can do on the Firefox side. I'll submit a small PR to the add-on and hopefully that will help. Pinging jgilbert because it seems to be his.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8753820 [details]
MozReview Request: Bug 1273094 - only trigger TabsInTitlebar.init() after the tabbrowser has updated the visibility of the tabs toolbar, r?mikedeboer

Needed for bug 1269462

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1fc21edf1ee5