The default bug view has changed. See this FAQ.

Status

SeaMonkey
Help Viewer
RESOLVED FIXED
13 years ago
a year ago

People

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

Tracking

({fixed-aviary1.0})

unspecified
fixed-aviary1.0
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [have patch] affects l10n)

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
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

13 years ago
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

13 years ago
(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".
(Assignee)

Comment 3

13 years ago
Created attachment 156886 [details] [diff] [review]
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.
(Assignee)

Comment 4

13 years ago
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
Attachment #156886 - Flags: review?(rlk)
(Assignee)

Updated

13 years ago
Flags: blocking-aviary1.0PR?
Whiteboard: [have patch] affects l10n

Comment 5

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

Comment 6

13 years ago
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.
Flags: blocking-aviary1.0PR? → blocking-aviary1.0PR-
(Assignee)

Comment 7

13 years ago
(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].
Flags: blocking-aviary1.0PR- → blocking-aviary1.0PR?

Comment 8

13 years ago
Comment on attachment 156886 [details] [diff] [review]
Patch

r=rlk@trfenv.com
Attachment #156886 - Flags: review?(rlk)
Attachment #156886 - Flags: review+
Attachment #156886 - Flags: approval-aviary?

Updated

13 years ago
Flags: blocking-aviary1.0PR? → blocking-aviary1.0PR+
its still not a blocker.  If it gets in, great, but that doesn't change whether
its a blocker.
Flags: blocking-aviary1.0PR+ → blocking-aviary1.0PR-

Comment 10

13 years ago
mconnor: its a locale change. It can't go in after 1.0 PR.
Flags: blocking-aviary1.0PR- → blocking-aviary1.0PR+
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.
Flags: blocking-aviary1.0PR+ → blocking-aviary1.0PR-

Comment 12

13 years ago
(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.
Flags: blocking-aviary1.0PR- → blocking-aviary1.0PR+
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.

Flags: blocking-aviary1.0PR+ → blocking-aviary1.0PR-

Comment 14

13 years ago
Comment on attachment 156886 [details] [diff] [review]
Patch

a=asa for branch checkin.
Attachment #156886 - Flags: approval-aviary? → approval-aviary+
(Assignee)

Comment 15

13 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.0beta

Updated

12 years ago
Component: Help Viewer → Help Viewer
Flags: review+
Flags: blocking-aviary1.0PR-
Flags: approval-aviary+
Product: Firefox → Toolkit
Target Milestone: Firefox1.0beta → ---
Version: 1.0 Branch → unspecified
Flags: in-testsuite?
Product: Toolkit → Seamonkey
You need to log in before you can comment on or make changes to this bug.