Closed Bug 233607 Opened 21 years ago Closed 20 years ago

About Dialog Should Not be Themeable (Skin Change Causes Loss of Image on About Dialog & about:)

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox0.9

People

(Reporter: zach, Assigned: robert)

References

Details

(Keywords: polish)

Attachments

(1 file, 7 obsolete files)

Patch moving the about dialog stuff to browser.jar (also fixes Bug 233606) v2 (Updated to Tip)
5.58 KB, patch
Details | Diff | Splinter Review 
User-Agent:       
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8

it shows the text, but is broken and looks wierd, doesnt have the image either.
I wasn't useing the standard theme, but when i switched to the regular theme, i
could.

Reproducible: Sometimes
Steps to Reproduce:
1.open firefox
2.switch to different theme
3.go to about mozilla firefox

Actual Results:  
It did the problem

Expected Results:  
The correct about box, should have popped up
Which theme?  This was (perhaps still is) themable, so it might have been the
theme's change.
Hi Zach,

What else is "broken and looks weird" in the about: window, apart from the image?

It sounds like a duplicate of the bug just before it: bug 233606, except that
bug only mentions a broken image, and says the info is fine.

Because the contents and the graphical design of the about Firefox dialog
(which is nice), is split over /browser/content and /browser/skin,
it is partly themeable, which can cause problems if the theme doesn't behave.
May be that is just the responsibility of the themer, who want to be able
to at least position the buttons and such according to the theme.
The 'about' content itself should be untouched by the theme.

So, at least the 'about.png' should be in /browser/content, and also most
of the formatting (version, agent, copyright, credits, etc).
Only a very brief /browser/skin/about.css should be left for themers, to do 
something extra if they really want.

The current setup is much to inviting to change the about contents.

P.s. how about adding an extra 'page' for the themers? 
Something like 'about:theme'? (from Tools/Options/Themes, and from
Help/About/Themes, and from about:theme?)
Summary: when i go to Help> About Mozilla Firefox, it displays a broken about dialog → The Help> About Mozilla Firefox is partly themeable:make it either fully or not themeable
-> Taking
Severity: normal → minor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: polish
OS: Linux → All
Hardware: PC → All
-> Really taking it this time
Assignee: firefox → rparenton
Status: ASSIGNED → NEW
Blocks: 233606
To be in line with the Mozilla Foundation branding policy, the about dialog
really shouldn't be modifiable by skins.  A forthcoming patch will reflect this
change.

Also, this will allow us to save about 80K on duplicate and unnecessary images.
Status: NEW → ASSIGNED
Also, it should be noted that the Credits dialog (on the About dialog) is not
themeable in Firefox 0.8 (not present on the trunk yet/ever as of yesterday), so
it makes sense that the About dialog should not be either, especially from a
branding perspective.

Also, to clarify my space saving remark in the previous comment, we currently
have three identical copies of about.png in the tree:

browser/base/content/about.png
browser/base/skin/about.png
browser/base/skin/mac/about.png

This change would allow the use of only browser/base/content/about.png and the
elimination of the other two.
Summary: The Help> About Mozilla Firefox is partly themeable:make it either fully or not themeable → About Dialog Should Not be Themeable (Skin Change Causes Loss of Image on About Dialog & about:)
Removing accidentally set Target Milestone
Removal of the following files goes with this patch:

browser/base/skin/about.png
browser/base/skin/mac/about.png
browser/base/content/aboutDialog.css
browser/base/content/mac/aboutDialog.css
This file needs to be added in addition to the patch above.
This file needs to accompany the above patch.
Attachment #141497 - Attachment is patch: false
Attached file browser/base/content/mac/jar.mn (obsolete) — 
This file needs to be added in addition running the above patch.
Comment on attachment 141496 [details] [diff] [review]
Patch moving the remaining elements of the about dialog from classic.jar to browser.jar

Requesting patch review from Blake Ross.

Please note comments 9-12
Attachment #141496 - Flags: review?(firefox)
This seems like the right thing to do.

Bart, from a branding perspective, keeping control over this seems like a good
plan, and letting people theme it doesn't add any value to the user.  I wouldn't
mind seeing your thoughts on this.
Target Milestone: --- → Firefox0.9
sounds good to me.
Comment on attachment 141496 [details] [diff] [review]
Patch moving the remaining elements of the about dialog from classic.jar to browser.jar

Removing review request and making obsolete.  New patch coming soon.
Attachment #141496 - Attachment is obsolete: true
Attachment #141496 - Flags: review?(firefox)
Attachment #141497 - Attachment is obsolete: true
Attachment #141498 - Attachment is obsolete: true
Attachment #141499 - Attachment is obsolete: true
QA Contact: mconnor
After applying this patch, browser/base/skin/aboutDialog.css must be moved to
browser/base/content/.	This is to preserve the history in CVS (mconnors
suggestion).  

Also, after applying the patch, it is possible to remove the following files
from the tree:
browser/base/skin/about.png
browser/base/skin/mac/about.png
browser/base/skin/mac/aboutDialog.css

Thanks to bsmedberg for the preprocessor help.
Comment on attachment 141862 [details]
Patch moving the about dialog stuff to browser.jar (also fixes Bug 233606)

-> Obsolete - Indenting was off on a line or two.
Attachment #141862 - Attachment is obsolete: true
Attachment #141862 - Attachment is patch: false
After applying this patch, browser/base/skin/aboutDialog.css must be moved to
browser/base/content/.	This is to preserve the history in CVS (mconnors
suggestion).  

Also, after applying the patch, it is possible to remove the following files
from the tree:
browser/base/skin/about.png
browser/base/skin/mac/about.png
browser/base/skin/mac/aboutDialog.css

Thanks to bsmedberg for the preprocessor help.
Comment on attachment 141863 [details] [diff] [review]
Patch moving the about dialog stuff to browser.jar (also fixes Bug 233606)

Blake, can you take a look at this?  Please see my notes in Comment #19 for
some additional info about the patch.

bsmedberg suggested I do the CSS using the preprocessor.

Also, Bart agrees with this from the branding perspective, which is the basis
behind this bug.
Attachment #141863 - Flags: review?(firefox)
Attachment #141863 - Flags: review?(firefox) → review+
Comment on attachment 141863 [details] [diff] [review]
Patch moving the about dialog stuff to browser.jar (also fixes Bug 233606)

Blake, don't check this version in.  It currently fails on the preprocessor
(most likely due to leading space issues).
Attachment #141863 - Attachment is obsolete: true
After applying this patch, browser/base/skin/aboutDialog.css must be MOVED to
browser/base/content/.	This is to preserve the history in CVS (mconnors
suggestion).  

Also, after applying the patch, it is possible to remove the following files
from the tree:
browser/base/skin/about.png
browser/base/skin/mac/about.png
browser/base/skin/mac/aboutDialog.css

Thanks to bsmedberg for the preprocessor help.
Comment on attachment 142002 [details] [diff] [review]
Patch moving the about dialog stuff to browser.jar (also fixes Bug 233606) v2

Blake, can you take a look at this?

Please see my notes in Comment #22 for
some additional info about the patch.

Also, every line in aboutDialog.css was moved to the right one space per
bsmedberg's instructions to prevent the preprocessor from choking on the #id
CSS tags.  This is the only difference from the previous patch you marked
review+.
Attachment #142002 - Flags: review?(firefox)
Updated the proposed patch to tip.  Notes in Comment #22 still apply
Attachment #142002 - Attachment is obsolete: true
Comment on attachment 142002 [details] [diff] [review]
Patch moving the about dialog stuff to browser.jar (also fixes Bug 233606) v2

Removing review request on obsolete patch.
Attachment #142002 - Flags: review?(firefox)
Comment on attachment 143056 [details] [diff] [review]
Patch moving the about dialog stuff to browser.jar (also fixes Bug 233606) v2 (Updated to Tip)

Blake, can you take a look at this?

Please see my notes in Comment #22 for some additional info about the patch.

Also, the change to aboutDialog.css in the patch is explained in Comment #23.
Attachment #143056 - Flags: review?(firefox)
Can you please mark the old patches as obsolete?
alanjstr, everything except the current patch is marked as obsolete when I look
at it.
Sorry.  CSS must have been cached wrong.
Robert, what is the actual differences (except that it doesn't break on the PP?)

If its essentially the same patch just slightly modified so the PP doesn't hork
on it, it doesn't really need another review.
Mike, the only difference is that it is updated to tip (there was a checkin to
browser/base/jar.mn after the original was created) and everything in
aboutDialog.css has been indented one space so the PP won't choke on the #id
labels in the CSS.

Other than that, there are no differences.
(In reply to comment #31)
> everything in aboutDialog.css has been indented one space so the PP won't
> choke on the #id labels in the CSS.

If you're just indenting to baby the PP, why not prepend the element name
instead of indenting everything?  For example, for this:
   <label id="copyright" value="&copyrightText;"/>
use this (without spaces in front):
   label#copyright { /* ... */ }

instead of indenting by a space?  It's more specific, and it might even speed up
layout with the more specific styling identification (as the style info is only
parsed for <label>s).  (If convention is against this, forgive my ignorance.)
Attachment #143056 - Flags: review?(firefox)
checked in just now.  My CVS-fu is weak like little girl
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
After changing to another theme, I can still see the about content and credit
content. However, the web site that I currently browse is gone and the web
address in the address bar is gone, too. It becomes "blank page". Here is the
configuration on my computer:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
Comment #34, please test with a current build.  However, your problem is an
issue with your theme, or dynamic theme switching (if it is enabled).  It has
nothing to do with this bug.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: