I'm working on bringing the menu reference up to snuff. For items like Tools>Options (Edit>Preferences on Linux, and therein lies the rub), it appears that it's impossible to make only one show up in the table of contents unless the file is preprocessed, which isn't doable because it'd be a localization hurdle. This would also apply to Windows-only entries, like the email items under Tools in Windows, Default Browser on the Mac, etc. I don't know how this is doable, but if it's possible to add a custom attribute to some entries in the ToC to mark them in the same way that CSS can be used to mark elements in the docs themselves as win/noWin/mac/noMac/unix/noUnix, that would work. Unfortunately, it's impossible for 1.0 (I'm not sure exactly what I'll do for now, because CSS hiding would be confusing if the entry doesn't exist). I do, however, think it's an extremely important feature for After Firefox 1.0 to ensure platform conformity. (It should have been done before 1.0, but of course we're 20/20 now.) I won't take this because I don't know enough yet, but I may give it a stab as I learn more about how Help works (which has zero chance of happening to the degree needed anytime before 1.0).

*** Bug 254991 has been marked as a duplicate of this bug. ***

Accepting. I got an idea for this.

(In reply to comment #2) > Accepting. I got an idea for this. Can we fix this for Seamonkey as well?

While I'm thinking about it, Index would also need a fix eventually, because it'll have links to platform-specific stuff like Default Browser.

My idea didn't work out so well :). Neil, do you have any idea on a possible solution? We need this for Seamonkey as well as Firefox (give me one and I'll make the other). I was thinking of making a attribute hack but it didn't work too well.

Moving to new Firefox Help owner.

I don't know enough about RDF and friends to even attempt fixing this for 1.0. I'd bet it'd be a pretty big change, anyways, and I'm not sure I'd want to introduce it into what's supposed to be the first mission-critical Firefox release. It looks bad, sure, but it's not something that really, really matters in the grand scheme of things. Patches still accepted...

I think it could be done along these lines: 1. Each platform has a platform-specific RDF file in the tree. Using jar.mn preprocessing this file is then built into a known location in content. 2. Each platform has a platform-specific RDF file in locale. These are separate files although due to 1. above only one will be used on each platform. 3. Common entries remain in the existing RDF files.

*** Bug 262744 has been marked as a duplicate of this bug. ***

This is a very noticeable issue, and it needs to be fixed. (Docs are also important, but right now I seem to be most productive at working with code instead of docs, so I'm sticking with what's working. Reviews will still happen, but I'm afraid I have a copy-writing block at the moment. Hopefully it'll be gone by the end of the week.) I'll try some hacking at this via the method Neil discussed to see if I can get anything to work. If I can't get anything to work without being forced to make anything more than trivial edits toolkit/components/help/content/help.js, then it probably won't get done without outside help from someone who knows RDF, Mozilla's RDF interfaces, and the workings of this code.

The attached help.js does this at run time by copying the datasources into memory and filtering out the entries that are not for the current platform. It filters the ToC, Search, glossary and index. In the RDF files, add nc:platform="xxx" to any platform-specific entry, where xxx is a space-separated list of case-insensitive tokens. The meaningful tokens are: unix mac os/2 win nounix nomac noos/2 nowin The first token in the list implies the converse for other platforms. For example, unix implies nomac noos/2 nowin, or nounix implies mac os/2 win. Normally specify just one or two tokens to get the effect you need. If you omit nc:platform, then the entry is displayed on all platforms. I produced this by hacking my installed help.js in Firefox, because I don't yet know how to work CVS. Maybe I'll figure it out soon... Meanwhile if someone sends me the source file I'll be happy to make the changes and return it.

I got CVS to work. Here's the same thing as a patch.

(In reply to comment #13) Wow. I've been struggling all weekend to understand 1) RDF and 2) Mozilla's RDF impl and help.js to make some sort of a hack to fix this (xulplanet's pretty decent, it seems, but it's still rather opaque for someone without any formal programming experience), and I was almost at the point of giving up and just pushing through some sort of workaround (at least for Options/Preferences -- not much to do about completely missing entries). Because I don't really know this code I can't actually review it, but I'll probably give it a quick once-over and then forward it on to someone who can. If the patch is deemed of sufficiently high quality, this needs to get in ASAP so that we can meet the Help documentation freeze (which was targeted for the 15th last I heard).

The previous patch, for whatever reason, had no context to it, making it difficult to evaluate without adding it to a source tree to read it. This patch contains context, making it much more understandable by itself.

Comment on attachment 161677 [details] [diff] [review] The previous patch, with 8 lines' context Okay, I've looked at it, and I've mostly got nits. I can't comment much on the code itself, however, because I don't quite understand it. (Oh, in the future when making a diff, use `cvs diff -pu8N mozilla/toolkit/components/help/content/help.js` from the top of the mozilla.org source tree. You'll give context this way, and the patch will be less fragile in general.) >+var platformCode; ... >+ setPlatformCode(); You leave platformCode as an undefined global variable here and define it later in setPlatformCode(), called within init(). While it's possible to get the platform using Javascript, in chrome JS it's not the way I've ever seen used. For what are most likely performance reasons, we use the tree's chrome preprocessor, which lets us use code specific to a certain platform and then only include that code in the built source. Here's how you probably want to define platformCode (insert in place of the line above): #ifdef XP_WIN var platformCode = "win"; #else #ifdef XP_MACOSX var platformCode = "mac"; #else #ifdef XP_OS2 var platformCode = "win"; #else var platformCode = "unix"; #endif #endif #endif The chrome preprocessor works similarly to how the preprocessor works in a C/C++ compiler. In the case above, all of XP_WIN, XP_MACOSX, and XP_OS2 are automatically defined/not defined for the proper target platform for the build. It's also worth noting that the preprocessor strips out the lines that aren't evaluated, so the above will only take one line in compiled source instead of many. >-function loadDatabasesBlocking(datasources) { >+# filter datasources for platform-specific entries, then build tree >+function setFilteredDatabase(datasources, tree) { > var ds = datasources.split(/\s+/); > for (var i=0; i < ds.length; ++i) { You've used (or rather left) a tab in the |var ds| line above. Tabs are technically not allowed in mozilla.org code (its presence here is an anomaly), so it would be nice if you could remove it. You should also correct formatting as you go, too. >+ var filteredDS = Components >+ .classes["@mozilla.org/rdf/datasource;1?name=in-memory-datasource"] >+ .createInstance(Components.interfaces.nsIRDFDataSource); >+ >+ filterNode(datasource, RDF.GetResource("urn:root"), filteredDS); Speaking almost 100% from a gut feeling here, I don't know how much your use of recursion in filterNode() will be liked (it's entirely possible I could be wrong). >+ else { >+ targetDS.Assert(node, arc, tgt, true); >+ // add tocid attribute for getLink() to look up >+ var hashpos = node.Value.indexOf("#"); >+ var tocid = node.Value.substr((hashpos == -1)? 0 : hashpos + 1); >+ targetDS.Assert( >+ node, >+ NC_TOCID, >+ RDF.GetLiteral(tocid), >+ true); >+ // recurse down tree >+ filterNode(sourceDS, tgt, targetDS); >+ } When you set tocid, you use node.Value.substr() with only one argument. This may or may not work properly (whatever properly is, as I couldn't really understand the code -- but don't forget I didn't understand the code to start), but it is definitely wrong. You need to use substr() with both arguments. >+# set global plaformCode used by filterNode() >+function setPlatformCode() { >+ var plat = window.navigator.platform.toLowerCase(); >+ if (plat.indexOf("mac") == 0) >+ platformCode = "mac"; >+ else if (plat.indexOf("os/2") == 0) >+ platformCode = "os/2"; >+ else if (plat.indexOf("win") == 0) >+ platformCode = "win"; >+ else platformCode = "unix"; > } I notice that you define a platformCode="os/2" here. Because OS/2 isn't officially supported, there's pretty much no code that is OS/2-specific within toolkit or browser. In fact, the one place in Help that's used to create some platform-specific code specifically diverts OS/2 into the same subset as Windows. (That's probably an error as standalone #ifdef XP_WINs appear pretty frequently through source, but OS/2 behavior should be expected to be erratic as long as no one working on Help tests it.) Anyways, we basically shouldn't do any OS/2 filtering, and the preprocessor code I give above will properly filter OS/2 in with Windows. Aside from those comments, I don't have anything else to say on the rest of the patch. It /looks/ like it works as desired, even if in a small test it didn't work (my syntax for defining the platform-ness of a node in firebirdhelp.rdf could easily have been wrong). Neil is much more familiar with how the RDF code in Help works, so I'll forward this on to him for a real review.

Thanks for all your comments. It will be obvious how unfamiliar I am with Mozilla coding! A couple of remarks make me think it would be worthwhile explaining the approach I took. When the existing code loads the ToC or any other panel, loadHelpRDF() gets a list of RDF files from (say) firefoxhelp.rdf, calls loadDatabasesBlocking() to preload them, and sets the tree's datasources. This automatically makes the tree rebuild. The aggregated datasources are available in the tree's database property. To open a particular help topic, getLink() looks up its ID in the aggregated ToC. Unfortunately the ID has file scope--it is tied to the file that it originally came from. This is a fault in the original design. To work around it, getLink() finds the names of the original files and tries each of them in turn. My approach was to replace loadDatabasesBlocking() with setFilteredDatabase(). This preloads the RDFs as before, but calls filterNode() to filter out any unwanted platform-specific parts, storing the result in a temporary datasource in memory. Then it adds the temporary datasource to the tree's database. By doing it this way, the tree has to be rebuilt explicitly. filterNode() is recursive because it walks the tree structure of the ToC. An expert might be able to copy and filter the datasource without walking the tree, just by copying the arcs individually, but I could not make it work that way. Now getLink() cannot work around the ID problem in the same way as before. It cannot get the original RDF file names from the tree, because the tree got its data from my filtered in-memory copies and knows nothing about the files. So I worked around it in a different way, making filterNode copy each ID (without the file name) into a tocid attribute. This simplifies getLink(). substr with one argument goes to the end of the string, as in ECMA-262. It is not hard to find other examples of this usage in Mozilla code. I have a personal preference for platform-independent code that determines the platform at run time. This is because I want to be able to put a help viewer in a XPI that will work on any platform. But I understand that there may be good reasons to use the preprocessor instead. I can easily work around it.

No way now this'll get in for 1.0. Pushing off the list...

Is this bug branch-only? Should it be moved to Trunk?

Comment on attachment 161677 [details] [diff] [review] The previous patch, with 8 lines' context Rerequesting review, er first-review, after the component move, for Jeff.

Targetting bugs which were targetted to Firefox1.1 before the move to mozilla1.8beta2.

Naturally I'm working on Suite help but I expect the port to be trivial.

Comment on attachment 178742 [details] [diff] [review] First Draft Actually, I like this quite much... Especially, how mozillahelp.rdf goes to content and leaves only a dtd behind... now if we could do that for help-toc and help-win as well, I'd be even happier... but I guess that's a different bug :)

Comment on attachment 178742 [details] [diff] [review] First Draft If we fix the problem this way, we fix it for ourselves, but we don't fix it in a way that will let extensions use it. Contrast this with the original patch, which would have been usable by extensions. Is this loss something we should be concerned about? I'm inclined to think it is, but I'm not familiar enough with external uses of the Help Viewer to be able to argue this point. One advantage to the first method, however, is that it wouldn't require creating a bunch of new files in source code to make things work properly. The second method requires simultaneous checkin of all the platform-specific code, which will be slower (for Firefox Help, at least, if not Suite Help) than committing a fix and making trivial attribute changes in the appropriate RDF files at a later time.

Comment on attachment 178742 [details] [diff] [review] First Draft >Index: content/mozillahelp.rdf >=================================================================== >RCS file: content/mozillahelp.rdf >diff -N content/mozillahelp.rdf >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ content/mozillahelp.rdf 27 Mar 2005 22:03:46 -0000 >@@ -0,0 +1,38 @@ >+<?xml version="1.0"?> >+ >+<!DOCTYPE rdf:RDF [ >+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" > >+%brandDTD; >+<!ENTITY % helpDTD SYSTEM "chrome://help/locale/mozillahelp.dtd" > >+%helpDTD; >+]> >+ >+<rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" >+ xmlns:nc="http://home.netscape.com/NC-rdf#"> >+ >+<!-- MASTER HELP DOCUMENT --> >+ <rdf:Description about="urn:root" >+ nc:title="&title;" >+ nc:defaulttopic="welcome" >+ nc:base="chrome://help/locale/"> >+ <nc:panellist> >+ <rdf:Seq> >+ <rdf:li> <rdf:Description nc:panelid="glossary" nc:datasources="help-glossary.rdf"/> </rdf:li> >+#ifdef XP_WIN >+ <rdf:li> <rdf:Description nc:panelid="index" nc:datasources="help-indexAZ.rdf help-index1.rdf"/> </rdf:li> >+ <rdf:li> <rdf:Description nc:panelid="toc" nc:datasources="help-toc.rdf"/> </rdf:li> >+#else >+ <rdf:li> <rdf:Description nc:panelid="index" nc:datasources="help-indexAZ.rdf help-index1.rdf help-win.rdf"/> </rdf:li> >+ <rdf:li> <rdf:Description nc:panelid="toc" nc:datasources="help-toc.rdf help-win.rdf"/> </rdf:li> >+#endif Shouldn't help-win.rdf be in the #ifdef XP_WIN part?

(In reply to comment #24) The patch does not require a simultaneous checkin of the platform-specific files. The mozillahelp.rdf can be moved to content as the first step. Only when someone actually moves a contents or index entry might they need to create a new file. Note that extensions can adapt my idea by using script rather than preprocessing to choose between master help files. Although, you've got me thinking... maybe the master help file should contain RDF lists of toc and index files rather than space-separated values...

(In reply to comment #26) > Note that extensions can adapt my idea by using script rather than > preprocessing to choose between master help files. Scripting is still to a degree hacking around it, and IMHO Help should simply dynamically adapt without the programmer having to think about it. Here's another idea along the lines of what you're proposing that wouldn't require quite as much work for extension writers: add an optional attribute to each panel definition within help.rdf that would contain one (or more) OS string(s). Then, add datasources if no such attribute exists (i.e., cross-platform datasource) or the attribute equals (or contains) the appropriate OS string. Even for someone like me who's unfamiliar with RDF, this wouldn't be at all difficult. (Right now I'm feeling like I might even hack this into a patch tomorrow if the difficulty isn't too much for me.) > Although, you've got me thinking... maybe the master help file should contain > RDF lists of toc and index files rather than space-separated values... There are a few existing users who might not like this if the old way didn't still work, but I personally couldn't care. What we have may not be as RDF-like as it could be, but it's a nitpicky point (and probably another bug).

So, using the same help-win.rdf as before, but now (as yet unwritten) help.js code tries urn:<platform> before url:<root> ?

I mean urn:root of course.

No, more like this. Instead of having a whole bunch of definition duplication, just add the attribute to the datasource definition for each help panel. If the panel's platform attribute is empty, non-existent, or contains the appropriate platform string, add it to the datasources -- otherwise, move on to the next panel definition. The logic's simple to implement, and the results are about as expected in my Linux build. (I haven't yet tested whether panels are properly hidden when they're specified for another platform, but I don't expect surprises.) With the patch above applied, some of the expected changes occur as expected. The totally new top-level RDF node "foo-toplevel" is displayed in the table of contents after all the previous datasources' nodes. The node "foobar" is added at the end in the correct place, too. The small problem comes with the redefinition of "prefs" and "prefs-general". I'd hoped these would overwrite the originals, but apparently they don't -- new items were added instead of old items being overwritten. This is probably just an area of RDF that I haven't read about yet (I hope). As a worst-case scenario, we either define the relevant elements twice or add in an sharedRDF.dtd containing duplications, which would get around the problem. This also ignores the problem of ordering. It appears the composite is created such that nodes added later are automatically pushed to the end of the list, which isn't optimal, as order often *does* matter (for example, preference panel items, which would be in the order of the panels themselves). This can be worked around by expanding the datasource into more and more RDF files as necessary, but it's still not optimal. Anyway, there's my solution, more or less as it would be added. I don't know RDF or the RDF API well enough to get very far with the above questions, so I'll leave this to you now.

Comment on attachment 179521 [details] [diff] [review] More like this then? (untested) Yeah, pretty much, but at least in the toolkit patch we're probably going to use #ifdefs because it's how we usually handle such things.

Comment on attachment 179588 [details] [diff] [review] Completed patch >+var emptySearchText = "No search items found."; Shouldn't this be localized? >+var emptySearchLink = "about:blank"; > var helpTocPanel; > var helpIndexPanel; > var helpGlossaryPanel; >@@ -59,6 +59,7 @@ > const RDF_ROOT = RDF.GetResource("urn:root"); > const NC_PANELLIST = RDF.GetResource(NC + "panellist"); > const NC_PANELID = RDF.GetResource(NC + "panelid"); >+const NC_PLATFORM = RDF.GetResource(NC + "platform"); > const NC_EMPTY_SEARCH_TEXT = RDF.GetResource(NC + "emptysearchtext"); > const NC_EMPTY_SEARCH_LINK = RDF.GetResource(NC + "emptysearchlink"); > const NC_DATASOURCES = RDF.GetResource(NC + "datasources"); >@@ -87,6 +88,9 @@ > var defaultTopic = "welcome"; > var searchDatasources = "rdf:null"; > var searchDS = null; >+var platform = /mac/i.test(navigator.platform) ? "mac" : >+ /win/i.test(navigator.platform) ? "win" : >+ /os\/2/i.test(navigator.platform) ? "os/2" : "unix"; > > const NSRESULT_RDF_SYNTAX_ERROR = 0x804e03f7; > >@@ -212,24 +239,27 @@ function loadHelpRDF() { > var panelDefs = helpFileDS.GetTarget(RDF_ROOT, NC_PANELLIST, true); > RDFContainer.Init(helpFileDS, panelDefs); > var iterator = RDFContainer.GetElements(); >- while (iterator.hasMoreElements()) { >+ while (iterator.hasMoreElements()) { > var panelDef = iterator.getNext(); >- var panelID = getAttribute(helpFileDS, panelDef, NC_PANELID, null); >+ if (getAttribute(helpFileDS, panelDef, NC_PLATFORM, platform) != platform) >+ continue; // ignore datasources for other platforms. > >- var datasources = getAttribute(helpFileDS, panelDef, NC_DATASOURCES, "rdf:none"); >+ var panelID = getAttribute(helpFileDS, panelDef, NC_PANELID, null); >+ >+ var datasources = getAttribute(helpFileDS, panelDef, NC_DATASOURCES, "rdf:null"); > datasources = normalizeLinks(helpBaseURI, datasources); > // cache additional datsources to augment search datasources. > if (panelID == "search") { >- emptySearchText = getAttribute(helpFileDS, panelDef, NC_EMPTY_SEARCH_TEXT, "No search items found."); >- emptySearchLink = getAttribute(helpFileDS, panelDef, NC_EMPTY_SEARCH_LINK, "about:blank"); >- searchDatasources = datasources; >- datasources = "rdf:null"; // but don't try to display them yet! >+ emptySearchText = getAttribute(helpFileDS, panelDef, NC_EMPTY_SEARCH_TEXT, emptySearchText); >+ emptySearchLink = getAttribute(helpFileDS, panelDef, NC_EMPTY_SEARCH_LINK, emptySearchLink); >+ searchDatasources += " " + datasources; >+ continue; // but don't try to display them yet! > } > > // cache toc datasources for use by ID lookup. > var tree = document.getElementById("help-" + panelID + "-panel"); > loadDatabasesBlocking(datasources); >- tree.setAttribute("datasources", datasources); >+ tree.setAttribute("datasources", tree.getAttribute("datasources") + " " + datasources); > } > } > catch (e) { >Index: content/help.xul >=================================================================== >RCS file: /cvsroot/mozilla/extensions/help/resources/content/help.xul,v >retrieving revision 1.57 >diff -u -d -r1.57 help.xul >--- content/help.xul 19 Feb 2005 00:50:40 -0000 1.57 >+++ content/help.xul 27 Mar 2005 22:03:46 -0000 >@@ -155,8 +155,7 @@ > </template> > <treecols> > <treecol id="GlossaryNameColumn" flex="1" >- hideheader="true" >- primary="true"/> >+ hideheader="true"/> > </treecols> > </tree> > >@@ -248,7 +247,7 @@ > > <treecols> > <treecol id="ResultsColumn" flex="1" >- hideheader="true" primary="true" >+ hideheader="true" > sortActive="true" > sortDirection="ascending" > sort="?name"/> >Index: locale/en-US/help-index1.rdf >=================================================================== >RCS file: /cvsroot/mozilla/extensions/help/resources/locale/en-US/help-index1.rdf,v >retrieving revision 1.42 >diff -u -d -r1.42 help-index1.rdf >--- locale/en-US/help-index1.rdf 6 Feb 2005 21:37:16 -0000 1.42 >+++ locale/en-US/help-index1.rdf 27 Mar 2005 22:03:49 -0000 >@@ -1828,16 +1828,6 @@ > </nc:subheadings> > </rdf:Description> > >-<rdf:Description about="help-indexAZ.rdf#q"> >- <nc:subheadings> >- <rdf:Seq><rdf:li> >- <rdf:Description ID="Quick_Launch" >- nc:name="Quick Launch" >- nc:link="nav_help.xhtml#using_quick_launch"/> >- </rdf:li></rdf:Seq> >- </nc:subheadings> >-</rdf:Description> >- > <rdf:Description about="help-indexAZ.rdf#r"> > <nc:subheadings> > <rdf:Seq><rdf:li> >@@ -1934,11 +1924,6 @@ > <rdf:Description ID="SSL" > nc:name="SSL" > nc:link="ssl_help.xhtml#edit_ciphers"/> >- </rdf:li> >- <rdf:li> >- <rdf:Description ID="System" >- nc:name="System Preferences" >- nc:link="cs_nav_prefs_advanced.xhtml#system"/> > </rdf:li></rdf:Seq> > </nc:subheadings> > </rdf:Description> >Index: locale/en-US/help-toc.rdf >=================================================================== >RCS file: /cvsroot/mozilla/extensions/help/resources/locale/en-US/help-toc.rdf,v >retrieving revision 1.73 >diff -u -d -r1.73 help-toc.rdf >--- locale/en-US/help-toc.rdf 25 Mar 2005 23:09:59 -0000 1.73 >+++ locale/en-US/help-toc.rdf 27 Mar 2005 22:03:52 -0000 >@@ -131,7 +131,6 @@ > <rdf:li> <rdf:Description ID="nav-doc-cache" nc:name="Changing Cache Settings" nc:link="nav_help.xhtml#changing_cache_settings"/> </rdf:li> > <rdf:li> <rdf:Description ID="nav-doc-smartup" nc:name="Getting the Latest Software Automatically" nc:link="nav_help.xhtml#getting_the_latest_software_automatically"/> </rdf:li> > <rdf:li> <rdf:Description ID="nav-doc-mousewheel" nc:name="Using a Mouse Wheel" nc:link="nav_help.xhtml#using_a_mouse_wheel"/> </rdf:li> >- <rdf:li> <rdf:Description ID="nav-doc-quicklaunch" nc:name="Using Quick Launch" nc:link="nav_help.xhtml#using_quick_launch"/> </rdf:li> > </rdf:Seq> > </nc:subheadings> > </rdf:Description> >@@ -804,7 +803,6 @@ > <rdf:li><rdf:Description ID="advanced_pref_installation" nc:name="Software Installation" nc:link="cs_nav_prefs_advanced.xhtml#software_installation"/> </rdf:li> > <rdf:li><rdf:Description ID="advanced_pref_mouse_wheel" nc:name="Mouse Wheel" nc:link="cs_nav_prefs_advanced.xhtml#mouse_wheel"/> </rdf:li> > <rdf:li><rdf:Description ID="advanced_pref_dom_inspector" nc:name="DOM Inspector" nc:link="cs_nav_prefs_advanced.xhtml#dom_inspector"/> </rdf:li> >- <rdf:li><rdf:Description ID="advanced_pref_system" nc:name="System" nc:link="cs_nav_prefs_advanced.xhtml#system"/> </rdf:li> > </rdf:Seq> > </nc:subheadings> > </rdf:Description> >Index: locale/en-US/help-win.rdf >=================================================================== >RCS file: locale/en-US/help-win.rdf >diff -N locale/en-US/help-win.rdf >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ locale/en-US/help-win.rdf 27 Mar 2005 22:03:52 -0000 >@@ -0,0 +1,41 @@ >+<?xml version="1.0"?> >+<rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" >+ xmlns:nc="http://home.netscape.com/NC-rdf#"> >+ >+ <rdf:Description about="help-toc.rdf#nav-doc-ses"> >+ <nc:subheadings> >+ <rdf:Seq> >+ <rdf:li> <rdf:Description ID="nav-doc-quicklaunch" nc:name="Using Quick Launch" nc:link="nav_help.xhtml#using_quick_launch"/> </rdf:li> >+ </rdf:Seq> >+ </nc:subheadings> >+ </rdf:Description> >+ >+ <rdf:Description about="help-toc.rdf#advanced_pref_advanced"> >+ <nc:subheadings> >+ <rdf:Seq> >+ <rdf:li><rdf:Description ID="advanced_pref_system" nc:name="System" nc:link="cs_nav_prefs_advanced.xhtml#system"/> </rdf:li> >+ </rdf:Seq> >+ </nc:subheadings> >+ </rdf:Description> >+ >+<rdf:Description about="help-indexAZ.rdf#q"> >+ <nc:subheadings> >+ <rdf:Seq><rdf:li> >+ <rdf:Description ID="Quick_Launch" >+ nc:name="Quick Launch" >+ nc:link="nav_help.xhtml#using_quick_launch"/> >+ </rdf:li></rdf:Seq> >+ </nc:subheadings> >+</rdf:Description> >+ >+<rdf:Description about="help-indexAZ.rdf#s"> >+ <nc:subheadings> >+ <rdf:Seq><rdf:li> >+ <rdf:Description ID="System" >+ nc:name="System Preferences" >+ nc:link="cs_nav_prefs_advanced.xhtml#system"/> >+ </rdf:li></rdf:Seq> >+ </nc:subheadings> >+</rdf:Description> >+ >+</rdf:RDF>

It's pretty much the same as the above patch with localization of the two search strings added in.

Comment on attachment 179639 [details] [diff] [review] toolkit version of patch This may end up waiting until after the tree closes, but it's simple enough I guess it can live with a little extra hassle if necessary.

(In reply to comment #34) >(From update of attachment 179588 [details] [diff] [review]) >>+var emptySearchText = "No search items found."; >Shouldn't this be localized? It is, in mozillahelp.rdf

Comment on attachment 179639 [details] [diff] [review] toolkit version of patch >+# Strings >+var bundle = document.getElementById("bundle_help"); >+var emptySearchText = bundle.getString("emptySearchText"); >+var emptySearchLink = bundle.getString("emptySearchLink"); IIRC you have to load strings from your onload function as the XBL won't work until then, assuming that you're actually able to retrieve the DOM element at this point, which I doubt, thus the r-, unless someone like bryner says that this is safe. > emptySearchText = getAttribute(helpFileDS, panelDef, >+ NC_EMPTY_SEARCH_TEXT, null) || emptySearchText; > emptySearchLink = getAttribute(helpFileDS, panelDef, >+ NC_EMPTY_SEARCH_LINK, null) || emptySearchLink; Nit: nice though the || operator is, the fourth parameter to getAttribute should be the default value anyway. >+emptySearchLink=about:blank I'm not quite sure why you felt it necessary to localize about:blank :-P

(In reply to comment #38) > IIRC you have to load strings from your onload function as the XBL won't work > until then Fixed and tested. > Nit: nice though the || operator is, the fourth parameter to getAttribute > should be the default value anyway. Yes, true, and also fixed. > I'm not quite sure why you felt it necessary to localize about:blank :-P Me neither. I was just in a rush and wasn't thinking properly. I also moved Help's string bundle out to be a global variable instead of having it be manually queried any time a string from it was needed.

Comment on attachment 179804 [details] [diff] [review] Updated to address review Having parity with SeaMonkey's Help would be a Good Thing(TM). This will also be useful for Firefox Help and the calendarhelp project on mozdev, as I assume they'll port the fix over into their mirror of the code when they have time.

Comment on attachment 179804 [details] [diff] [review] Updated to address review a=asa

Fix checked in.

Comment on attachment 191513 [details] [diff] [review] Missing link Trivial suite help fix accidentally omitted from attachment 179588 [details] [diff] [review].

Comment on attachment 191513 [details] [diff] [review] Missing link Neil, can you check this in please?