Closed Bug 260054 Opened 17 years ago Closed 17 years ago

add context sensitive Help

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox1.5

People

(Reporter: steffen.wilberg, Assigned: steffen.wilberg)

References

Details

Attachments

(2 files, 6 obsolete files)

I want to see "Help" buttons in a couple of dialogs, most important in Options.

The backend is already (still rather) in place, see
http://www.mozilla.org/projects/help-viewer/content_packs.html#launching_help_viewer

I removed the Help button in the password manager in bug 217147 (among other
things). It doesn't even have l10n imact as far as I can see.
Adding a Help button is not that hard :)
Getting it to work in the Options panel is a bit trickier since the button is
in pref.xul, but it has to open Help sections depending on the content shown in
an iframe in the right half of the dialog. So I'm adding the helpTopic to the
respective file and query that from nsPrefWindow.js.

R.J., comments? I guess you're somewhat more familiar with context-sensitive
Help!?
Target Milestone: --- → Firefox1.0
Sounds good to me. Usually I'd say this would be bad if help was to be not
built, but currently its not possible without hacking the makefiles. Definetely
a different issue than what arises with seamonkey. So I say sure IMO. As always,
I'd ask ben (and only ben) for review/approval. Sounds like a decision he should
make.
Comment on attachment 159273 [details] [diff] [review]
patch for the main Options dialog

found in nsPrefWindows.js:
* CHANGES MUST BE REVIEWED BY ben@netscape.com!!

That's you I guess!? Do you want me to change your email address as well?
Attachment #159273 - Flags: review?(bugs)
(In reply to comment #3)
> (From update of attachment 159273 [details] [diff] [review])
> found in nsPrefWindows.js:
> * CHANGES MUST BE REVIEWED BY ben@netscape.com!!
> 
> That's you I guess!? Do you want me to change your email address as well?
> 

Not me :). I'd just say ben instead of a email address. Those things change often.
I meant: I guess that's you, Ben Goodger? Do you want me to change
ben@netscape.com to bugs@bengoodger.com?
Ben is the guy I requested review from at the same time you know :)
Comment on attachment 159347 [details] [diff] [review]
patch for the fonts, languages and connections dialogs

More Help buttons.
Attachment #159347 - Flags: review?(bugs)
Flags: blocking-aviary1.0?
Whiteboard: [have patch]
not a blocker, but nice-to-have.
Flags: blocking-aviary1.0? → blocking-aviary1.0-
With the UI freeze at the beginning of October and the rejection of a few other
minor issues with the Help viewer UI, this won't make 1.0.  Retargeting...
Target Milestone: Firefox1.0 → After Firefox 1.0
Status: NEW → ASSIGNED
Whiteboard: [have patch]
Target Milestone: Future → Firefox1.1
Comment on attachment 159273 [details] [diff] [review]
patch for the main Options dialog

Bitrot is near! (Bug 274712 )
Attachment #159273 - Attachment is obsolete: true
Attachment #159273 - Flags: review?(bugs)
Attachment #159347 - Attachment is obsolete: true
Attachment #159347 - Flags: review?(bugs)
*** Bug 283651 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.0-
Product: Firefox → Toolkit
Target Milestone: Firefox1.1 → ---
Targetting bugs which were targetted to Firefox1.1 before the move to
mozilla1.8beta2.
Target Milestone: --- → mozilla1.8beta2
*** Bug 287819 has been marked as a duplicate of this bug. ***
I'd be glad if anybody could give me a tip on how to add Help buttons in the new
preferences system.
Keywords: helpwanted
Attached patch patch for the new world, v1 (obsolete) — Splinter Review
This patch adds Help buttons to all prefpanes of the prefwindow, as well as all
of its child prefwindows and dialogs: conncection settings, sanitize settings,
fonts, colors, change actions, languages.
It does not include the <window>s, because we need to decide whether and where
to add the button manually: allowed sites/exceptions, cookies, download
actions.
It also does not include the password manager and the set/change/remove master
password prefwindows, and the dialogs in the Advanced-Security-Certificates
section, because we have no useful documentation for that.

If the prefpane has tabs, the Help button is specific to that, so if you click
Help when the passwords tab of the privacy pane is displayed, the Help Viewer
displays the section on passwords.

The most difficult part was to get the Help button to appear. In dialogs, you
just specify buttons="accept,cancel,help". This doesn't work in prefwindows.
But since I wanted to make the Help button specific to the selected tab, I
needed a special implementation anyway.

The "makeHelpButton" method un-hides the Help button if the current prefpane
has a helpURI attribute. That method is called at the end of the "showPane"
method. The problem is that the prefpane may not be finished with loading,
especially when clicking the pane the first time after a restart of Firefox.
So I used a timeout to call makeHelpButton. It needs to be quite a long one, at
least on my Linux/GTK2 build, which is pretty slow in displaying the
Preferences dialog. Maybe this can be improved by using an onload handler.

The "dialogHelp" handler calls openHelp(helpTopic, helpURI) in contextHelp.js.
The helpTopic can be set as an attribute of prefpanes, or tabs.
Attachment #182675 - Flags: first-review?(mconnor)
Moving this into the correct product/component, sorry for bugspam in advance...
Component: Help Viewer → Preferences
Flags: first-review?(mconnor)
Product: Toolkit → Firefox
QA Contact: help → preferences
Target Milestone: mozilla1.8beta2 → Firefox1.1
Version: unspecified → Trunk
Attachment #182675 - Flags: review?(mconnor)
Comment on attachment 182675 [details] [diff] [review]
patch for the new world, v1

>Index: mozilla/toolkit/content/widgets/preferences.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/preferences.xml,v
>retrieving revision 1.14
>diff -u -p -8 -r1.14 preferences.xml
>--- mozilla/toolkit/content/widgets/preferences.xml	28 Apr 2005 21:49:46 -0000	1.14
>+++ mozilla/toolkit/content/widgets/preferences.xml	5 May 2005 14:13:11 -0000
>@@ -579,20 +579,39 @@
>               }
>             };
>             
>             var obs = new OverlayLoadObserver(aPaneElement);
>             document.loadOverlay(aPaneElement.src, obs);
>           }
>           else
>             this._selectPane(aPaneElement);
>+
>+#ifndef XP_MACOSX
>+          //Let the Help button stay hidden on Mac, like the other buttons
>+          //XXXsw make this an onload handler?
>+          setTimeout(this.makeHelpButton, 100, aPaneElement);
>+#endif
>         ]]>
>         </body>
>       </method>

err, we do want the help button on mac,  you need to fix preferences.xml to
only hide the relevant buttons (instead of their parwent)

Also, why is it setTimeoted? If it is a "dom isn't ready" case, "0" should be
fine.


>+      <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];
>+        openHelp(pane.helpTopic, pane.getAttribute("helpURI"));
>+      </handler>

As you're using openHelp() in the binding, it has to provide this method
(that's instead of including an external script in every pane).
Attachment #182675 - Flags: review?(mconnor) → review-
> err, we do want the help button on mac,  you need to fix preferences.xml to
> only hide the relevant buttons (instead of their parwent)
So there should be a Help button on Mac, but no Close (or Cancel/Accept)
buttons, right?

> Also, why is it setTimeouted? If it is a "dom isn't ready" case, "0" should be
> fine.
"0" doesn't work. The Help button would stay hidden the first time you display a
prefpane after a restart of Firefox. But I've found a better solution. There's
already a "OverlayLoadObserver", which is called in the showPane method if the
prefpane hasn't been loaded before. _selectPane is called either from that or
directly in showPane. So I can move the logic into _selectPane and don't need a
timeout.
Attached patch patch v1.1 (obsolete) — Splinter Review
This patch fixes the Help button on Mac, and gets rid of the setTimeout.
Attachment #182675 - Attachment is obsolete: true
Well, ths still doesn't address the openHelp issue.
I'm not completely convinced that we need to do this, so I made a separate
patch for this.
Attachment #183265 - Attachment is obsolete: true
Attachment #183267 - Flags: review?(mconnor)
*** Bug 268299 has been marked as a duplicate of this bug. ***
Comment on attachment 183267 [details] [diff] [review]
include the contextHelp.js stuff into preferences.xml

IMO, its better to have these as methods/properties of the binding, instead of
external dependencies

>+      // copied from contextHelp.js
>+      // Locate existing help window for this helpFileURI.
>+      <method name="locateHelpWindow">
>+        <paramter name="helpFileURI"/>
>+        <body>
>+          const windowManagerInterface = Components
>+              .classes['@mozilla.org/appshell/window-mediator;1'].getService()
>+              .QueryInterface(Components.interfaces.nsIWindowMediator);
>+          const iterator = windowManagerInterface.getEnumerator("mozilla:help");
>+          var topWindow = null;
>+          var aWindow;
>+
>+          // Loop through help windows looking for one with selected helpFileURI
>+          while (iterator.hasMoreElements()) {
>+              aWindow = iterator.getNext();
>+              if (aWindow.getHelpFileURI() == helpFileURI) {
>+                  topWindow = aWindow;
>+              }
>+          }
>+          return topWindow;
>+        </body>
>+      </method>

please fix the atrocious styling here to fit toolkit and not the
should-be-beaten-with-a-stick "style" that Help code used.  also,
s/windowManagerInterface/wm/ for compactness/readability/style.

>+      <property name="helpTopic">
>+        <getter>
>+          // first try the panel
>+          var helpTopic = this.getAttribute("helpTopic");
>+
>+          // are there tabs, and do they offer a helpTopic?
>+          var box = this.getElementsByTagName("tabbox");
>+          if (box[0])
>+            var tab = box[0].selectedTab;
>+          if (tab)
>+            if (tab.hasAttribute('helpTopic'))
>+              helpTopic = tab.getAttribute("helpTopic");
>+
>+          return helpTopic;
>+        </getter>
>+      </property>

you can refactor this in a much more straightforward/compact way, trying the
panel first only makes sense if it takes priority:

// if there's tabs, use the tab's helpTopic if set
var box = this.getElementsByTagName("tabbox");
if (box[0]) {
  var tab = box[0].selectedTab;
  if (tab && tab.hasAttribute("helpTopic");
    return tab.getAttribute("helpTopic");
}

return this.getAttribute("helpTopic");

r=me with comments addressed

I'm sure that the positioning of the solitary Help button in the main
prefwindow is going to be wrong, but that can be done as a followup by someone
more Mac-focused *cough*
Attachment #183267 - Flags: review?(mconnor) → review+
Attached patch comments addressed (obsolete) — Splinter Review
This patch addresses comment 23.
Attachment #183267 - Attachment is obsolete: true
Attachment #186240 - Flags: review+
Attachment #186240 - Flags: approval-aviary1.1a2?
Same as above, with some indentation fixes.
Attachment #186240 - Attachment is obsolete: true
Attachment #186242 - Flags: review+
Attachment #186242 - Flags: approval-aviary1.1a2?
Attachment #186240 - Flags: review+
Attachment #186240 - Flags: approval-aviary1.1a2?
Attachment #186242 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Whiteboard: [checkin needed]
Checked in. Please file new bugs if you want more Help buttons, or don't like
its position on Mac :-)
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Attached patch fix typoSplinter Review
Oops.
Attachment #186638 - Flags: review?(mconnor)
Attachment #186638 - Flags: approval-aviary1.1a2?
Comment on attachment 186638 [details] [diff] [review]
fix typo

man, how'd I miss that?
Attachment #186638 - Flags: review?(mconnor) → review+
Attachment #186638 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Comment on attachment 186638 [details] [diff] [review]
fix typo

Checked in.
You need to log in before you can comment on or make changes to this bug.