Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Help Viewer uses browser chrome

RESOLVED FIXED in mozilla1.8beta4

Status

SeaMonkey
Help Viewer
RESOLVED FIXED
13 years ago
a year ago

People

(Reporter: alanjstr, Assigned: Jeff Walden (gone starting June 8))

Tracking

unspecified
mozilla1.8beta4
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
Some of the URIs point to chrome://help/skin/ and some to chrome://browser/skin/

This is inconsistent and forces themes to skin the Help window.  The Help Window
should be self contained in terms of styles.  Unfortunately this was not noticed
until 1.0 was released, so now I have to re-skin.  Even if the build script has
to copy the images (although it appears in help.jar that it already does), these
should be chrome://help/skin.

for example:
 41 .toolbarbutton-1 {
 42         -moz-box-orient: vertical;
 43         min-width: 0px;
 44         list-style-image: url("chrome://help/skin/Toolbar.png");
 45 }
 46 
 47 .browserButton {
 48   -moz-box-orient: vertical;
 49   min-width: 0px;
 50   list-style-image: url("chrome://browser/skin/Toolbar.png");
 51 }
(Assignee)

Comment 1

13 years ago
Okay, so what exactly do you want here?  There's an obvious toolkit dependency
on browser here which will be eradicated in bug 260058; how exactly it will be
eradicated is still uncertain.

Because Help uses many of the images the browser uses, I was thinking of
creating a default skin for Help (with paths to images in chrome://help/skin/)
but overriding it when building Firefox to reduce the download size by
overriding the toolkit help.css with a browser-specific help.css (with paths to
the relevant images in chrome://browser/skin/) and somehow removing the images
from the help.jar package.  I hadn't ironed out the details of exactly how this
would work yet.

If you want only chrome://help/skin/ images, that's easy enough to do.  As I was
planning on using as many chrome://browser/skin/ as possible to reduce build
size, tho, could you explain exactly what benefits are gained by using only
chrome://help/skin/ images?  It's unclear what the pros of this are, while the
cons (bigger build size, more duplication, more effort needed to sync button
images) are pretty obvious to me.
Blocks: 260058
(Reporter)

Comment 2

13 years ago
There are two approaches:
1) Use browser's theme's for everything and not have your own.  That means have
it use the same CSS as browser.css, but not literally.  Maybe an @import if you
have to.  If I am going to theme the Help Viewer, I'd want it to be the same as
my browser theming, and of course I don't want to maintain the same code in many
places.  Pinball is already riddled with that.

2) Be self-contained.  Any changes to the browser's chrome will not affect you.
 If a theme does not include Help Viewer css, then they will see the defaults. 
The downsides to this include having to change your CSS every time they alter
one of the standard buttons.  

Obviously I prefer option 1.  Why have #help-back-button and not just use
#back-button ?  Any images you have that aren't in classic.jar will need to be
moved, of course.  
(Reporter)

Comment 3

13 years ago
There's also inconsistent use of .toolbarbutton-1 and .browserButton

etc etc

Comment 4

13 years ago
Jeff, you might want to talk to ben about this first. Help originally didn't use
browser buttons but he requested that it be done in order to reduce bloat. And
if ben doesn't like it than it won't go in :).
Maybe we need/want to have certain "universal" buttons as part of the core theme
in toolkit?  This isn't super-critical because there's no overlap because we
have multiple themes in /toolkit that aren't currently shared, but if we have a
Winstripe for Tbird replacing Qute at some point, it makes sense to have the
icons in a single place.

Or, alternatively, move the Help css into the app directory so each app skins
stuff their own way.
(Assignee)

Comment 6

13 years ago
(In reply to comment #5)
> Maybe we need/want to have certain "universal" buttons as part of the core
> theme in toolkit?  This isn't super-critical because there's no overlap
> because we have multiple themes in /toolkit that aren't currently shared, but
> if we have a Winstripe for Tbird replacing Qute at some point, it makes sense
> to have the icons in a single place.
> 
> Or, alternatively, move the Help css into the app directory so each app skins
> stuff their own way.

If I haven't said this elsewhere, this would be ideal.  This actually happens
for some things (throbber on Mac only, for example -- there's a bug for it
somewhere that I can't remember), but it doesn't for others.

By the way, my computer is now back in fully, *fully* working condition.  A BIOS
update and some compressed air to clear out the vents did the trick, and I'm now
going through the backlog of about 970-odd bugmails since mid-November, when
things started failing.  As I have January all but completely free to work on
Mozilla-related things, this is the ideal time to get these and other such
issues with Help fixed -- I just need a little help here and there with things I
can't do (like reviews).  I'll be working over the next couple days on a list of
these things so I can make my requests for possibly expedited attention soon...

Comment 7

13 years ago
Created attachment 176405 [details] [diff] [review]
Patch - untested

(In reply to comment #5)
> 
> Or, alternatively, move the Help css into the app directory so each app skins

> stuff their own way.

sounds good to me. Proposal attached. Not too sure what to do with help.xul. I
put a #ifdef in there for now but it's a bit rough. Got any idea for another
solution?

Updated

13 years ago
Assignee: jwalden+fxhelp → rj.keller
Status: NEW → ASSIGNED

Comment 8

13 years ago
Created attachment 176406 [details] [diff] [review]
new CSS file -> browser/themes/help.css

Here's the CSS file, since my CVS SSH key is long gone and can't diff new files
without it.
(Assignee)

Comment 9

13 years ago
(In reply to comment #5)
> Maybe we need/want to have certain "universal" buttons as part of the core
> theme in toolkit?  This isn't super-critical because there's no overlap
> because we have multiple themes in /toolkit that aren't currently shared, but
> if we have a Winstripe for Tbird replacing Qute at some point, it makes sense
> to have the icons in a single place.

I think this is the only correct way to do it.  App-specific Help viewer
skinning is essentially forking the Help viewer CSS for every app that uses it,
and as Help Viewer's part of toolkit it should be fully self-contained.

(In reply to comment #7)
> sounds good to me. Proposal attached. Not too sure what to do with help.xul. I
> put a #ifdef in there for now but it's a bit rough.

Given that creating a stash of generic buttons is the correct way to do this,
and also given that this patch would introduce an app-specific #ifdef that
doesn't do much to add to the utility of the Help viewer, I think this bug will
have to wait for a different patch.  This seems like it shouldn't be overly
difficult -- maybe I'll take a look at it in a couple weeks after we fix bug
279506, which is currently a far more pressing concern.

Updated

13 years ago
Component: Help Viewer → Help Viewer
Product: Firefox → Toolkit
Version: 1.0 Branch → unspecified

Updated

12 years ago
Blocks: 292995
(Assignee)

Comment 10

12 years ago
If we can get bug 295817 fixed, finishing this up should be pretty easy.  The
goal's to get that bug done by 1.1, and given how long the development cycle for
the next release is likely to be (and also given that we're seeing more Help
users), I *really* want to see a fully app-independent Help by 1.1 - one that's
usable by Sunbird, xulrunner apps, and Thunderbird once it gets its act
together.  Barring any unexpected developments, expect to see this bug fixed in
the 1.8 timeframe.
Assignee: rj.keller → jwalden+fxhelp
Status: ASSIGNED → NEW
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta3
(Assignee)

Comment 11

12 years ago
Patch and attachment with new images coming soon...
Status: NEW → ASSIGNED
(Assignee)

Comment 12

12 years ago
Created attachment 188718 [details]
ZIP of the various newly-edited Toolbar.pngs

The format of a new toolbar image is as follows:

+--------------+-----------------+--------------+---------------+
| Back/normal  | Forward/normal  | Home/normal	|  Print/normal |
+--------------+-----------------+--------------+---------------+
| Back/hover   | Forward/hover	 | Home/hover	|  Print/hover	|
+--------------+-----------------+--------------+---------------+
| Back/disable | Forward/disable | Home/disable | Print/disable |
+--------------+-----------------+--------------+---------------+

Home/disable and Print/disable are currently unused, but I think avoiding any
possible future hassle is worth the extra cost of leaving them in place instead
of just marking their areas transparent.

The size of each tile is dependent upon the theme itself.  The burden from such
an image is between 10-20K depending on the size of the buttons, which I think
is acceptable.
(Assignee)

Comment 13

12 years ago
Created attachment 188724 [details] [diff] [review]
Patch

This patch has bits of bug 296012 mixed in it, and it'll conflict with the
patch in bug 299710.  I'll wait until those are wrapped up before requesting
review (on a new version of the patch, probably).

The patch requires the images in attachment 188718 [details] to be in the tree for it to
work properly.

I haven't been able to get a build environment working in Windows, so it's
untested both there and on Macs.  I suspect it'll work fine there, too, because
the fiddling I had to do to get Linux to work correctly also applied pretty
easily to Windows.

This patch patches qute in addition to the two main toolkit themes, pinstripe
and winstripe.	No testing whatsoever was done to qute, and the changes are
only to get rid of a few ifdefs and put it in a possibly-working state whenever
someone decides to use qute with Help.	I don't think it's worth really doing a
thorough review of the qute changes -- just skim to see whether the general
idea's correct and leave it at that, if it's even worth reviewing them.
Attachment #176405 - Attachment is obsolete: true
Attachment #176406 - Attachment is obsolete: true
(Assignee)

Comment 14

12 years ago
Created attachment 189221 [details] [diff] [review]
Final patch

This patch should remove the last vestiges of Firefox-specificness from the
help viewer, making it a proper part of toolkit.  It's tested on Linux but is
untested on Mac because I don't own one and Windows because I don't have a
working build environment.

The patch requires some mucking around with image files to work properly.  To
test, open attachment 188718 [details].  Inside it are three directories -- one for each
theme in toolkit.  Inside each directory you'll find a Toolbar.png specific to
that theme containing the image the help viewer uses.  Copy the Toolbar.png
over the existing copy in mozilla/toolkit/themes/<theme>/help/ (where <theme>
corresponds to the directory within attachment 188718 [details]).  Apply the patch and
rebuild to get a fully app-independent help viewer.
Attachment #188724 - Attachment is obsolete: true
Attachment #189221 - Flags: first-review?(mconnor)
(Assignee)

Updated

12 years ago
Attachment #189221 - Flags: first-review?(mconnor) → first-review?(kevin)

Updated

12 years ago
Attachment #189221 - Flags: first-review?(kevin) → first-review+
(Assignee)

Comment 15

12 years ago
Comment on attachment 189221 [details] [diff] [review]
Final patch

This should make the toolkit help viewer usable by non-Firefox applications
such as Sunbird.
Attachment #189221 - Flags: approval1.8b4?
Attachment #189221 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 16

12 years ago
The patch and toolbar palette image updates have been checked in, so I'm marking
this FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta3 → mozilla1.8beta4
Product: Toolkit → Seamonkey
You need to log in before you can comment on or make changes to this bug.