1438504 - Tab bar overflows when it shouldn't after applying a lightweight theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Alice0775 White]

Attachment: screenshot

Comment 1

[Dão Gottwald [:dao]]

I can't seem to reproduce this. Brian, can you check please?

Comment 2

[Alice0775 White]

STR
1. launch Nightly
2. Select Light theme
3. Open new window(Ctrl+N) if not reproduced

Comment 3

[Dão Gottwald [:dao]]

Okay, I can reproduce with the Light theme selected (or any non-default theme really).

Comment 4

[Brian Grinstead [:bgrins]]

I'm guessing this has to do with the timing of when LightweightThemeConsumer gets constructed. Prior to Bug 1434401 it was running very early, in a <contructor> in a XBL binding attached to :root. Now it runs in the onLoad event: https://dxr.mozilla.org/mozilla-central/rev/27fd083ed7ee5782e52a5eaf0286c5ffa8b35a9e/browser/base/content/browser.js#1317.

Comment 5

[Dão Gottwald [:dao]]

Attachment: Bug 1438504 - Ignore bogus overflow events.

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

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220556/diff/1-2/

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

https://reviewboard.mozilla.org/r/220556/#review226574

After discussing on IRC, it sounds like we always get an unnecessary overflow event with or without LWT, but without LWT (or also with LWT prior to Bug 1434401) we would also get a corresponding underflow event. For whatever reason, moving the LWTConsumer out of the XBL constructor and into onLoad is causing us to not get an underflow event. I don't understand why this is, but it seems reasonable to skip the unnecessary events in all cases.

Just one request before landing: can you add a small regression test for this, especially since we don't fully understand exactly why/when these unnecessary events are firing? I think something like this would work (just double check that the assertion is on the right element - I don't have a build with this failing locally to confirm):

    const {LightweightThemeManager} = ChromeUtils.import("resource://gre/modules/LightweightThemeManager.jsm", {});
    registerCleanupFunction(() => {
      LightweightThemeManager.currentTheme = null;
      Services.prefs.clearUserPref("lightweightThemes.usedThemes");
    });

    add_task(async function withoutLWT() {
      let win = await BrowserTestUtils.openNewBrowserWindow();
      ok(!win.gBrowser.tabContainer.hasAttribute("overflowing"), "not overflowing");
      await BrowserTestUtils.closeWindow(win);
    });

    add_task(async function withLWT() {
      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme("[e-mail]");
      let win = await BrowserTestUtils.openNewBrowserWindow();
      ok(!win.gBrowser.tabContainer.hasAttribute("overflowing"), "not overflowing");
      await BrowserTestUtils.closeWindow(win);
    });

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220556/diff/2-3/

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

https://reviewboard.mozilla.org/r/220556/#review226580

Clearing review as per Comment 7

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220556/diff/3-4/

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

https://reviewboard.mozilla.org/r/220556/#review226602

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220556/diff/4-5/

Comment 13

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1b383ab9d2f24e9159aa1f51272d4bd11a7ed4b

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cb84f4afeb8
Ignore bogus overflow events. r=bgrins

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8cb84f4afeb8

Comment 16

[Sayed [:LiquidO2]]

I can reproduce this issue in Nightly 60.0a1 (2018-02-15) (64-bit) in 64 bit Linux

I can verify that this is fixed in latest Nightly 60.0a1

Build ID 	20180217100053
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0

Comment 17

[Dão Gottwald [:dao]]

Thanks for verifying.

Comment 18

[John]

It's still reproducible for me on Windows 7 32-bit (20180301223350)

Comment 19

[John]

Attachment: overflow.png