423486 - Remove help viewer from Firefox and point to SUMO

Status: VERIFIED FIXED

(Firefox Graveyard :: Help Documentation, defect, P1)

Description

[:Gavin Sharp [email: [e-mail]]]

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

This is part of a shift from packaging in-product help with the binary and moving to online help in SUMO. Needs to be done for beta 5.

Comment 2

[Benjamin Smedberg]

Discussed with mconnor on IRC: I think we should leave the help viewer code in the toolkit for 1.9, since other apps such as Thunderbird/SeaMonkey/? may be depending on it, and we're really late in the cycle. Is there any reason we shouldn't do that?

Comment 3

[Robert Kaiser]

Yes, I really hope the help viewer stays in toolkit for other applications to use - but if we know early enough beforehand, we probably could do separate, forked implementations in SeaMonkey and Thunderbird in time. I'd prefer to have the toolkit one around though for 1.9 at least.

Comment 4

[Robert Kaiser]

Oh, actually, Thunderbird has no in-product help, they just link to http://www.mozilla.org/support/thunderbird/ from the menu item.
I wonder if anyone except SeaMonkey is actually using the toolkit help viewer right now - what about Sunbird, Songbird, Miro, etc.?

Comment 5

[Mike Connor [:mconnor]]

To be clear, this would be a build switch, not a cvs removal. I don't think the help viewer as it stands is really that useful, being RDF-based and having no automated indexing facility, etc.  Seems like if we wanted to support a local help system we should resurrect the idea of compiling help docs into some sort of native format and use the OS native help systems, rather than some homespun RDF+HTML pain.  Not sure its worth it, but that seems massively better than the current version.

SeaMonkey already doesn't use stock toolkit build options, in the absense of other known consumers I don't see why they can't just add an extra build option.

FWIW, Miro just has a web link, Songbird uses the built in browser, and Sunbird doesn't have any help files in CVS, so pretty sure they're not using it.  I don't believe anyone would really want to use the current setup if they were starting from scratch.  Not sure why SeaMonkey would keep it either, FWIW!

Anyway, will fix the Firefox bits for now, we can revisit later once we can check around for other possible consumers.

Comment 6

[Robert Kaiser]

OK, Mike, thanks for your input on this, and good to know SeaMonkey is probably the only user, so we should go for our own implementation of in-product help probably. We really want to have help available offline, so we feel we need the stuff in the product, and this basically has worked fine so far.
True, there are enough hacks there, and RDF ugliness, and if it would be reimplemented now, it probably would be done differently.
The reason why we're using it in SeaMonkey is that toolkit only provides us this in-product option, and we already had basically the same working in the xpfe code we inherited from Mozilla suite.

Comment 8

[Mike Connor [:mconnor]]

Attachment: WIP, needs a bit of testing

This patch: 

* Removes the ugly dependency/copied code in preferences.xml, consumers can set their own ondialoghelp callers
* Does some misc fixup to remove the helpURI attrs from Firefox's prefwindow
* adds a helper function for opening Help links (openHelpLink)
* Makes the help key binding in page info actually do the right thing
* Hooks up the bits in baseMenuOverlay to call the new method

still to come:

* stop building/packaging local help content
* figure out what to do with the help viewer code in toolkit
* figure out what to do with connection support docs

Comment 9

[Jesse Ruderman]

What about help with connecting to the web, setting up proxies, and such?  Will users still be able to access that kind of help without being online?  (Although now that I look at the contents of the Help window, it doesn't seem to have much troubleshooting information anyway.)

Comment 10

[Steffen Wilberg]

Comment on attachment 310405 [details] [diff] [review]
WIP, needs a bit of testing

>Index: browser/components/preferences/advanced.xul
>===================================================================
>-  <prefpane id="paneAdvanced" onpaneload="gAdvancedPane.init();"
>-            helpURI="chrome://browser/locale/help/help.rdf">
>+  <prefpane id="paneAdvanced" onpaneload="gAdvancedPane.init();">

>Index: toolkit/content/widgets/preferences.xml
>===================================================================
>           var helpButton = document.documentElement.getButton("help");
>-          if (aPaneElement.hasAttribute("helpURI"))
>+          if (aPaneElement.hasAttribute("helpTopic"))
>             helpButton.hidden = false;
>           else
>             helpButton.hidden = true;

This will hide the Help button on the Advanced pane because that doesn't have a helpTopic itself. Instead, the helpTopic is specified for the <tab>s, so that clicking Help on e.g. the Encryption tab takes you right to the "prefs-advanced-encryption" topic.

Comment 11

[Steffen Wilberg]

Comment on attachment 310405 [details] [diff] [review]
WIP, needs a bit of testing

>Index: browser/components/preferences/preferences.xul
>===================================================================
>+    <script type="application/javascript">
>+      function openPrefsHelp() {
>+        var helpTopic = document.getElementById("BrowserPreferences").currentPane.helpTopic;
>+        openHelpLink(helpTopic);
>+      }
>+    </script>

>Index: toolkit/content/widgets/preferences.xml
>===================================================================
>-      <handler event="dialoghelp">
>-        // currentPane is set in _selectPane for prefwindows which aren't childs.
>-        // So if there's no currentPane, just use the first pane.
>-        var pane = this.currentPane ? this.currentPane : this.preferencePanes[0];
>-        this.openHelp(pane.helpTopic, pane.getAttribute("helpURI"));
>-      </handler>

Although I couldn't test it yet, I think this will break child prefwindows like Fonts, Colors, Languages etc.
I'd suggest to follow my old code from preferences.xml to select the first pane if there's no currentPane.

Comment 12

[Robert Kaiser]

I think we either shouldn't mess with the functionality to call help from toolkit's preferences.xml or SeaMonkey needs to implement it in its extended version of that binding. CCing Mnyromyr, who did the work on the extended binding for SeaMonkey.

Comment 13

[Mike Connor [:mconnor]]

Given that all of the major embeddors are doing something different, it doesn't make sense for the <prefwindow> binding to be tied to any single help implementation.  Its trivial to add an ondialoghelp handler to your own prefwindow, and you can reuse contextHelp.js's methods instead of using copied code that needs to stay in sync.

Comment 14

[Mike Connor [:mconnor]]

Steffen:

good catch on the hasAttr check, just checking that the property returns something now.

The child prefwindows are broken because there isn't any ondialoghelp method specified,fixing that.

Comment 15

[Robert Kaiser]

Mike:
Sure, this is probably something that should move into the SeaMonkey implementation (we already extend the toolkit binding for supporting our suite-style prefwindow appearance on top of it), I just wanted to make sure that our code doesn't break and wondered that a patch for such a change in toolkit's behavior would only come up so late in the toolkit cycle, I expected all toolkit-side patches for the FF switch to web-based help would have been prepared for a long time, as the decision to do this for FF3 is pretty old now.
Still, even if it means fixing up SeaMonkey code for unexpected toolkit changes once again, I'm pretty sure we're up to cope with it, just needed the right people to be CCed here so they can react to the changes.

Comment 16

[Reed Loden [:reed]]

Comment on attachment 310405 [details] [diff] [review]
WIP, needs a bit of testing

>+pref("app.support.baseURL", "http://support.mozilla.com/1/%APP%/%MAJORMINORVERSION%/%OS%/%LOCALE%/");
...
>+    MAJORMINORVERSION: function() this.appInfo.version.substr(0,3),

This seems like an unneeded limit, since it would hurt us if we needed to show differing help on a dot release. Seems like SUMO should be handling the 3.0b5 -> 3.0 conversion instead of the app, imho.

Comment 17

[Mike Connor [:mconnor]]

If we change from foo to bar in 3.0.x, we should update help docs to match current.  Older point releases are not supported, nor should they be.

Comment 18

[David Tenser [:djst]]

I agree. SUMO currently supports 2.0.x and will support 3.0.x, where x means "latest point release."

Comment 19

[Mike Connor [:mconnor]]

Attachment: more...

updates in this patch
* fixes advanced tab help button (thanks steffen!)
* adds the removed handlers to suite's prefwindow binding
** this could be modified to not add helpURI attrs all over the place and simplify the code, the generic binding couldn't be like that
* removes help viewer from default build for toolkit
* adds help viewer to default build for xulrunner and suite

does not fix:
* child prefwindows (working on it)
* network troubleshooting (we really didn't have any info on this before though, so it shouldn't block, but the right way to help those users is IMO to build a troubleshooting wizard, not try to expand the help docs)

Comment 20

[Robert Kaiser]

(In reply to comment #19)
> * adds the removed handlers to suite's prefwindow binding

This seems to work fine, in my testing here, SeaMonkey help buttons in the new toolkit-based prefwindow still call the correct help entries.

Thanks for including this part in the patch!

Comment 21

[Mike Connor [:mconnor]]

Attachment: more...

changes in this rev:

* removes the xulrunner change and just leaves it on for seamonkey
* fixes child windows and cleans up the openPrefsHelp impl to handle modal vs. non-modal prefwindows
* implements steffen's fallback if currentPane is null by making currentPane a property that handles this properly

Comment 22

[Mike Connor [:mconnor]]

Attachment: for review

* Fixes the #ifdef to use the right define in browser/locales/jar.mn

has moa=bsmedberg on IRC for disabling the help viewer by default, kairo's already tested the earlier rev of the patch to ensure seamonkey still works, so I think we just need gavin to review this.

Need a litmus test to ensure that all of the help interactions listed here work and show appropriate content:
* All main prefpanes
* All tabs under Advanced
* Child prefwindows
** Advanced Javascript prefwindow
** advanced fonts
** colors
** languages
** clear private data prefs
** connections window
* F1 in Page Info
* Help Menu
** <appname> Help
** For Internet Explorer Users (only on Windows)

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

I can't get this patch to apply cleanly to a fresh tree :(

Comment 24

[Mike Connor [:mconnor]]

Attachment: unbitrotted

beltzner: try server builds are coming, this applies cleanly.  had to upload the patch without the /suite bits for tryserver to work.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311099 [details] [diff] [review]
unbitrotted

There are a couple of clunky interactions here that I think could be helped by having a special "help" window which is always opened to service help requests (as opposed to having three F1 keypresses open three separate tabs, or having help sometimes open new windows as a way of getting around modal dialogs) but no showstoppers.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 311099 [details] [diff] [review]
unbitrotted

>Index: toolkit/components/urlformatter/src/nsURLFormatter.js

>+    VERSION:           function() this.appInfo.version,
>+    MAJORMINORVERSION: function() this.appInfo.version.substr(0,3),

This seems fragile. Can we just use "version" and have sumo interpret it correctly? (Even just moving this "substr" hack to sumo would be better, since it's easier to adjust SUMO after-the-fact.)

>Index: toolkit/content/widgets/preferences.xml

>+      <property name="currentPane"
>+                onset="this._currentPane = val;return val;">

nit: "return this._currentPane = val;"

>Index: browser/base/content/utilityOverlay.js

>+// aCalledFromModal is optional
>+function openHelpLink(aHelpTopic, aCalledFromModal) {
>+  var url = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
>+                      .getService(Components.interfaces.nsIURLFormatter)
>+                      .formatURLPref("app.support.baseURL");
>+  url += aHelpTopic;
>+
>+  var where = aCalledFromModal ? "window" : "tab";
>+  openUILinkIn(url, where);

Will this work if there are no open browser windows, cross platform?

>+function openPrefsHelp() {
>+  var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                        .getService(Components.interfaces.nsIPrefBranch2);
>+  var instantApply = prefs.getBoolPref("browser.preferences.instantApply");
>+
>+  var helpTopic = document.getElementsByTagName("prefwindow")[0].currentPane.helpTopic;
>+  openHelpLink(helpTopic, !instantApply);

Would be nice if the link between "modal dialog" and "instant apply enabled" was clearer, but maybe that's just something you have to know - though it looks like you can get a non-modal pref dialog regardless of instantApply on Windows, if you use |firefox.exe -preferences|? I'm away from my windows boxes, can't test right now. Perhaps this should also check whether the window has a parent?

>Index: browser/base/content/pageinfo/pageInfo.xul

> #ifdef XP_MACOSX
>     <key key="."                 modifiers="meta"  command="cmd_close"/>
>-#endif
>+    <key id="key_openHelpMac"
>+         command="cmd_help;"
>+         key="&openHelpMac.commandkey;"
>+         modifiers="&openHelpMac.modifiers;"/>
>+    <key id="key_openHelpMacFrontend"
>+         key="&openHelpMac.frontendCommandkey;"
>+         modifiers="&openHelpMac.frontendModifiers;"/>

Aren't these redundant given that they already exist due to baseMenuOverlay.xul (applied to pageInfo.xul via macBrowserOverlay)?

Looks like you forgot to fix advanced-scripts.xul (remove helpURI, add reference to utilityOverlay and call to openPrefsHelp()).

>Index: suite/common/bindings/prefwindow.xml

>+      <handler event="dialoghelp">
>+      <![CDATA[
>+        // currentPane is set in _selectPane for prefwindows which aren't childs.
>+        // So if there's no currentPane, just use the first pane.
>+        var pane = this.currentPane ? this.currentPane : this.preferencePanes[0];

The new currentPane property that you're adding on the base binding means this can just use .currentPane, right?

Comment 27

[Mike Connor [:mconnor]]

(In reply to comment #26)
> (From update of attachment 311099 [details] [diff] [review])
> >Index: toolkit/components/urlformatter/src/nsURLFormatter.js
> 
> >+    VERSION:           function() this.appInfo.version,
> >+    MAJORMINORVERSION: function() this.appInfo.version.substr(0,3),
> 
> This seems fragile. Can we just use "version" and have sumo interpret it
> correctly? (Even just moving this "substr" hack to sumo would be better, since
> it's easier to adjust SUMO after-the-fact.)

fixing SUMO in bug 424714 to handle non-numeric versions,removing this

> >Index: browser/base/content/utilityOverlay.js
> 
> >+// aCalledFromModal is optional
> >+function openHelpLink(aHelpTopic, aCalledFromModal) {
> >+  var url = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
> >+                      .getService(Components.interfaces.nsIURLFormatter)
> >+                      .formatURLPref("app.support.baseURL");
> >+  url += aHelpTopic;
> >+
> >+  var where = aCalledFromModal ? "window" : "tab";
> >+  openUILinkIn(url, where);
> 
> Will this work if there are no open browser windows, cross platform?

yes, openUILinkIn handles no open windows and opens in a new link instead.  Its
what we use for a lot of our UI, i.e. release notes.

> >+function openPrefsHelp() {
> >+  var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> >+                        .getService(Components.interfaces.nsIPrefBranch2);
> >+  var instantApply = prefs.getBoolPref("browser.preferences.instantApply");
> >+
> >+  var helpTopic = document.getElementsByTagName("prefwindow")[0].currentPane.helpTopic;
> >+  openHelpLink(helpTopic, !instantApply);
> 
> Would be nice if the link between "modal dialog" and "instant apply enabled"
> was clearer, but maybe that's just something you have to know - though it looks
> like you can get a non-modal pref dialog regardless of instantApply on Windows,
> if you use |firefox.exe -preferences|? I'm away from my windows boxes, can't
> test right now. Perhaps this should also check whether the window has a parent?

Yeah, added a check for window.opener and a comment explaining this.

> >Index: browser/base/content/pageinfo/pageInfo.xul
> 
> > #ifdef XP_MACOSX
> >     <key key="."                 modifiers="meta"  command="cmd_close"/>
> >-#endif
> >+    <key id="key_openHelpMac"
> >+         command="cmd_help;"
> >+         key="&openHelpMac.commandkey;"
> >+         modifiers="&openHelpMac.modifiers;"/>
> >+    <key id="key_openHelpMacFrontend"
> >+         key="&openHelpMac.frontendCommandkey;"
> >+         modifiers="&openHelpMac.frontendModifiers;"/>
> 
> Aren't these redundant given that they already exist due to baseMenuOverlay.xul
> (applied to pageInfo.xul via macBrowserOverlay)?
> 
> Looks like you forgot to fix advanced-scripts.xul (remove helpURI, add
> reference to utilityOverlay and call to openPrefsHelp()).

oops, patch didn't apply cleanly to the new tree I was using, will be in next
rev.

> >Index: suite/common/bindings/prefwindow.xml
> 
> >+      <handler event="dialoghelp">
> >+      <![CDATA[
> >+        // currentPane is set in _selectPane for prefwindows which aren't childs.
> >+        // So if there's no currentPane, just use the first pane.
> >+        var pane = this.currentPane ? this.currentPane : this.preferencePanes[0];
> 
> The new currentPane property that you're adding on the base binding means this
> can just use .currentPane, right?

Yeah, I can do that, was pretty much just moving existing code to minimize
risk.

Comment 28

[Mike Connor [:mconnor]]

Attachment: with gavin's comments

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 311316 [details] [diff] [review]
with gavin's comments

>Index: browser/base/content/utilityOverlay.js

>+function openPrefsHelp() {

>+  // non-instant apply prefwindows are usually modal, so we can't open in the topmost window, 
>+  // since its probably behind the window.  The exception is if there's no window.opener.
>+  var instantApply = prefs.getBoolPref("browser.preferences.instantApply");
>+
>+  var helpTopic = document.getElementsByTagName("prefwindow")[0].currentPane.helpTopic;
>+  openHelpLink(helpTopic, !instantApply && window.opener);
>+}

Sorry for the flip-flop, but looking at this again, we don't need to protect against the "no open browser windows" case since openUILinkIn already handles that (by opening a new window). We just need to be sure to always take the "window" path if the dialog is modal, and !instantApply is true in all cases where we can end up with a modal dialog, so the opener check isn't needed (and will in fact prevent correct tab targeting in the "launched with -preferences and then clicked two help links" corner case). r=me with that removed.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311316 [details] [diff] [review]
with gavin's comments

a=beltzner with gavin's latest comments (commnet #29) addressed

Connor: please briefly describe the litmus cases that can be used to check the behavioural change, or include chrome tests

Comment 31

[Mike Connor [:mconnor]]

Litmus tests were already outlined in comment 22 when I set the in-testsuite? flag.

Checked in, won't spam the bug with the giant checkin spew.

Comment 32

[Steffen Wilberg]

On Windows, where the Options dialog is modal, this suffers from bug 256555: the new window with the help page is itself modal. You can use that new window for regular browsing, but can't access the original window or even the Options dialog while the new window is open. Only the new window appears on the Windows task bar. And it's fullscreen if you use fullscreen browsing windows, hiding the original window and the Options dialog.

Maybe we should open the help page in a popup window instead of a regular browser window?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Steffen, can you file a new bug (CC me and mconnor)? We can probably either use the opener or the window watcher to launch the window, or teach openUILinkIn to do that.

Comment 34

[Ian Spence]

>-        <menuitem id="menu_openHelp"
>-                  oncommand="openHelp('firefox-help', 'chrome://browser/locale/help/help.rdf');"
>+        <menuitem oncommand="openHelpLink('firefox-help')"

May I ask why the id attribute was removed? That regresses part of bug 415664 (specifically attachment 310754 [details] [diff] [review])

Comment 35

[Reed Loden [:reed]]

(In reply to comment #34)
> May I ask why the id attribute was removed? That regresses part of bug 415664
> (specifically attachment 310754 [details] [diff] [review])

I'm guessing this was a mistake, so I'm going to reland attachment 310754 [details] [diff] [review].

Comment 36

[Aaron Lawrence]

Can someone please refer me to the discussion (past or ongoing) about removing local help files? I tried a few google searches and didn't find much, but I'm not sure what to search for. This bug has no justification or discussion so is not helpful in understanding the decision. Thanks.

Comment 37

[David Tenser [:djst]]

(In reply to comment #36)
> Can someone please refer me to the discussion (past or ongoing) about removing
> local help files?

Here's an important part of the discussions: http://groups.google.com/group/mozilla.dev.l10n/browse_thread/thread/7ffb5ded536559f4/24649275882c5fc5#24649275882c5fc5

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Tested on each platform so far => Verified.

Comment 39

[Aaron Lawrence]

Thanks David. Looks like the decision was already taken before that post, presumably in IRC or even in face to face meetings at Mozilla foundation. 

Comment 40

[David Tenser [:djst]]

(In reply to comment #39)
> Thanks David. Looks like the decision was already taken before that post,
> presumably in IRC or even in face to face meetings at Mozilla foundation. 
> 

No, the decision was made one month after that post. See http://wiki.mozilla.org/WeeklyUpdates/2008-01-07#Marketing.2FPR. The discussion started in September though.

Comment 41

[Axel Hecht [:Pike]]

Recap:

The decision has been growing in several heads over the years, and has been discussed on and off, with changing pros and cons. SUMO entered the picture for various reasons, in-product help being surely in the back of some heads while doing so. So now that we have a technical infrastructure in place, the pros for moving the help content onto the web outweigh the cons, and we're doing it.

Comment 42

[Aaron Lawrence]

Thanks for the information all.
I think that built-in help is a basic requirement of any end-user application. The only apps I know that don't come with built-in help have an amateurish flavour to them.
Apart from the obvious problem that no help will be available if you're not connected to the internet... also the FF help viewer is really quite nice, and the web pages is probably not going to be anywhere as nice to use. Still I have to admit I have rarely if ever used it and the kind of help beginners need might not be covered by a fixed help file. So we'll see. I do predict though that there will be quite a bit of complaining when the change lands....

Will the new help be context sensitive? That's an important part. Linking to explanations of what dialog fields do (e.g. options).

Comment 43

[David Tenser [:djst]]

Aaron,

Thanks for your feedback. Can we please move this discussion out of Bugzilla though? You're welcome over to the mozilla.support.planning newsgroup, where I'd be happy to address your concerns.

Thanks!

Comment 44

[Tony Cassandra]

Question on this revolving around extensions:

Was the Help Viewer a mechanism that general extension developers could use? (I personally thought it was.)

If so, then removal of openHelp() would seem an important thing to update the release notes for extension developers for 3.0 so there are no surprises for any extension developers that are currently using this facility.

(From an extension developer who was very surprised to find his extension's help system broke in Beta 5.)

Comment 45

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=5229 added to litmus.