Closed Bug 226791 Opened 21 years ago Closed 14 years ago

fix dynamic theme switching feature

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
Future

People

(Reporter: asa, Unassigned)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5)

Attachments

(4 files, 3 obsolete files)

patch to nsCSSLoader for chrome: URIs
1.68 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
restore ReResolveMenusAndTrees
12.31 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
screenshot of the remaining problem
3.76 KB, image/png
Details
list of potentially problematic theme entry points (from aviary branch tree)
28.64 KB, text/plain; charset=UTF-8
Details 
We need to remove the dynamic theme switching feature and just require a restart. 
This means removing the "use this theme" checkbox on the theme install
confirmation dialog and we should decide whether we want to explain the required
restart in the pref panel or continue to throw an alert.
QA Contact: bugzilla
This feature shouldn't removed, but merely turned off by default until its bugs
are fixed.
I personally don't think that its worth the effort to make it work.  Its not 
necessary at this point, however if someone wants to try investing the effort 
to get this to work, a new bug should be opened to implement it properly.
Bug 134260 is the dynamic theme switching meta bug.
Adding "disable" to summary.
Summary: remove dynamic theme switching feature → disable/remove dynamic theme switching feature
*** Bug 233694 has been marked as a duplicate of this bug. ***
*** Bug 231503 has been marked as a duplicate of this bug. ***
*** Bug 233696 has been marked as a duplicate of this bug. ***
No longer blocks: 224777
Attached image Screenshot (obsolete) — 
Example screenshot showing how it looks when you switch theme, not sure if it
was this bad before.
Flags: blocking1.0?
*** Bug 247601 has been marked as a duplicate of this bug. ***
One way or another, we need to fix this for 1.0.
Flags: blocking1.0? → blocking1.0+
I suggest removing it. It creates too much confusion for users when installing a
new theme and then clicking "Use this theme" doubles all of their buttons and
address/search bars, which is what's been happening for many users of
FirefoxModern. The effort to make this work can be spent after 1.0; there's
other more important bugs to be fixed, and this one has no real gain in making
the dynamic switching work.
resummarizing. Now that we have enough themes of good quality - this function
should "just work".
Priority: -- → P2
Summary: disable/remove dynamic theme switching feature → fix dynamic theme switching feature
Target Milestone: --- → Firefox1.0beta
*** Bug 248091 has been marked as a duplicate of this bug. ***
Depends on: 245327
Depends on: 241614
No longer depends on: 241614
*** Bug 245327 has been marked as a duplicate of this bug. ***
*** Bug 248426 has been marked as a duplicate of this bug. ***
Should the component in this report be changed to "Extension/Theme Manager" so
people searching for an existing report are more likely to find it?  This bug
report  was orginally reported long before the new theme manager was implemented
but today users see the problem in the Theme Manager.
(In reply to comment #15)
> Should the component in this report be changed to "Extension/Theme Manager" so
> people searching for an existing report are more likely to find it?  This bug
> report  was orginally reported long before the new theme manager was implemented
> but today users see the problem in the Theme Manager.

I agree. Bug 245327 was filed against the Extension/Theme Manager component.
And all the dupes that Bug 245327 where also filed in that component. Updating
it. Let me know if this is incorrect.
Component: General → Extension/Theme Manager
I suggest increasing the priority of this bug. It's one of the most visible
Firefox issues for end-users that I can remember since 0.1. A lot of
inexperienced end-users have mentioned the bug to me.
I know there are a lot of dependencies with regards to screen/graphic
refreshing, but theme switching in Firefox was one of the nicest features over
moz/ie.  Being naive to the complexity of the code involved with theme
switching, I really have to say that the restarting option is such a cop-out and
would really hope that this can be addressed properly somehow.  Don't want to
start a flame, so please direct any personal attact to me at
mreyes@mrtech[REMOVE].com

Opera having a ton of themes and excellent theme switching, I would love to see
the day that Firefox could handle things the same way.
*** Bug 248778 has been marked as a duplicate of this bug. ***
*** Bug 248747 has been marked as a duplicate of this bug. ***
Attached image duplicate toolbar image (bug) (obsolete) — 
slightly different from the bug exhibited by the other, which was attached.
*** Bug 248950 has been marked as a duplicate of this bug. ***
*** Bug 249010 has been marked as a duplicate of this bug. ***
*** Bug 249137 has been marked as a duplicate of this bug. ***
*** Bug 249312 has been marked as a duplicate of this bug. ***
-> dbaron for some investigation, locate themes by doing Tools->Themes, "Get New
Themes" 

I suggest trying with the same set of themes.
Assignee: bugs → dbaron
Flags: blocking-aviary1.0RC1+
No longer depends on: 245327

*** This bug has been marked as a duplicate of 134260 ***
No longer blocks: 184151, 214777, 231503
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → DUPLICATE
Sometimes we hit this code for loading chrome stylesheets, and when we do, it
breaks the URIs for nsXULPrototypeCache's flush-on-Observe code.
Attachment #152981 - Flags: superreview?(bzbarsky)
Attachment #152981 - Flags: review?(bzbarsky)
Reopening because there's a patch now. Blocks bug 134260.
Blocks: 134260
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment on attachment 152981 [details] [diff] [review]
patch to nsCSSLoader for chrome: URIs

r+sr=bzbarsky.
Attachment #152981 - Flags: superreview?(bzbarsky)
Attachment #152981 - Flags: superreview+
Attachment #152981 - Flags: review?(bzbarsky)
Attachment #152981 - Flags: review+
Comment on attachment 152981 [details] [diff] [review]
patch to nsCSSLoader for chrome: URIs

Checked in to trunk, 2004-07-14 15:21 -0700.
We need to restore the function ReResolveMenusAndTrees that was removed in bug
196557 (but called from somewhere else).
This fixes the menu problem I've been noticing.
Attachment #153535 - Flags: superreview?(bzbarsky)
Attachment #153535 - Flags: review?(bzbarsky)
Comment on attachment 153535 [details] [diff] [review]
restore ReResolveMenusAndTrees

r+sr=bzbarsky... Looks reasonable.
Attachment #153535 - Flags: superreview?(bzbarsky)
Attachment #153535 - Flags: superreview+
Attachment #153535 - Flags: review?(bzbarsky)
Attachment #153535 - Flags: review+
Comment on attachment 153535 [details] [diff] [review]
restore ReResolveMenusAndTrees

Checked in to trunk, 2004-07-17 18:16 -0700.
Comment on attachment 153535 [details] [diff] [review]
restore ReResolveMenusAndTrees

I just added a null-check of rootFrame to this patch, r+sr=bryner via IRC.  It
was causing problems on beast tinderbox.
Attached image screenshot (obsolete) — 
That's a nice improvement, especially with bug 226791 fixed. I have one issue
though: The toolbar button icons look distorted after a theme switch, they're
like a mixture of two themes, see the screenshot. Using the default and the
Charamel theme (http://members.shaw.ca/lucx/). But if I open the customize
toolbar dialog (right click->Customize), the icons get fixed. I don't see this
with these themes in a build made before today's changes.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040722
Firefox/0.9.1+ (Steffen)
I know about that.  It's the reason I haven't actually marked this bug as fixed.
*** Bug 253822 has been marked as a duplicate of this bug. ***
FYI, bug 252703 is fixed now.

What problems remain with dynamic theme switching?  I've noticed some minor
glitches in the theme switching dialog itself.
In today's branch build (20040731), switching themes between default and
charamel doesn't update the icons correctly so I end up missing the reload,
stop, and home buttons. Other than that, things looks pretty solid.
should this be marked fixed-aviary1.0?
I have no way of knowing whether it's fixed, since there is no clear description
of what problem it describes.
This shows the remaining problem Asa mentioned in comment 43.
Attachment #150705 - Attachment is obsolete: true
Attachment #154039 - Attachment is obsolete: true
Attachment #151816 - Attachment is obsolete: true
Depends on: 222575
*** Bug 255671 has been marked as a duplicate of this bug. ***
decision made to turn off dynamic theme switching in an effort to focus on
higher priority and more visable areas.  dbaron is still looking at issues and
is likely to make another run at providing this in a future release.
Flags: blocking-aviary1.0PR-
Flags: blocking-aviary1.0PR+
Flags: blocking-aviary1.0-
Flags: blocking-aviary1.0+
As a theme maintainer, I use this feature a lot. It works for me. It has worked
for quite some time now, and I sometimes whipsaw the theme manager fairly
vigorously.

DON'T disable this, please!


I started a discussion on this topic at Mozillazine at
http://forums.mozillazine.org/viewtopic.php?t=119547

The impression I get is that the problems that remain with dynamic
theme-switching is with the themes, not with Firefox.
(In reply to comment #43)
> In today's branch build (20040731), switching themes between default and
> charamel doesn't update the icons correctly so I end up missing the reload,
> stop, and home buttons. Other than that, things looks pretty solid.

This is fixed by attachment 157269 [details] [diff] [review] on bug 252703 (see bug 252703 comment 18),
although that's probably too risky for the FF 1.0 branch at this point.
So, yesterday morning I suddenly realized what the theme switching
problem that I've spent a few days last week debugging was.  So now I
think I understand the basic reasons why theme switching is broken.
That said, I haven't actually fixed the second one, so I'm not sure that
there aren't going to be other problems after these are fixed.

The two basic problems are:

 1. URI Equality:  Lots of code assumes that if two URIs are equal, they
    refer to the same resource.  The normal way of saying "don't make
    this assumption anymore" is to reload the page -- but we don't want
    to do that, since it destroys lots of UI state that we want to keep
    when switching themes.  It's also a reasonable assumption -- the
    only thing that violates it that we care about is theme switching,
    and we don't want to slow down pretty much everything else to make
    theme switching work.

 2. Entry points into themes:  The current theme switching design
    assumes that the only entry points to themes -- i.e., the only
    places where content that's not in a "skin" package links to content
    in a "skin" package, are stylesheet links, either from
    xml-stylesheet processing instructions or from stylesheet elements
    in XBL.

    I've attempted to autogenerate a list of the places these rules are
    violated.  My list at this point is quite noisy, since it includes
    places where people are using "skin" packages when things probably
    belong in "content" and are not modified by themes, such as some
    stuff related to form control and help stylesheets.  It also
    includes all the places that help content (in XHTML) includes skin
    images for demonstration -- but I think we can live with these not
    changing until the help content is reloaded.

    My full list of potentially problematic entry points will be attached
    to this bug.  The most prominent problems for Aviary look like
    extensions.css linking to the richview.xml#richview binding (which
    causes problems in the Themes dialog itself when switching -- this
    is the one I was debugging) and various bits of JS code linking to
    xpinstallItemGeneric.png and themeGeneric.png (although I'm not sure
    if these cause any noticeable problems).

A lot of the code that currently does theme switching involves clearing
caches, such as chrome images from the image cache and skin XBL from the
XUL cache.  It might appear that this code is designed to address
problem (2), but it's really only designed to address (1), i.e., to
prevent the resource that used to be equivalent to the URI but no longer
is from being retrieved out of the cache.

So how do I propose fixing these problems?

1. URI Equality:  The path of least resistance is probably to use the
   patch I've been working on in bug 252703.  This makes a new nsIURI
   (etc.) implementation for chrome: URIs that basically works the same
   as current ones do, except it stores the converted form of the URI
   (generally a jar: URI) and causes nsIURI::Equals to return false when
   compared with an equivalent chrome: URL that has a different
   converted URI (i.e., because it was first converted when the current
   theme was different).  I'm not entirely happy with this approach, but
   it does seem to do what's necessary and I can't think of any better
   ideas.

2. Entry points:  I think the best solution here is to make the rules
   clearer and fix the themes.  There aren't that many problems -- and
   they don't necessarily all need to be fixed at once, although fixing
   them does require incrementing the skinVersion since it requires
   all themes to add the additional style rules that create the extra
   level of indirection that we need.

   The rule in question is:

     The only ways something not in a theme should link to something in
     a theme are through the xml-stylesheet processing instruction in a
     XUL file or the stylesheet element (inside a resources element) in
     an XBL file.  Everything that you might want to do some other way
     requires an extra layer of indirection through a style rule.

I'm interested in thoughts on whether this makes sense, and also
thoughts on when this should happen (although it seems like it's
probably out for 1.0 at this point).
>      The only ways something not in a theme should link to something in
>      a theme are through the xml-stylesheet processing instruction in a
>      XUL file or the stylesheet element (inside a resources element) in
>      an XBL file.  Everything that you might want to do some other way
>      requires an extra layer of indirection through a style rule.

Does this exclude @import rules in style sheets? I think that would be a very
bad precedent to set, especially for the (future moz2.0) case where we want XUL
style to be automatically applied, instead of having to manually import
chrome://global/skin/.

It sure looks to me that you're going to need specialty logic for scrollbars.css
in any case, since it gets imported everywhere (and rather magically at that).
(In reply to comment #54)
> >      The only ways something not in a theme should link to something in
> >      a theme are through the xml-stylesheet processing instruction in a
> >      XUL file or the stylesheet element (inside a resources element) in
> >      an XBL file.  Everything that you might want to do some other way
> >      requires an extra layer of indirection through a style rule.
> 
> Does this exclude @import rules in style sheets? I think that would be a very
> bad precedent to set, especially for the (future moz2.0) case where we want XUL
> style to be automatically applied, instead of having to manually import
> chrome://global/skin/.

Yes, but I don't see how it's a precedent.  They don't work now.  If people need
them in themes, we can see how much work it is to make it work, but if people
don't, then why bother?
> It sure looks to me that you're going to need specialty logic for scrollbars.css
> in any case, since it gets imported everywhere (and rather magically at that).

I'm not proposing removing of the special logic that we already have for this. 
This currently works.  But adding any new entry points to themes requires adding
code to make the dynamic switching work correctly.
How about making dynamic theme switching optional, disabled by default of
course.  For all of us who code or use it on a day to day basis?

Also recommunicating the proper entry points and validating update.mozilla.org
themes to make sure they don't violate these rules.  Or just adding some
validation to the current code base to chuck out any rogue/outdated themes.

I think that would be a great start, no?
(In reply to comment #56)
> How about making dynamic theme switching optional, disabled by default of
> course.  For all of us who code or use it on a day to day basis?

That's what's being done here:
http://bugzilla.mozilla.org/show_bug.cgi?id=257304

I believe it's, specically, this part:
>>pref("extensions.dss.enabled", false);          // Dynamic Skin Switching    
                                          
>>pref("extensions.dss.switchPending", false);    // Non-dynamic switch pending
after next
>>                                                // restart.
>
>Turn off dynamic skin switching.

So you just need to change that pref/prefs for 1.0PR ;).

-[Unknown]
Dynamic theme switching works on the RC PR10 (Mozilla/5.0 (Windows; U; Windows
NT 5.1; rv:1.7.3) Gecko/20040911 Firefox/0.10) but the page you're reading goes
blank :/
Sincerly sorry for the spam, I can't reproduce the dynamic theme switching. Sorry.
*** Bug 259886 has been marked as a duplicate of this bug. ***
Having no dynamic theme switching or instantaneous extension installations is
outdated. Firefox isn't some software from 1970.
(In reply to comment #61)
> Having no dynamic theme switching or instantaneous extension installations is
> outdated. Firefox isn't some software from 1970.

I'd really have to say that's a bit harsh; I don't think there were any products
that HAD extensions in 1970.

I do agree that it'd be nice to have this back on, and indeed the extensions.  I
think this is probably a goal the developers mean to work toward (at least
dynamic theme switching..) but it's a rather complicated issue, and as such will
take time to get fixed.

I'm with the developers on leaving it off, because I agree that the most
paramount issue here - above theme switching, above extensions even existing
maybe - is making the Firefox 1.0 release stable.  If they release 1.0, and put
all this marketting work into it, and all that... and it is buggy, and they
release a perfect 1.0.1 even.... it will have all been lost.

The number of users who will be lost by bugs far outnumber those who will be
lost by having to restart Firefox to apply a theme.

But, all that said, I am greatly anticipating this feature being done, and many
of the other features I hope for getting completed/added/fixed/etc.  But not for
1.0... all I want for 1.0 is stability.

Anyhow, unless I'm wrong, you can instantly and magically upgrade your
retro-70's browser to a new age one by setting this pref:

extensions.dss.enabled

To true.  Talk about time travel, eh?  Firefox's developers already mastered it.

I'm afraid I have not tested that this works personally, although I have been
told it does, because I am currenty still using 0.9 pending the time to upgrade
some customizations I have made to my browser.

-[Unknown]
Can a setting for dynamic theme switching/extension installs be at least added 
then somewhere in the UI for the moment?
not a chance.  We do NOT expose broken features to end users.
I have never come across this broken feature. Certainly there should be some way
of enabling it.
(In reply to comment #65)

As comment 62 says:
"Anyhow, unless I'm wrong, you can instantly and magically upgrade your
retro-70's browser to a new age one by setting this pref:

extensions.dss.enabled

To true."

This enables theme switching for 1.0PR, but it's buggy.
What I find interesting is that until it was turned off, this feature appeared
to be working flawlessly in my Windows XP setup. 

Then it was turned off and I turned it back on with the user_pref set in
user.js. DSS has now regressed back to long ago.
*** Bug 272289 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.0PR-
Flags: blocking-aviary1.0-
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Flags: blocking-aviary2.0?
*** Bug 252952 has been marked as a duplicate of this bug. ***
QA Contact: bugzilla → extension.manager
*** Bug 316246 has been marked as a duplicate of this bug. ***
This involves substantial Gecko changes that will not happen in the Fx2 scope.
Flags: blocking1.9a1?
Flags: blocking-aviary2?
Flags: blocking-aviary2-
*** Bug 317030 has been marked as a duplicate of this bug. ***
FWIW, I'm seeing "broken dynamic theme switching" in:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051118 Firefox/1.5 ID:2005111803

In most cases the toolbar buttons fail to change aspect, or lose their icons and/or text. In severe cases i've seen the whole display get wrong, including the display of the Themes window switching to all Times New Roman black-on-white with no graphic items whatsoever, and HTML links in the main window becoming unclikable.

See also bug 317030
Note that this was fixed by the patch in bug 252703 which was backed out for performance reasons.
Target Milestone: Firefox1.0beta → ---
Flags: blocking1.9a1? → blocking1.9-
Whiteboard: [wanted-1.9]
mconnor:  So how badly to the front-end folks want this?  I suppose I could try relanding it to see how bad the regression is...
I used to miss this feature, but i think we don't need it anymore. Ever since Firefox 2.0 began to allow a restart to the same windows and tabs we have been able to accomplish the equivalent of the dynamic theme switching.

The only thing we really need can be found in MR Tech Local install: when a theme is changed, it asks if you want to restart the browser. That works fine in lieu of dynamic theme switching.
(In reply to comment #76)
> Firefox 2.0 began to allow a restart to the same windows and tabs we have been
> able to accomplish the equivalent of the dynamic theme switching.

That's not an equivalent. The more (heavy) tabs you have, the more annoying it gets. Of course you usually don't do this every day. But still, fixing dynamic theme switching is a nice-to-have.
It's a minor nice-to-have, its not a WONTFIX, but not something I'd want dbaron to spend significant time on...
Flags: wanted-firefox3+
Whiteboard: [wanted-1.9]
Product: Firefox → Toolkit
Putting this on my radar for some investigation on how much effort this might need
Priority: P2 → --
Target Milestone: --- → mozilla1.9.2
Target Milestone: mozilla1.9.2 → mozilla1.9.3
Assignee: dbaron → nobody
Target Milestone: mozilla1.9.3 → Future
Status: REOPENED → NEW
Version: unspecified → Trunk
As an author of Mozilla themes for 10 years, I would really like to have dynamic theme switching enabled and working. Especially now that LightWeight themes (or Personas) do have this feature. Not having dynamic theme switching for normal themes is just 'unfair'. 

My experience with extensions.dss.enabled is that dynamic switching generally works, except for the reloading of some of the images, which should be not to difficult.
We aren't going to get into making decisions based on some sense of "fairness."  Down that path lies madness.  If we got this for free, or minimal effort, great, but from a product perspective I don't think it's in our top 50 platform asks.

Some stuff works even now, but the rest, in order to work properly, requires changes that caused across-the-board regressions.  The amount of work to fix that, if it can be fixed, is unlikely to be trivial.  I'd be happy to be wrong on this, but it seems like wishful thinking that we'll ever put cycles into fixing this over the things that dbaron and others could do instead.
And yet, it IS continuously used as an excuse by yourself and others for why Personas are superior to Full Themes, that's almost funny.
Let's take a more positive route. Dynamic Theme Switching would be very nice, and something that we could try to implement. 

The current state is that the only issue remaining is the flushing of images.
When that is fixed a whole series of bugs can be closed, which is also good to achieve. Actually the flushing of images work good enough, but the current window keeps the 'old' images. Opening a new (browser) window, and closing the old fixes the image issue. Btw songbird restarts now the app, but all the windows to get the theme switch applied.
I've written a small extension that allows to change themes without a restart. It needs some work, but I guess it works fine:
Get it from AMO:
https://addons.mozilla.org/en-US/firefox/addon/61769

or direct installation:
www.silvermel.net/xpi/switchthemes_0.0.1.xpi
Depends on: 563241
per mconnor's comment #82, and the fact that my original bug report here was so dramatically warped I'm wontfixing this (my) bug report. If you'd like to continue this fight, please file your own bug and don't cc me.
Status: NEW → RESOLVED
Closed: 20 years ago → 14 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: