Closed Bug 629869 Opened 9 years ago Closed 9 years ago

Upgrading Firefox while using third-party theme gives unthemed scrollbars on first run after upgrade

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0

People

(Reporter: mcdavis941.bugs, Assigned: mossop)

References

Details

(Whiteboard: [addons-testday])

Attachments

(3 files, 1 obsolete file)

This is a follow-up to Bug 621475, which addressed the same issue for other element types.

Unlike that bug, this one is only about scrollbars.  Other element types seem to be handled correctly.

Issue: the first time you launch Firefox after upgrading from 4.0b10pre to 4.0b11pre, if you're using a third-party theme, you see most browser chrome styled by the theme, but theme styling doesn't apply to the scrollbars.  See the screenshot.

STR:
1 - Make sure you have both Fx4.0b10pre and Fx4.0b11pre installed
2 - Start Fx4.0b10pre with a fresh profile
3 - Go to about:config, set extensions.checkCompatibility.4.0b=false (may or
may not be needed based on whatever addons you're testing with)
4 - Install a theme (not a persona), select that theme, restart with it
4.1 - https://addons.mozilla.org/en-US/firefox/addon/14313/versions/ (along
with several others) will show this bug
5 - Notice everything looking relatively normal with the theme
6 - Exit Firefox
7 - Start Fx4.0b11pre (a version jump from b10pre to b11pre, representing an app
upgrade) with the same profile used in step 2
8 - Notice the checks for updates to installed addons (because it's a browser
upgrade) after which the app finishes launching normally
9 - Look at the main browser UI and notice scrollbars not using
the theme's style.

Expected Result:
- Theme applies to UI as usual.
Actual Result:
- Theme is only partially applied, resulting in a mix of themed and unthemed
elements in browser chrome.
- This error only shows up the first time Firefox is launched after an upgrade.
 After that, on every subsequent launch, it works normally and as expected.

- Vista 64
- Mozilla/5.0 (Windows NT 6.0; WOW64; rv:2.0b10pre) Gecko/20110125
Firefox/4.0b10pre
- Mozilla/5.0 (Windows NT 6.0; WOW64; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre
Attached image screenshot
The behavior is the same (unthemed scrollbars on first run) after the upgrade from b11pre to the current b12pre.
It's still happening on the upgrade from b12pre to b13pre.
Confirmed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b13pre) Gecko/20110226 Firefox/4.0b13pre ID:20110226030401

Dave or Blair, anything we can do for Firefox 4? Especially for users with dark themes this is kinda odd behavior in the primary ui after an upgrade.
OS: Windows Vista → All
Hardware: x86_64 → All
Attached patch patch rev 1 (obsolete) — Splinter Review
A couple of additional things need flushing after we've loaded any UI. The skin cache must be flushed to ensure the scrollbar stylesheet is reloaded (http://mxr.mozilla.org/mozilla-central/source/layout/style/nsLayoutStylesheetCache.cpp#64) and the string bundle service drops any loaded strings (http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/nsStringBundle.cpp#604). It's possible that only chrome-flush-caches is needed but there is at least one place where chrome-flush-skin-caches does something additional (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8159) so I'd rather flush them both to be on the safe side here.

Not a lot we can automatically test here, I could write an automated test to verify that the notifications are being sent but not sure there is a lot of value in that right now. As is this is simple enough that we ought to be able to land it before RC.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #515956 - Flags: review?(robert.bugzilla)
Whiteboard: [has patch][needs review rs]
Comment on attachment 515956 [details] [diff] [review]
patch rev 1

It looks like flushCaches won't always be true when appChanged is true. Will this only happen when flushCaches is set to true? I'm specifically worried about the cases below where a) the theme could be global and b) PREF_EM_SHOW_MISMATCH_UI returns false.

    if (aAppChanged && !this.allAppGlobal) {
      // Should we show a UI or just pass the list via a pref?
      if (Prefs.getBoolPref(PREF_EM_SHOW_MISMATCH_UI, true)) {
        this.showMismatchWindow();
        flushCaches = true;
      }
      else if (this.startupChanges.appDisabled.length > 0) {
        // Remember the list of add-ons that were disabled this startup so
        // the application can notify the user however it wants to
        Services.prefs.setCharPref(PREF_EM_DISABLED_ADDONS_LIST,
                                   this.startupChanges.appDisabled.join(","));
      }
    }
Comment on attachment 515956 [details] [diff] [review]
patch rev 1

Talked with Mossop on irc and he answered the question.

Please add a comment regarding this is needed because the early ui causes us to cache of parts of the default theme which then need to be flushed. Might be a good thing to only notify when the current theme isn't the default theme as well.
Attachment #515956 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][needs review rs] → [has patch]
ignore the default theme suggestions... please add to the comment regarding strings
Added the comments. This is an extremely low risk patch that fixes this display issue with custom themes. I think it is very safe to take for Firefox 4.
Attachment #515956 - Attachment is obsolete: true
Attachment #516040 - Flags: review+
Attachment #516040 - Flags: approval2.0?
Whiteboard: [has patch] → [has patch][needs approval]
Attachment #516040 - Flags: approval2.0? → approval2.0+
Landed, not sure if this is worth a litmus test, what do you think henrik?

http://hg.mozilla.org/mozilla-central/rev/a72d0f50429b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [has patch][needs approval]
Target Milestone: --- → mozilla2.0
We can bundle that test with a major update test very easily. So I would keep it.
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0
Status: RESOLVED → VERIFIED
Whiteboard: [addons-testday]
You need to log in before you can comment on or make changes to this bug.