295817 - Rework help viewer UI

Status: RESOLVED FIXED

(SeaMonkey :: Help Viewer, defect)

Description

[Jeff Walden (gone starting June 8)]

I've been thinking more and more about the Help viewer UI lately, partly with
respect to the accessibility concerns in bug 259052 and partly just because the
UI reeks of Netscape-ness and is to some extent larger than it needs to be.

I'm still trying to decide exactly what I want, working with both my own ideas
and ideas demonstrated in a mocked-up version Mike Connor made a while back. 
However, I do know that the first step I'd like to take is to pare down the
toolbar icons.

Currently the toolbar icons available are:

Toggle Sidebar            Customize
Increase/Decrease Text Size
Find                      Print
Back/Forward              Home
Activity Indicator

I think we can pare this list down to the following:

Toggle Sidebar
Back/Forward         Home
Print
Activity Indicator

==Justification for the removals==

Decrease/Increase Text Size:
When do you use these functions in Firefox?  You use them when you surf to a
page that has overly large or overly small text.  For example, some designers
seem to think some fonts look better at 90% instead of 100% for some odd reason.
 You might also (less often) use them to fit more text onto the screen.  For
example, if I'm viewing a presentation someone made and it almost (but not
quite) fits on the screen I might decrease text size.  Neither of these uses,
however, is likely to happen with Help content.  We're not showing arbitrary
content - we're showing content controlled by the app's author, and that
shouldn't be poorly designed.  The user's default text size should comfortably
display Help, and if it doesn't the problem lies with the accessibility of that
setting, not with Help.  Also, we're not displaying things where having a
smaller size might help with viewing.  The viewer's already displayed at smaller
than maximized, and Help content's unlikely to fit the second scenario.

Find:
The primary use of Find in the main browser is to search within a page for
something you (usually) know is there.  Help content is more often used as a
tutorial, with the user reading some amount of previously-unread material and
hopefully internalizing what's been read.  The utility of Find in this case is
rather lower.  The user may often have used the viewer's search function to get
where he is anyway, so chances are better that he's already at what he wants and
won't need in-text searching to find it.  

Customize:
This icon can go because I don't think we need a customizable toolbar.  The
utility of being able to remove the icon that will remain is extremely
questionable.  (See also the justification for remaining icons.)

These removals will *not* affect current keyboard shortcuts.  The user should
still be able to change text size using Ctrl++ and friends, and Ctrl+F should
still work properly after this is done.

==Justification for what remains in the toolbar==

Back and Forward are core functionality for something that acts like a web
browser. The Home concept is useful for providing a starting point to Help, and
it keeps the user from ever getting too lost. Print makes referring to
instructions easier, and requiring it to always be available makes telling the
user to print the current Help topic something you can always do.  Toggle
Sidebar is still required functionality as long as the sidebar can be hidden. 
(At some later time I'm pretty sure I want to remove this, but we're not quite
there yet.)

This leaves the Activity Indicator. It's less useful, but it helps explain to
the user why images are displaying slowly in Firefox Help (which stores
screenshots remotely) and tells the user when help content is stored remotely. 
I'm still tempted to remove it, but for now I'd like to give it some more
thought.  Perhaps we can duplicate its functionality elsewhere, which would be
ideal if it can be done properly.

==After the changes==

After the removals, the toolbar should display as follows:

+-----------------------------------------------------------------------+
| Back  Foward  Home  Print | Toggle Sidebar                   Activity |
+-----------------------------------------------------------------------+

As mentioned earlier, Toggle Sidebar and the separator should be removed soon
when I figure out how to reorganize the rest of the UI to make the sidebar
required.  If I later decide the Activity Indicator should go, the Print button
will move over to replace it.

Note that with this change that I think we're down to few enough duplicated
icons that we can ignore the small amount of bloat and just add them in toolkit,
so we'll finally be able to make the viewer fully app-independent.  (However,
that's another bug and a different patch.)

The patch for this is coming probably sometime later today.

Comment 1

[Mike Connor [:mconnor]]

Jeff, I've been looking for you on IRC, but I have been working on rewriting the
help viewer to use a Firefox/Thunderbird-style sidebar, and probably dropping
customization for the toolbars if I get that part right.  This needs to be done
in the next two weeks, to make sure we have time to get this tested by the
accessibility folks.

I'll hopefully have a screenshots in the next day or so, but I'm mostly in sync
with these ideas, which shouldn't be a surprise.

Comment 2

[Jeff Walden (gone starting June 8)]

Attachment: Removes icons, customize styles/code, and associated styles

(In reply to comment #1)
> Jeff, I've been looking for you on IRC

Classes ended last week, and since then I've mostly been busy looking for a
summer job.

> This needs to be done in the next two weeks, to make sure we have time to get

> this tested by the accessibility folks.

That fits with my timeline; my goal for Help in general is to have UI that's at
most a step or two from a freezable point, be fully app-independent (for proper
toolkit apps, meaning no Thunderbird), and have adequate support for
cross-platform documentation (bug 254992 is theoretically enough, but the
control's simply too granular for it to be useful enough for something as
OS-customized as Firefox), all by the next code freeze (and exactly when that
is is as clear as ever - as clear as mud, that is).

> I'll hopefully have a screenshots in the next day or so, but I'm mostly in
> sync with these ideas, which shouldn't be a surprise.

Well, in the event you're willing to work with the hacking I've done to meet
the goals mentioned here, here's the patch.  It's basically a lot of
slash-and-burn with no real additions, which is good for code size.  It works
fine on Windows for me, and I assume (but cannot test) that it works on Mac as
well.  Style code isn't organized exactly the same between winstripe and
pinstripe.

As for possible UI, I've been mulling over axing visible display of the
glossary and index in favor of relying purely on the table of contents.  Search
would still be available, although where I don't know.	Searching would look
through any glossary/index datasources, so content accessible only there would
still be available for some level of backwards-compatibility.  (This isn't
completely unheard of; there's always been the possibility to define additional
info sources and have them available only during Search according to docs,
although this didn't actually work in toolkit/ help until I fixed bug 294232.) 
Of course, this would require more emphasis on searching in the Help on Help
document (also possibly in app docs as well).

The problem I've been mulling over is exactly how to display Search and any
results in a way that's easily understood.  Having search results share the
same space as the table of contents is obvious enough, but making the switch
between modes painless and intuitive is the question.  Putting a search box
above the info pane is simple enough, but then you have to label it and somehow
make it obvious that clearing it will revert to the table of contents.	A Clear
button is just extra UI; something like Thunderbird's inline search box (which
uses an embedded [X] to clear it) could work, but how do you make it obvious
that the box has to be cleared to return to the table of contents?

Anyway, that's where I'm at now.  Hopefully the patch will be useful, and
hopefully we can get done with all this stuff before deadlines.

Comment 3

[Mike Connor [:mconnor]]

http://steelgryphon.com/stuff/help/ has a couple screenshots of what I'm
monkeying with right now.

Search box is in the tab order, the other items use extremely common shortcuts,
so per aaronlev, not worrying about those for accessibility reasoning.  Once you
start to search, the search results sidebar appears, clearing the field
automatically dismisses the sidebar and restores the contents listing.

Esc dismisses the search sidebar, contents list is always there (yes, this is a
significant change, but the entire Help content setup is reliant on that sidebar
being visible, so other than forcing lots of toggling, there isn't a benefit
that I see here).  The idea is to make this faster to use, and reduce the time
spent in the Help Viewer.

The patch is pretty big, even with -w its 700 lines in just
tookit/components/help, so this should be fun to get done in the next three
days.  that's not including what I need to bring in from Jeff's patch, either.

Feedback please, let me know if you want a (probably not clean) diff, since I'm
working on this in my hacked-for-a2 tree with tab dnd and tabbox changes, and I
think some other things (need a full tree diff so I can figure it out :)).

Comment 4

[Aaron Leventhal]

Mike, can I have accesskey mnemonics for the controls that have text labels?

So, _S_earch and _C_ontents

Comment 5

[Jeff Walden (gone starting June 8)]

(In reply to comment #3)
> Esc dismisses the search sidebar,

This is the problem I was hitting: how is the user supposed to discover this
(particularly if Esc to exit Help is already learned)?  Trial and error should
usually work, but I think we can come up with something more intuitive.  The
Close button somewhat does the trick, but what could be better is if we had a
button that sent you back to the contents list.  I'm just not sure what such a
button would look like, because it would have to take up very little space to be
effective.

> contents list is always there (yes, this is a significant change, but the
> entire Help content setup is reliant on that sidebar being visible, so other
> than forcing lots of toggling, there isn't a benefit that I see here).

That's about what I was expecting to happen.

> The patch is pretty big, even with -w its 700 lines in just
> tookit/components/help, so this should be fun to get done in the next three
> days.  that's not including what I need to bring in from Jeff's patch, either.
> 
> Feedback please, let me know if you want a (probably not clean) diff, since
> I'm working on this in my hacked-for-a2 tree with tab dnd and tabbox changes,
> and I think some other things (need a full tree diff so I can figure it out
> :)).

A preliminary diff using the following:

cvs diff -u8N toolkit/components/help toolkit/themes/winstripe/help
toolkit/themes/pinstripe/help toolkit/themes/qute/help
toolkit/locales/en-US/chrome/mozapps/help

...would be nice to see so I can get an idea where everything relevant to Help
is going.  (We might want to make changes to browser/locales/*/chrome/help
eventually, but I don't think we should be making them yet so we can be certain
things are still backwards-compatible.)

Comment 6

[Mike Connor [:mconnor]]

(In reply to comment #5)
> (In reply to comment #3)
> > Esc dismisses the search sidebar,
> 
> This is the problem I was hitting: how is the user supposed to discover this
> (particularly if Esc to exit Help is already learned)?

Esc to close windows shouldn't be learned.  Esc can cancel dialogs, but isn't a
close window shortcut, and shouldn't be.  Clearing the search field also removes
the search field, and Esc is used in this way without issues with the Find Toolbar.

Should have a usable patch later tonight, but there's a lot of cleanup I'm doing.

Comment 7

[Mike Connor [:mconnor]]

Attachment: first cut

This includes and extends waldo's patch to kill toolbar customization.	I don't
think that the throbber has real value at present, and on most modern broadband
setups it'll _never_ get shown since the content loads instantly, basically.

If some people can test this, especailly waldo since I'm concerned about
conflicts etc, if the other big bug needs some whacking then maybe we'll get
this in and refactor that patch for the new world.

And yes, I'll post a diff -w version when this is ready for review (read:
includes Mac styles etc.)

Also need to get a new Toolbar.png to eliminate the bogus winstripe->browser
dependency.

Comment 8

[Jeff Walden (gone starting June 8)]

(In reply to comment #6)
> Esc to close windows shouldn't be learned.  Esc can cancel dialogs, but isn't
> a close window shortcut, and shouldn't be.

You misunderstand me; I refer to Firefox 1.0 users who may possibly have grown
accustomed to the existing Esc shortcut to exit Help.  I agree they shouldn't
have learned it, but because it's there it's possible they did.

> Clearing the search field also removes the search field, and Esc is used in
> this way without issues with the Find Toolbar.

There's a substantial difference between Esc in the Find Toolbar and Esc in the
proposed changes:  find bar Esc *dismisses* the find bar UI completely whereas
Esc in search would *replace* the search results.

Of course, we seem to be overlooking the most obvious solution - do what's been
done before.  Both the History and Bookmarks sidebars always have the same
title, and immediately underneath is line for "Search:" and a small textbox for
search input.  Typing starts a search, and clearing reverts to tree display. 
Users will already be familiar with how this model works, and then we're not
completely replacing the sidebar on a search.  The only problem I see with this
is that it starts to clutter up the sidebar a bit.  I've hastily GIMPed up two
demos of this idea, one with the search box at bottom and one with it at the
top.  Bottom might be less usable if we expect the user to be working with a
mouse (because results show up at the top of the pane, not the bottom), but the
UI seems a tad bit cleaner done that way.  I don't know whether the tradeoff's
worthwhile or not - my guess is that it isn't.

http://whereswalden.com/files/mozilla/help/newhelp-search-top.png
http://whereswalden.com/files/mozilla/help/newhelp-search-bottom.png

More's coming after I get a chance to look at the patch in detail and test it.

Comment 9

[Jeff Walden (gone starting June 8)]

Comment on attachment 185281 [details] [diff] [review]
first cut

A couple things mentioned on IRC, paraphrased here:

Having Esc as a window-global shortcut to hide search results is at least
debatable.  Some points to make are that the model's different from that used
in the find bar and making Esc only work in the text field itself (and probably
the search tree, too, come to think of it) means you need more keypresses to
hide search (or you have to move the mouse to the close button).

Try commenting out the <template/> elements inside the hidden trees.  I'm
pretty sure they're not needed any more, because they exist only to make
displayable trees.  This means we'll probably have to ensure we're not calling
|tree.builder.rebuild()| then, because that probably errors on non-templated
datasourced elements.

There are also a couple CSS rules that don't apply to anything that obviously
are getting removed sometime down the line.

There are probably other things I didn't notice or forgot to mention because
I'm just reading the patch and not actually testing it yet (needed to rebuild
tree because it didn't seem to want to be compatible with mine), so further
analysis will have to wait.  Plus, I need sleep. :-)

Comment 10

[Jeff Walden (gone starting June 8)]

Comment on attachment 185281 [details] [diff] [review]
first cut

One other thing: jar.mn changes are missing to remove the two customization
files, so the patch fails to build.  I assume this was just an oversight during
running cvs diff, but it's worth mentioning so it's not forgotten next time.

Comment 11

[Jeff Walden (gone starting June 8)]

Currently, this bug is halting Help backend development, which is a problem.  I
need to make some backend fixes in bug 296012 to fix bug 289776, which affects
l10n and thus has to be finished for the l10n freeze.  Time's getting shorter
and shorter and I'm starting to get a little worried, so I'm requesting this
block 1.8b3.

Comment 12

[Mike Connor [:mconnor]]

Attachment: patch

Comment 13

[Mike Connor [:mconnor]]

Attachment: same as above with diff -w

Comment 14

[Mike Connor [:mconnor]]

Attachment: patch with minor fixes

Comment 15

[Mike Connor [:mconnor]]

Attachment: diff -w version

Comment 16

[Steffen Wilberg]

Comment on attachment 187918 [details] [diff] [review]
diff -w version

>Index: components/help/content/help.js
>===================================================================
>-    //Display the Table of Contents
>-    showPanel("help-toc");
>+  try { 
>+    var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                         .getService(Components.interfaces.nsIPrefBranch);
>+       pref.getBoolPref("browser.urlbar.clickSelectsAll");
>+  } catch(ex) {
>+    return def;
>+  }
>+
>+  gClickSelectsAll = getBoolPref("browser.urlbar.clickSelectsAll", true);
>+}
Looks like you want to remove try/catch section ("def" is not defined here
anyway) since you're using the new getBoolPref() in help.js.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 187918 [details] [diff] [review]
diff -w version

Some drive by review comments

>Index: components/help/content/help.js
>===================================================================

>+  try { 
>+    var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                         .getService(Components.interfaces.nsIPrefBranch);
>+       pref.getBoolPref("browser.urlbar.clickSelectsAll");
>+  } catch(ex) {
>+    return def;
>+  }
>+
>+  gClickSelectsAll = getBoolPref("browser.urlbar.clickSelectsAll", true);
>+}
>+

The whole try-catch block isn't needed here.

>Index: components/help/content/help.xul
>===================================================================

>@@ -67,53 +66,57 @@
> #endif
> #ifdef HELP_ALWAYS_RAISED_TOGGLE
>         persist="width height screenX screenY zlevel"
> #else
>         persist="width height screenX screenY"
> #endif
>         onload="init();"
>         onunload="uninitFindBar(); window.XULBrowserWindow.destroy();">
> 
>-    <script type="application/x-javascript" src="chrome://help/content/toolbarCustomization.js"/>
>     <script type="application/x-javascript" src="chrome://help/content/help.js"/>
>     <script type="application/x-javascript" src="chrome://global/content/findBar.js"/>
>     <script type="application/x-javascript" src="chrome://global/content/viewZoomOverlay.js"/>
>     <script type="application/x-javascript" src="chrome://global/content/globalOverlay.js"/>
>+  <script type="application/x-javascript" src="chrome://global/content/utilityOverlay.js"/>

There is no such script (it's in the browser package). Also, the only function
form utilityOverlay which was used here is |getBoolPref|, which was coppied
into help.js...


>     <broadcasterset id="helpBroadcasters">
>         <broadcaster id="canGoBack"    disabled="true"/>
>         <broadcaster id="canGoForward" disabled="true"/>
>+    <broadcaster id="viewIndexSidebar" autoCheck="false" label="Index"
>+                 type="checkbox" group="sidebar"
>+                 oncommand="showIndexSidebar();"/>
>+    <broadcaster id="viewTOCSidebar" autoCheck="false" label="Contents"
>+                 type="checkbox" group="sidebar"
>+                 oncommand="showTOCSidebar();"/>
>+    <broadcaster id="viewSearchSidebar" autoCheck="false" label="Search"
>+                 type="checkbox" group="sidebar"
>+                  oncommand="showSearchSidebar();"/>

Extra space in the last line (and you might want each attribute in a new line).

>+    <key id="key_closeSearchSidebar" keycode="VK_ESCAPE"
>+         oncommand="hideSearchSidebar()"/>
>         <key id="key_textZoomEnlarge" key="&textZoomEnlargeCmd.commandkey;"
>             command="cmd_textZoomEnlarge" modifiers="accel"/>
>         <key id="key_textZoomEnlarge2" key="&textZoomEnlargeCmd.commandkey2;"
>             command="cmd_textZoomEnlarge" modifiers="accel"/>
>         <key id="key_textZoomReduce" key="&textZoomReduceCmd.commandkey;"
>             command="cmd_textZoomReduce" modifiers="accel"/>
>         <key id="key_textZoomReset" key="&textZoomResetCmd.commandkey;"
>             oncommand="ZoomManager.prototype.getInstance().reset();" modifiers="accel"/>
>-        <key id="key_toggleSidebar" keycode="VK_F9" command="Help:ToggleSidebar"/>
>-        <key id="key_viewNextHelpPanel" keycode="VK_TAB"
>-             oncommand="showRelativePanel(true);" modifiers="control"/>
>-        <key id="key_viewPrevHelpPanel" keycode="VK_TAB"
>-             oncommand="showRelativePanel(false);" modifiers="control,shift"/>
>+    <key id="key_focusSearch" key="&helpSearch.commandkey;"
>+         oncommand="focusSearch()" modifiers="accel" />

We don't have this entity, either in the patch or in the CVS.

>+         

Whitespaces only line here...


>+    <toolbox id="help-toolbox" class="toolbox-top" customizable="false">

customizable="false" is the default, we only do customizable="true" when
necessary.

>+          <textbox id="findText" type="timed" timeout="500"
>+                   onfocus="URLBarFocusHandler(event, this);"
>+                   onmousedown="URLBarMouseDownHandler(event, this);"
>+                   onclick="URLBarClickHandler(event, this);"
>+                   oncommand="doFind();"/>

URLBar*Handler should be renamed (Searchbar*Handler), we don't have a urlbar in
the help viewer :)

Comment 18

[Asaf Romano (gone)]

Also, at least in pinstripe, the back and forward buttons can get a pressed look
even when they're disabled.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 187918 [details] [diff] [review]
diff -w version


>Index: components/help/content/help.js
>===================================================================

>+function focusSearch() {
>+  var searchBox = document.getElementById("findText");
>+  searchBox.focus();
>+  if (gClickSelectsAll)
>+    aElt.select();
>+}
>+

aElt isn't defined, i assume you meant |searchBox|.


Also when each sidebar gets hidden, the content area (or maybe the other
sidebar?) should be focused.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 187918 [details] [diff] [review]
diff -w version

>-   <toolbar id="helpToolbar" toolbarname="Help Toolbar" class="toolbar-primary chromeclass-toolbar"

...
>     <toolbar id="HelpToolbar">

Removing class="chromeclass-toolbar" breaks the os x's collpase toolbar button.

Comment 21

[Jeff Walden (gone starting June 8)]

Comment on attachment 187918 [details] [diff] [review]
diff -w version

Other stuff:

>+function showSearchSidebar() {
>+  var elt = document.getElementById("help-find-button");
>+  var tableOfContents = document.getElementById("help-toc-sidebar");
>+  tableOfContents.setAttribute("hidden", "true");
>+
>+  var sidebar = document.getElementById("help-search-sidebar");
>+  sidebar.removeAttribute("hidden");
>+  var elts = document.getElementsByAttribute("group", "sidebar");
>+  for (var i = 0; i < elts.length; ++i)
>+    elts[i].removeAttribute("checked");
>+
>+  if (elt) {
>+    elt.setAttribute("checked", "true");
>+  }
>+}

help-find-button is getting removed in the patch, so I'm not sure why you're
doing anything with it here.  Also, what are the broadcasters doing that they
need a |checked| attribute to be removed?

>+function hideSearchSidebar() {
>+  var sidebar = document.getElementById("help-search-sidebar");
>+  sidebar.setAttribute("hidden", "true");
>+
>+  var tableOfContents = document.getElementById("help-toc-sidebar");
>+  tableOfContents.removeAttribute("hidden");
>+  
>+  var elts = document.getElementsByAttribute("group", "sidebar");
>+  for (var i = 0; i < elts.length; ++i)
>+    elts[i].removeAttribute("checked");
> }

Once again, why are we removing the |checked| attribute from the broadcasters?

>+var gIgnoreFocus = false;
>+var gClickSelectsAll;
>+var gIgnoreClick = false;

Personally, I'd prefer these be declared at the top of the file.

> # doFind - Searches the help files for what is located in findText and outputs into
> #	the find search tree.
> function doFind() {
>+    var sidebar = document.getElementById("help-search-sidebar");
>+    if (sidebar.hidden)
>+      showSearchSidebar();

I don't think you gain much in readability by creating a sidebar variable and
then asking that for its hidden-ness.  The id is clear enough that |if
(document.getElementById("help-search-sidebar").hidden)| should be fine.

>+function getBoolPref ( prefname, def )

Fix the spacing of the arguments here, please.

>+  try { 
>+    var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                       .getService(Components.interfaces.nsIPrefBranch);
>+    return pref.getBoolPref(prefname);
>     }
>+  catch(er) {
>+    return def;
>   }

|e| is more common in toolkit code than |er| or |ex|, so go with |e| here.  We
don't even use the exception, so we might as well save a byte or two.

>+  <script type="application/x-javascript" src="chrome://global/content/utilityOverlay.js"/>

Since according to previous comments this doesn't exist, what were the reasons
you added it (if they actually matter)?

>+    <broadcaster id="viewIndexSidebar" autoCheck="false" label="Index"
>+                 type="checkbox" group="sidebar"
>+                 oncommand="showIndexSidebar();"/>
>+    <broadcaster id="viewTOCSidebar" autoCheck="false" label="Contents"
>+                 type="checkbox" group="sidebar"
>+                 oncommand="showTOCSidebar();"/>
>+    <broadcaster id="viewSearchSidebar" autoCheck="false" label="Search"
>+                 type="checkbox" group="sidebar"
>+                  oncommand="showSearchSidebar();"/>

I don't see these being used anywhere except to hold |checked| attributes, and
they aren't being observed by anything, so why are they here?  Also, if these
do belong here |label| should be localized.


>Index: themes/winstripe/help/help.css
>===================================================================

>-.toolbarbutton-1, .browserButton, .toolbarbutton-menubutton-button {
>-  padding: 5px;
>+.toolbarbutton-1, .browserButton, .toolbarbutton-menubutton-button, toolbarbutton[checked="true"] {
>+
> }

> .toolbarbutton-1 .toolbarbutton-icon,
> .browserButton .toolbarbutton-icon {
>-  -moz-margin-end: 0px;
> }

These obviously can be removed, assuming they don't adversely affect anything.

> /* Set the minimum sidebar width so the help contents aren't squeezed together.*/
>-#helpsidebar-box { width: 200px; }
>+#help-sidebar-box { min-width: 150px; max-width: 250px; }

We probably want to change this to ems or something, because this will clearly
break for people who use large text sizes.  At least before the sidebar was
resizable to accommodate large default text sizes, but if we do it this way it
won't be.

As a final note, I haven't built with this patch, so I'm making the assumption
it's been tested pretty well.

Comment 22

[Mike Connor [:mconnor]]

Attachment: address comments, with -w

Comment 23

[Mike Connor [:mconnor]]

Attachment: same without -w

Comment 24

[Mike Connor [:mconnor]]

please whack on this ASAP, I've tested pretty thoroughly, but I may have missed
one or two things.  I want to land this very very early in beta and get some
testing on it.

Comment 25

[Asaf Romano (gone)]

Comment on attachment 188277 [details] [diff] [review]
address comments, with -w

Works very well.

Index: toolkit/components/help/content/help.js
===================================================================

+//copied from browser.js to handle the searchbar

space here, please.

 function doFind() {
+    if (document.getElementById("help-search-sidebar").hidden)
+      showSearchSidebar();
+

It would be better to do this right after the |!RE| check (which calls
|hideSearchSidebar).



-# toggleSidebarStatus - Toggles the visibility of the sidebar.
-function toggleSidebar()
+function getBoolPref (aPrefname, aDefault)

There is only one call to |getBoolPref|, do we actually need it?

+  try { 
+    var pref = Components.classes["@mozilla.org/preferences-service;1"]
+			.getService(Components.interfaces.nsIPrefBranch);


Wrong indentation.

Index: toolkit/components/help/content/help.xul
===================================================================

	 <key id="key_textZoomEnlarge" key="&textZoomEnlargeCmd.commandkey;"
	     command="cmd_textZoomEnlarge" modifiers="accel"/>
	 <key id="key_textZoomEnlarge2" key="&textZoomEnlargeCmd.commandkey2;"
	     command="cmd_textZoomEnlarge" modifiers="accel"/>
	 <key id="key_textZoomReduce" key="&textZoomReduceCmd.commandkey;"
	     command="cmd_textZoomReduce" modifiers="accel"/>
	 <key id="key_textZoomReset" key="&textZoomResetCmd.commandkey;"
	     oncommand="ZoomManager.prototype.getInstance().reset();"
modifiers="accel"/>
-	 <key id="key_toggleSidebar" keycode="VK_F9"
command="Help:ToggleSidebar"/>
-	 <key id="key_viewNextHelpPanel" keycode="VK_TAB"
-	      oncommand="showRelativePanel(true);" modifiers="control"/>
-	 <key id="key_viewPrevHelpPanel" keycode="VK_TAB"
-	      oncommand="showRelativePanel(false);" modifiers="control,shift"/>
+    <key id="key_focusSearch" key="&helpSearch.commandkey;"
+	  oncommand="focusSearch()" modifiers="accel" />
+

There shoudn't be a space before the />.

-		 <tree id="help-glossary-panel" class="focusring"
+      <vbox id="help-sidebar" persist="width">
+	 <vbox flex="1" id="help-toc-sidebar">
+	   <sidebarheader align="center">
+	     <label id="help-toc-sidebar-header" flex="1" crop="end"
value="&toctab.label;"
+		    accesskey="&toctab.accesskey;" control="help-toc-panel" />

-	     <splitter id="helpsidebar-splitter" collapse="before"
-		     persist="state hidden" state="open">
-	     </splitter>
+      <splitter id="help-sidebar-splitter" collapse="before" />

ditto.


+    <toolbox id="help-toolbox" class="toolbox-top">
+      <toolbar id="HelpToolbar" class="toolbar-primary chromeclass-toolbar">

|toolbar-primary| isn't necessary, only |chromeclass-toolbar|.


Index: toolkit/locales/en-US/chrome/mozapps/help/help.dtd
===================================================================

+<!ENTITY textZoomReduceBtn.label	"Decrease Text Size">
+<!ENTITY textZoomReduceBtn.accesskey	 "D">
+<!ENTITY textZoomEnlargeBtn.label	"Increase Text Size">
+<!ENTITY textZoomEnlargeBtn.accesskey	 "I">
\ No newline at end of file

new line please :)

Comment 26

[Jeff Walden (gone starting June 8)]

Comment on attachment 188277 [details] [diff] [review]
address comments, with -w

Looks good to me.  Now let's get this in so I can attack bug 296012 in the day
I have left before certain doom.

Comment 27

[Caleb]

I've applied the latest version of the patch to my local tree, and the sidebar
seems to be pretty much squashed (need bigger width?).

Beside that, everything else seems to be ok.

Comment 28

[Jeff Walden (gone starting June 8)]

(In reply to comment #27)
> Beside that, everything else seems to be ok.

The sidebar id was previously help-sidebar-box, but it's now help-sidebar, so
all the |#help-sidebar-box| rules need changing.  I tested the changes using the
DOM Inspector, and 15-25em seems to work fine.

Comment 29

[Mike Connor [:mconnor]]

my bad, I noticed that, and fixed in my local tree, but didn't upload the patch,
new patch coming.

Comment 30

[Mike Connor [:mconnor]]

Attachment: latest comments addressed, with -w

Comment 31

[Mike Connor [:mconnor]]

Attachment: without -w

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 188455 [details] [diff] [review]
latest comments addressed, with -w

r=vladimir

Looks fine to me; however, check your tab settings in help.js and help.xul --
looks like you've got some indentation using 8-space tabs in there where the
existing code is using spaces for indentation, so things are lining up
incorrectly.

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 188455 [details] [diff] [review]
latest comments addressed, with -w

I'd like to get this in for a2 since it affects third-party themes, and the
risk is zero outside of Help.

Comment 34

[Mike Connor [:mconnor]]

fixed on trunk, with tabs fixed up to spaces.  Thanks to everyone who prodded
this patch.