Tabs not in Title-bar after bug 1269462

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Jim Jeffery not reading bug-mail 1/2/11, Assigned: Gijs)

Tracking

({regression})

49 Branch
Firefox 49
regression
Points:
---

Firefox Tracking Flags

(firefox48 fixed, firefox49+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

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
Created attachment 8752756 [details]
titlebar.jpg

See attachment for image of issue
(Assignee)

Comment 3

2 years ago
(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?
Flags: needinfo?(jmjeffery)
(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.
Flags: needinfo?(jmjeffery)
(Assignee)

Comment 5

2 years ago
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...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 6

2 years ago
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.
Flags: needinfo?(enndeakin)
Flags: needinfo?(dholbert)

Comment 7

2 years ago
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?
Flags: needinfo?(enndeakin)
(Assignee)

Comment 8

2 years ago
Created attachment 8752828 [details] [diff] [review]
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...
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1273179
(Assignee)

Comment 10

2 years ago
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.
Flags: needinfo?(enndeakin)

Updated

2 years ago
Duplicate of this bug: 1273287
Tracking for 49.
tracking-firefox49: --- → +

Comment 13

2 years ago
When I examine #TabsToolbar at that point in browser-tabsintitlebar.js it has collapsed="true"
Flags: needinfo?(enndeakin)
(Assignee)

Comment 14

2 years ago
(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!)
Flags: needinfo?(dholbert) → needinfo?(jaws)
(Assignee)

Comment 15

2 years ago
(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.
(Assignee)

Comment 16

2 years ago
Created attachment 8753820 [details]
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/
Attachment #8753820 - Flags: review?(mdeboer)
(Assignee)

Updated

2 years ago
Flags: needinfo?(jaws)
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 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...
Attachment #8753820 - Flags: review?(mdeboer) → review+

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5a4cdb6dfb19
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(Assignee)

Comment 21

2 years ago
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.
Attachment #8753820 - Flags: approval-mozilla-aurora?
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/
(Assignee)

Comment 23

2 years ago
(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.
Flags: needinfo?(jgilbert)
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
Attachment #8753820 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox48: unaffected → fixed
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.