259002 - clean up Help Viewer shortcuts (forward/backward/home/reload commandkeys missing)

Status: RESOLVED FIXED

(SeaMonkey :: Help Viewer, defect)

Description

[Jo Hermans]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; rv:1.7.3) Gecko/20040911
Firefox/0.10
(first 1.0PR release candidate)
Mac OS X 10.2.8

I noticed that option-left/right keyboard shortcuts aren't hooked up in the Help
Viewer, but I couldn't find an exisiting bug, except bug 256231 (which wants
full keybaord access). Is this also true on other platforms (where they will be
alt-left/right) ?

I don't think that it should block the 1.0 release though. There are more urgent
bugs.

Comment 1

[Jeff Walden (gone starting June 8)]

Yup, this is *nix/win too.  It'll be an l10n change, so my guess is that it
won't make 1.0 because now only late-l10n bugs that affect l10n will get in
aviary.  For consistency it should be added for 1.0, but I don't know that the
benefits are worth the l10n headache it creates.

The patch should be relatively simple to do.  Maybe I'll get to it sometime this
week after we wrap up a few docs bugs.

Too many bindings have been added piecemeal -- Ctrl+0, Ctrl++, and probably
another one or two that I've forgotten.  This *should* be morphed into a bug to
"examine browser.xul keybindings and port all to Help window".  Whether that
happens now or not depends on how much time the fixer is will to devote to a patch.

Comment 2

[Steffen Wilberg]

More missing shortcuts are Alt+Home (Home), Backspace (Back), F3 (Find again),
Shift+F3 (Find Previous), Ctrl+Shift+G (Find Previous).

But that won't be an l10n change, because we VK_LEFT, VK_RIGHT, VK_HOME,
VK_BACK, and VK_F3 will be hardcoded into help.xul, just like a bunch of other
VK_* keys, and the "G" commandkey is already there (Ctrl+G = Find again).

I guess we don't need Full Screen and Reload.

There's other cleanup needed as well, e.g.
<key id="key_close"/>
<keyset id="viewZoomKeys"/>
<keyset id="navigationKeys"/>
do just nothing.

<key id="key_viewNextHelpPanel" keycode="VK_TAB" 
     oncommand="showRelativePanel(true);" modifiers="control"/>
doesn't seem to work, but this works:
<key id="key_viewPrevHelpPanel" keycode="VK_TAB" 
     oncommand="showRelativePanel(false);" modifiers="control,shift"/>

Comment 3

[Steffen Wilberg]

Is this just me, or do the Up/Down arrow keys, PgUp, PgDown, Home, End not work
in Firefox's Help Viewer?

Comment 4

[Jo Hermans]

(In reply to comment #3)
> Is this just me, or do the Up/Down arrow keys, PgUp, PgDown, Home, End not work
> in Firefox's Help Viewer?

They work for me, on Mac Os X and a slightly older version of Windows (I'll
install the RC as soon as possible).

Comment 5

[Jo Hermans]

(In reply to comment #2)
> I guess we don't need Full Screen and Reload.

Reload is bug 230693

Comment 6

[Steffen Wilberg]

> > Is this just me, or do the Up/Down arrow keys, PgUp, PgDown, Home, End
> > not work in Firefox's Help Viewer?
> They work for me, on Mac Os X and a slightly older version of Windows (I'll
> install the RC as soon as possible).
Works for me on Windows as well. But not in my GTK2 build. They do work in the
main browser window.(In reply to comment #5)

> Reload is bug 230693
That's about View Source, not the Help Viewer.

Comment 7

[Jeff Walden (gone starting June 8)]

(In reply to comment #3)
> Is this just me, or do the Up/Down arrow keys, PgUp, PgDown, Home, End not work
> in Firefox's Help Viewer?

Is the content area focused?  I find they do work when the content area is
focused, but not when it isn't.

So long as it's possible to load remote Help documentation (untested, but likely
possible) or load images in Help documentation remotely (we do it now), we
should add the keybinding for Reload.  If remote content fails for an unknown
reason, it should be reloadable the same way as remote content in the main
window is reloadable.

Comment 8

[Steffen Wilberg]

Never mind, it works in official builds. I'll file a new bug when I've found out
more.

And I'll add VK_F5 as well.

Comment 9

[Steffen Wilberg]

Attachment: patch

Adds Alt+Home (Home), Backspace (Back), Shift+Backspace (Forward), F3 (Find
again), Shift+F3 (Find Previous), Ctrl+Shift+G (Find Previous), F5 (Reload).
The go back/forward keys are copied from browser-sets.inc.
Makes Reload actually work.

Removes viewZoomCommands, viewZoomKeys (text zoom: viewZoomOverlay.xul),
navigationKeys (go back/forward/home, reload, full screen:
platformNavigationBindings.xul), and clipboardEditMenuItems. Seamonkey uses
them, we're not.

Do we need Accel+[ for Back? On Mac and Linux only? That would have l10n
impact.

Comment 10

[Steffen Wilberg]

Attachment: diff -uw for review

Comment 11

[Steffen Wilberg]

Comment on attachment 159091 [details] [diff] [review]
diff -uw for review

I'm not sure who has to review help.xul patches. Browser people? Or Help
people?
Help System is a separate module, but the files live in browser and toolkit.
Maybe we should move them to a separate directory?

Comment 12

[Mike Connor [:mconnor]]

IMO, Help Viewer needs to be consolidated into toolkit, minus #ifdefs, as much
as possible, and subject to toolkit review.  The only thing we should really
have on the browser/mail/whatever train is content and styling.  But that's
separate, really.

rjk was deemed ok to review stuff in in Help previously, but I don't think that
carries over to Jeff yet.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 159091 [details] [diff] [review]
diff -uw for review

>Index: mozilla/toolkit/components/help/content/help.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/help/content/help.js,v
>retrieving revision 1.1.4.1.2.3
>diff -u -p -w -8 -r1.1.4.1.2.3 help.js
>--- mozilla/toolkit/components/help/content/help.js	25 Jul 2004 15:01:34 -0000	1.1.4.1.2.3
>+++ mozilla/toolkit/components/help/content/help.js	16 Sep 2004 12:47:48 -0000
>@@ -16,16 +16,17 @@
> # The Initial Developer of the Original Code is
> # Ian Oeschger.
> # Portions created by the Initial Developer are Copyright (C) 2003
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #      [e-mail]
> #      [e-mail]
>+#      Steffen Wilberg <[e-mail]>

If we're going to do this, we should at least put names for the others.


>-    <commandset id="clipboardEditMenuItems"/>
why don't we have at least Copy in the context menu?  That doesn't make sense
to me.

>         <key id="printKb" key="&printCmd.commandkey;" oncommand="print();"
>             modifiers="accel"/>
>-        <keyset id="viewZoomKeys"/>
>-        <keyset id="navigationKeys"/>
>         <key id="key_find" key="&findOnCmd.commandkey;"
>             command="Help:Find" modifiers="accel"/>
>         <key id="key_findAgain" key="&findAgainCmd.commandkey;"
>             command="Help:FindAgain" modifiers="accel"/>
>+        <key id="key_findPrevious" key="&findAgainCmd.commandkey;"
>+            command="Help:FindPrevious" modifiers="accel,shift"/>

spacing nit, this is all wrong, I hate this crap-ass style used here.  I'll let
it slide based on when-in-Rome, but Fx Help needs some enforcement of coding
style. (especially the tabs I'm noticing in existing code.

>+        <key keycode="VK_F3" command="Help:FindAgain"/>
>+        <key keycode="VK_F3" command="Help:FindPrevious" modifiers="shift"/>
>+        <key keycode="VK_F5" oncommand="reload();"/>

If we're going to bother doing this, are we going to have a button too? 
Context menus shouldn't be the only way to perform an action (ref. GNOME HIG)

There's a lot of problems I have with the current Help setup, but this is ok
for 1.0.

I think we need to make big changes for 1.5, in a number of places, but that's
future fodder for post 1.0.

Comment 14

[Steffen Wilberg]

> > # Contributor(s):
> > #      [e-mail]
> > #      [e-mail]
> >+#      Steffen Wilberg <[e-mail]>
> If we're going to do this, we should at least put names for the others.
Sure.
 
> >-    <commandset id="clipboardEditMenuItems"/>
> why don't we have at least Copy in the context menu?  That doesn't make sense
> to me.
I thought about that as well, but first I need to find out how. Fact is, this
commandset does absolutely nothing, because the overlay Seamonkey uses is
missing. Ctrl+C already works btw.

> spacing nit, this is all wrong, I hate this crap-ass style used here. I'll let
> it slide based on when-in-Rome, but Fx Help needs some enforcement of coding
> style. (especially the tabs I'm noticing in existing code.
I didn't want to rewrite the whole file. And note that this is the diff -uw
patch. I did remove a bunch of tabs and trailing whitespace.

> >+        <key keycode="VK_F3" command="Help:FindAgain"/>
> >+        <key keycode="VK_F3" command="Help:FindPrevious" modifiers="shift"/>
> >+        <key keycode="VK_F5" oncommand="reload();"/>
> 
> If we're going to bother doing this, are we going to have a button too? 
> Context menus shouldn't be the only way to perform an action (ref. GNOME HIG)
But we don't need to show that by default, do we? I'll file a follow-up bug.

> There's a lot of problems I have with the current Help setup, but this is ok
> for 1.0.
Especially context help is missing. And some content, bookmarks, livemarks and
tabbed browsing for instance. But Blake said he's got something written for his
guidebook. We'll see.

> I think we need to make big changes for 1.5, in a number of places, but that's
> future fodder for post 1.0.
We've been working hard to follow the changes between 0.9 and PR, just have a
look at our tracker bug 253104. But there's still a lot we can do in the next 4
weeks. And we don't mind a helping hand :)

Comment 15

[Steffen Wilberg]

(In reply to comment #12)
> IMO, Help Viewer needs to be consolidated into toolkit, minus #ifdefs, as much
> as possible, and subject to toolkit review.  The only thing we should really
> have on the browser/mail/whatever train is content and styling.  But that's
> separate, really.
Er, have a look: http://lxr.mozilla.org/aviarybranch/source/browser/components/help/
There's only locale in browser. But helpMenuOverlay.xul is indeed an ugly beast,
and I made it even uglier :)

Comment 16

[Jeff Walden (gone starting June 8)]

(In reply to comment #12)
> IMO, Help Viewer needs to be consolidated into toolkit, minus #ifdefs, as much
> as possible, and subject to toolkit review.  The only thing we should really
> have on the browser/mail/whatever train is content and styling.  But that's
> separate, really.

I've been thinking about that some wrt a few different areas.  First, the
#ifdefs need to go somehow, and second, we need a way to include a glossary
without being forced to add glossary entries to toolkit.  The glossary should
live in browser/components/help/, because otherwise Thunderbird will pick up the
glossary for Firefox (which obviously doesn't work).  Some of this is
post-aviary (like the #ifdefs), while other parts may need temporary aviary
workarounds (move glossary completely into b/c/h and have completely separate
glossary code for apps -- not good in long run, but it'll work for short run).

> rjk was deemed ok to review stuff in in Help previously, but I don't think
> that carries over to Jeff yet.

I'd have been somewhat surprised if it did.  Personally, I'd feel okay reviewing
*minor* changes to all XUL files, *maybe* some minor changes to JS files (if
changes touched the RDF parsing code, definitely not -- still need to learn that
stuff), non-major changes to CSS files, and changes to most RDF files.  This
patch (well, what it became) is far bigger than I'd even consider.

Comment 17

[timeless]

so, if i need *help* reading content, i can't increase the font size? thank you.

Comment 18

[Steffen Wilberg]

(In reply to comment #17)
> so, if i need *help* reading content, i can't increase the font size? thank you.
Wrong. I'm removing non-functional leftovers from Seamonkey Help. Of course you
 can still use Ctrl +/-/0, use the context menu, or the toolbar buttons to zoom.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 159091 [details] [diff] [review]
diff -uw for review

a=asa for aviary checkin.

Comment 20

[Steffen Wilberg]

Checked in branch & trunk 2004-09-27 00:07 - 00:08.