270629 - Add printBtn.label to help.xul and fix some other errors

Status: VERIFIED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[Stefan [:stefanh]]

The print button label is currently depending on navigator.dtd. It appears that
we're also using some button tooltips from navigator.dtd.

Comment 1

[Stefan [:stefanh]]

Attachment: patch

This adds "printbtn.label" to help.dtd and help.xul. Fixed the tooltips in
help.xul, changed some of the tooltip text as well. In order to be a bit more
consistent with the rest of the id names, we are now also using
"#helpPrintButton", instead of the previous "#print".

Comment 2

[[e-mail]]

Comment on attachment 166341 [details] [diff] [review]
patch

You don't seem to have removed the reference to navigator.dtd

>           label="&fwdBtn.label;" 
>-          tooltiptext="&forwardButton.tooltip;"
>+          tooltiptext="&forwardBtn.tooltip;"
Shouldn't this be fwdBtn.tooltip to match?

>-        <toolbarbutton id="print" class="toolbarbutton-1"
>+        <toolbarbutton id="helpPrintButton" class="toolbarbutton-1"
Please warn netscape.public.dev.skins about this id change.

>-#print[buttonover="true"] {
>+#helpPrintButton[buttonover="true"] {
I have another bug about this, and this is going to conflict with it, but you
should use the style of rules like the ones in the Modern theme.

Comment 3

[Stefan [:stefanh]]

> You don't seem to have removed the reference to navigator.dtd

These are in navigator.dtd:

<!ENTITY printCmd.commandkey "p" >
<!ENTITY findOnCmd.commandkey "f">
<!ENTITY findAgainCmd.commandkey "g">
<!ENTITY findPrevCmd.commandkey "g">
<!ENTITY findAgainCmd.commandkey2 "VK_F3">
<!ENTITY findPrevCmd.commandkey2 "VK_F3">
<!ENTITY pageSourceCmd.commandkey "u">
<!ENTITY pageInfoCmd.commandkey "i">

Surely we can remove pageSource and pageInfo from help.xul?

Comment 4

[[e-mail]]

Ah, then I had misunderstood the problem :-[

Should we actually be using more entities from navigator.dtd e.g. for back,
forward and home as well as print?

Comment 5

[Stefan [:stefanh]]

 
> Should we actually be using more entities from navigator.dtd e.g. for back,
> forward and home as well as print?

We could use all the entities except for the tooltip for "Home" and the throbber.

From the beginning I thought it was a good idea to include the entity for the
print button in help.dtd. It looked like an overkill to have to include
navigator.dtd for the xul buttons (that are going to replace the screenshots)
in, for instance, help-help.xhtml. 

Then i discovered that there were entities in help.dtd that we didn't use in
help.xul.

backBtn.label, fwdBtn.label, homeBtn.label, backBtn.tooltip, fwdBtn.tooltip
and homeBtn.tooltip are in help.dtd. Currently we're not using the tooltip
entities (apart from the home button and the throbber) that are in help.dtd. The
tooltips for the navigation buttons in help.dtd are currently "Back" and
"Forward", compared with "Go back/forward one page".

I guess this is just a matter of decision, we can:

1) Get rid of the reference to navigator.dtd in help.xul by including some more
entities in help.dtd. The advantages I can think of are: more logical to have
everything in a place where it belongs, not being dependant on entities from
another place, not having to load a bunch of entities that we're not using etc.

2) Use as much entities as possible from navigator.dtd. That would mean that
we're using all the button labels, tooltips etc etc, except for the "Home" and
throbber tooltips.
The advantages: Saves some footprint.

Comment 6

[[e-mail]]

OK, so let's put some figures on this:
a) how many entites does help.xul use?
b) how many of them exist in navigator.dtd (or other suitable dtd)?

Comment 7

[Stefan [:stefanh]]

> OK, so let's put some figures on this:
> a) how many entites does help.xul use?
27 (including tooltip. label, commandkey etc) 


> b) how many of them exist in navigator.dtd (or other suitable dtd)?
12 exists in nagigator.dtd and 15 in help.dtd

help.dtd has 24 entities. There are 9 unused entities:

<!ENTITY helpWin.ttl        "&brandShortName; Help" >
<!ENTITY tab1.label         "Contents">
<!ENTITY tab2.label         "Index">
<!ENTITY searchbtn.label       "Find">
<!ENTITY ixname.label          "Name">
<!ENTITY glossname.label       "Name">
<!ENTITY name.label            "Item">

<!ENTITY backBtn.tooltip    "Back" >
<!ENTITY fwdBtn.tooltip     "Forward" >

The last two are sort of "abandoned", help.xul uses the corresponding ones from
navigator.dtd instead (Go back/forward one page)

One question:

<key id="key_viewSource" key="&pageSourceCmd.commandkey;"
  oncommand="BrowserViewSource();" modifiers="accel"/>
<key id="key_viewInfo" key="&pageInfoCmd.commandkey;"
  oncommand="BrowserPageInfo();"   modifiers="accel"/>

Should this really be in help.xul?

Comment 8

[[e-mail]]

OK, so let's a) not use navigator.dtd b) get rid of those two keys.

Comment 9

[Stefan [:stefanh]]

Attachment: New version

OK, this patch does the following:

* Removes the two keys from help.xul.

* We now have all 25 entities used in help.xul in help.dtd. The obsolete ones
removed as well. Couldn't help to reorganize and remove/add some comments.

* Fixes the two css files (new id and changed styled rules).

* It appears that we're not using anything from brand.dtd, so I removed the
references to that file in help.xul (as well as navigator.dtd).

Comment 10

[Stefan [:stefanh]]

Attachment: New version without the rubbish at the top

Sorry, forgot to remove the first lines...

Comment 11

[Stefan [:stefanh]]

Comment on attachment 167217 [details] [diff] [review]
New version without the rubbish at the top

Sorry for the spam...

Comment 12

[[e-mail]]

Comment on attachment 167217 [details] [diff] [review]
New version without the rubbish at the top

><!DOCTYPE window [
><!ENTITY % helpDTD SYSTEM "chrome://help/locale/help.dtd" >
>%helpDTD;
>]>
Actually, perhaps we can shrink this further:
<!DOCTYPE window SYSTEM "chrome://help/locale/help.dtd">

Comment 13

[Stefan [:stefanh]]

Attachment: New version with one-line DTD

(In reply to comment #12)
> Actually, perhaps we can shrink this further:
> <!DOCTYPE window SYSTEM "chrome://help/locale/help.dtd">

Sure.

Comment 14

[Stefan [:stefanh]]

Comment on attachment 167273 [details] [diff] [review]
New version with one-line DTD

Neil, do I need sr on this one?

Comment 15

[Stefan [:stefanh]]

Comment on attachment 167273 [details] [diff] [review]
New version with one-line DTD

Not sure if this needs another r. Fixed a few whitespaces (apart from the DTD
change) in help.xul.

Comment 16

[[e-mail]]

Comment on attachment 167273 [details] [diff] [review]
New version with one-line DTD

Well, it's a fair sized patch, but maybe your superreviewer will give you rs
anyway...

Comment 17

[Stefan [:stefanh]]

Comment on attachment 167273 [details] [diff] [review]
New version with one-line DTD

Alec, a rubberstamp would hopefully be enough on this one.

Comment 18

[Stefan [:stefanh]]

Comment on attachment 167273 [details] [diff] [review]
New version with one-line DTD

jag, I hope a rs is enough on this one.

Comment 19

[jag (Peter Annema)]

Comment on attachment 167273 [details] [diff] [review]
New version with one-line DTD

sr=jag

Comment 20

[Stefan [:stefanh]]

Attachment: Update of the /skin diffs

It appears that the two help.css files have changed, here's an updated patch
for those.

Comment 21

[Steffen Wilberg]

Checking in mozilla/extensions/help/resources/content/help.xul;
/cvsroot/mozilla/extensions/help/resources/content/help.xul,v  <--  help.xul
new revision: 1.54; previous revision: 1.53
done
Checking in mozilla/extensions/help/resources/locale/en-US/help.dtd;
/cvsroot/mozilla/extensions/help/resources/locale/en-US/help.dtd,v  <--  help.dtd
new revision: 1.13; previous revision: 1.12
done
Checking in mozilla/extensions/help/resources/skin/classic/help.css;
/cvsroot/mozilla/extensions/help/resources/skin/classic/help.css,v  <--  help.css
new revision: 1.15; previous revision: 1.14
done
Checking in mozilla/extensions/help/resources/skin/modern/help.css;
/cvsroot/mozilla/extensions/help/resources/skin/modern/help.css,v  <--  help.css
new revision: 1.14; previous revision: 1.13
done

Comment 22

[Stefan [:stefanh]]

v