Closed Bug 1438504 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Theme, defect, P1)

60 Branch
x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified

People

(Reporter: alice0775, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached image screenshot
No description provided.
I can't seem to reproduce this. Brian, can you check please?
Flags: needinfo?(bgrinstead)
Keywords: steps-wanted
Priority: -- → P1
STR
1. launch Nightly
2. Select Light theme
3. Open new window(Ctrl+N) if not reproduced
Summary: Tab bar is broken(scroll arrow button and "List all Tabs" button are shown even if tab bar is not overflowed) → Tab bar is broken in Light theme (scroll arrow button and "List all Tabs" button are shown even if tab bar is not overflowed)
Okay, I can reproduce with the Light theme selected (or any non-default theme really).
Keywords: steps-wanted
Summary: Tab bar is broken in Light theme (scroll arrow button and "List all Tabs" button are shown even if tab bar is not overflowed) → Tab bar overflows when it shouldn't after applying a lightweight theme
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.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
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("firefox-compact-light@mozilla.org");
      let win = await BrowserTestUtils.openNewBrowserWindow();
      ok(!win.gBrowser.tabContainer.hasAttribute("overflowing"), "not overflowing");
      await BrowserTestUtils.closeWindow(win);
    });
Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

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

Clearing review as per Comment 7
Attachment #8951307 - Flags: review?(bgrinstead)
Comment on attachment 8951307 [details]
Bug 1438504 - Ignore bogus overflow events.

https://reviewboard.mozilla.org/r/220556/#review226602
Attachment #8951307 - Flags: review?(bgrinstead) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cb84f4afeb8
Ignore bogus overflow events. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/8cb84f4afeb8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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
QA Whiteboard: [testday-20180216]
Thanks for verifying.
Status: RESOLVED → VERIFIED
It's still reproducible for me on Windows 7 32-bit (20180301223350)
Attached image overflow.png
Blocks: 1442960
You need to log in before you can comment on or make changes to this bug.