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.

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.

(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.

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 :)).

Mike, can I have accesskey mnemonics for the controls that have text labels? So, _S_earch and _C_ontents

(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.)

(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.

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.

(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 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 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.

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 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 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 :)

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

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 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 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.

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 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 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.

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.

(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.

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

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 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.

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