256683 - Make Ctrl+0 work

Status: RESOLVED FIXED

(SeaMonkey :: Help Viewer, defect)

Description

[Jeff Walden (gone starting June 8)]

It was brought to my attention somewhere that Ctrl+0 (restore text size to
default) doesn't work in Help Viewer.

After I make a patch (Tuesday morning, most likely) I'll nominate for 1.0PR. 
(Of course, if someone else wants to grab this, feel free.  There's a
semi-decent chance I won't have the time do it.  The patch is probably about a
one-line addition, anyways.)

Comment 1

[Jo Hermans]

Shouldn't these lines be added to
/cvsroot/mozilla/toolkit/components/help/content/help.xul (sorry, not a proper
patch) ?

in commandset selectEditMenuItems :
<command id="cmd_textZoomReset"
oncommand="ZoomManager.prototype.getInstance().reset();"/>

in keyset keys :
<key id="key_textZoomReset" key="&textReset.commandkey;"
command="cmd_textZoomReset" modifiers="accel"/>

Note: I've left out the entries in the toolbar palette, becuase the main Browser
window doesn't contain a 'reset' command either. I'll file a bug about it.

in the file
/cvsroot/mozilla/toolkit/components/help/content/helpContextOverlay.xul :
<menuitem id="zoom-reset" label="&textZoomResetBtn.label;"
accesskey="&textZoomResetBtn.accesskey;"
oncommand="ZoomManager.prototype.getInstance().reset();"/>

in the file /cvsroot/mozilla/toolkit/components/help/locale/en-US/help.dtd :
<!ENTITY textZoomResetBtn.label       "Reset Text">
<!ENTITY textZoomResetBtn.accesskey   "s">
<!ENTITY textZoomResetBtn.tooltip     "Reset the size of the text in the 
Help documents.">

Comment 2

[R.J. Keller]

(In reply to comment #1)
> Shouldn't these lines be added to
> /cvsroot/mozilla/toolkit/components/help/content/help.xul (sorry, not a proper
> patch) ?
> 
> in commandset selectEditMenuItems :
> <command id="cmd_textZoomReset"
> oncommand="ZoomManager.prototype.getInstance().reset();"/>
> 
> in keyset keys :
> <key id="key_textZoomReset" key="&textReset.commandkey;"
> command="cmd_textZoomReset" modifiers="accel"/>
> 
> Note: I've left out the entries in the toolbar palette, becuase the main Browser
> window doesn't contain a 'reset' command either. I'll file a bug about it.
> 
> in the file
> /cvsroot/mozilla/toolkit/components/help/content/helpContextOverlay.xul :
> <menuitem id="zoom-reset" label="&textZoomResetBtn.label;"
> accesskey="&textZoomResetBtn.accesskey;"
> oncommand="ZoomManager.prototype.getInstance().reset();"/>
> 
> in the file /cvsroot/mozilla/toolkit/components/help/locale/en-US/help.dtd :
> <!ENTITY textZoomResetBtn.label       "Reset Text">
> <!ENTITY textZoomResetBtn.accesskey   "s">
> <!ENTITY textZoomResetBtn.tooltip     "Reset the size of the text in the 
> Help documents.">
> 


yup, plus you need <!ENTITY textZoomResetBtn.comandkey "0">.

Why don't you make a patch? Just run "cvs diff -u -w > new.diff".

Comment 3

[Jeff Walden (gone starting June 8)]

Attachment: Patch

(In reply to comment #1)
> Shouldn't these lines be added to
> /cvsroot/mozilla/toolkit/components/help/content/help.xul (sorry, not a
proper
> patch) ?
> 
> in commandset selectEditMenuItems :
> <command id="cmd_textZoomReset"
> oncommand="ZoomManager.prototype.getInstance().reset();"/>

Well, if we ever referred to Zoom Reset more than once, creating a <command/>
for it would make sense.  The main browser's style is just to use a <key/>.

> in keyset keys :
> <key id="key_textZoomReset" key="&textReset.commandkey;"
> command="cmd_textZoomReset" modifiers="accel"/>

Pretty much, except for the command attribute replaced with an oncommand
attribute.

> Note: I've left out the entries in the toolbar palette, becuase the main
> Browser window doesn't contain a 'reset' command either. I'll file a bug
> about it.

I don't think we need one; this is just so that Help behaves consistently with
the main interface.  If they ever change (almost definitely not before 1.0), we
will too.

> in the file
> /cvsroot/mozilla/toolkit/components/help/content/helpContextOverlay.xul :
> <menuitem id="zoom-reset" label="&textZoomResetBtn.label;"
> accesskey="&textZoomResetBtn.accesskey;"
> oncommand="ZoomManager.prototype.getInstance().reset();"/>
> 
> in the file /cvsroot/mozilla/toolkit/components/help/locale/en-US/help.dtd :
> <!ENTITY textZoomResetBtn.label	"Reset Text">
> <!ENTITY textZoomResetBtn.accesskey	"s">
> <!ENTITY textZoomResetBtn.tooltip	"Reset the size of the text in the 
> Help documents.">

Should Zoom Reset really be that visible?  It's nowhere in sight in the main
interface.  The patch here doesn't add a menuitem for that reason.

Comment 4

[Jeff Walden (gone starting June 8)]

Comment on attachment 156886 [details] [diff] [review]
Patch

Hmm, I notice you commented while I was patching.

Rationales:
-no <command/> because the main browser doesn't use one
-no menu item because main browser doesn't have one that I can see (Seamonkey
did, but not Firefox)
-no toolbar palette entry, although that, I suppose, is a reasonable idea
because we've got the enlarge/reduce buttons already
-very minimal, bare-bones functionality
-if browser doesn't have the access method, I don't think we need it either
-point is to add ctrl-0 keybinding, not extra stuff around it

Comment 5

[chris hofmann]

is this the same as http://bugzilla.mozilla.org/show_bug.cgi?id=198233

Comment 6

[Asa Dotzler [:asa]]

Help viewer font sizing is not a blocker. If you can get a patch in time for PR,
we'll consider it but not going to block on it.

Comment 7

[Jeff Walden (gone starting June 8)]

(In reply to comment #6)
> Help viewer font sizing is not a blocker. If you can get a patch in time for PR,
> we'll consider it but not going to block on it.

Asa, there *is* a patch - attachment 156886 [details] [diff] [review].

Comment 8

[R.J. Keller]

Comment on attachment 156886 [details] [diff] [review]
Patch

r=[e-mail]

Comment 9

[Mike Connor [:mconnor]]

its still not a blocker.  If it gets in, great, but that doesn't change whether
its a blocker.

Comment 10

[R.J. Keller]

mconnor: its a locale change. It can't go in after 1.0 PR.

Comment 11

[Mike Connor [:mconnor]]

if it doesn't go in by PR, then we ship without it.  A blocker is "we're not
going to ship without this" and its already been stated that yes, we will ship
without it if its not there.  It'd be nice to have but it is _not_ a blocker.

Comment 12

[R.J. Keller]

(In reply to comment #11)
> if it doesn't go in by PR, then we ship without it.  A blocker is "we're not
> going to ship without this" and its already been stated that yes, we will ship
> without it if its not there.  It'd be nice to have but it is _not_ a blocker.

drivers have given blocking+ status on many bugs like this, such as bug 253916.
If you really disagree, just keep it at blocking? so that drivers can determine
the right decision.

Comment 13

[Mike Connor [:mconnor]]

Asa said:
> Help viewer font sizing is not a blocker. If you can get a patch in time for PR,
> we'll consider it but not going to block on it.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 156886 [details] [diff] [review]
Patch

a=asa for branch checkin.

Comment 15

[Jeff Walden (gone starting June 8)]

Fixed branch & trunk.  (With new cvs commit access!  Woohoo!)

Hmm, just realized redwood is burning, so from what I've read of checkin rules I
shouldn't have checked in the fix to trunk.  (Or is that only when
*everything's* burning?)  However, a bunch of other people have checked in while
redwood's been burning.  An explanation would be helpful here -- respond
directly to prevent bugspam if you wish.