273518 - Home button displays wrong content pack, and defaultTopic is hardcoded

Status: RESOLVED FIXED

(SeaMonkey :: Help Viewer, defect)

Description

[Rod Whiteley]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

The help viewer opens a separate window for each content pack.  So Firefox help
has its own window, and any other content pack has its own window.  But the Home
button always displays Firefox help, even in a different content pack's window.

Reproducible: Always
Steps to Reproduce:
1. Display a content pack other than Firefox help.
2. Press the Home button.

Actual Results:  
The topic "Welcome to Mozilla Firefox Help" is displayed.

Expected Results:  
The default topic specified in the content pack's master help file should be
displayed.

This happens because help.js contains a goHome function where the home page URI
is hard coded instead of being looked up by topic.

Comment 1

[Steffen Wilberg]

Indeed. Confirming and taking.

Comment 2

[Steffen Wilberg]

Attachment: patch

use displayTopic(defaultTopic) instead of
loadURI("chrome://help/locale/firefox_welcome.xhtml").

While experimenting with defaultTopic, I found another (hidden) bug:
defaultTopic was const'ed to "use-help". This is both wrong and unnecessary.
The topic is either set
1. in the openHelp() call (like in helpMenuOverlay.xul), passed as an argument
to the Help window and evaluated in init(),
2. in nc:defaulttopic in the content pack (e.g. firebirdhelp.rdf), evaluated in
loadHelpRDF(),
3. "welcome" as a fallback, specified in loadHelpRDF() as well; displayTopic()
then uses the defaultTopic because topic is null.

Comment 3

[R.J. Keller]

One problem. What was supposed to happen is that helpTopic is defined by the
value taken in when showHelp() was called, and if it is not defined then we use
the content pack default. You are currently overriding any possible function
value. I'd remove line 136 and modify contextHelp.js to not take in a parameter
for a topic (which would be the best thing to do IMO then making a hack around it).

Comment 4

[Steffen Wilberg]

showHelp? You probably mean openHelp?
I don't understand you. openHelp() still opens the welcome page, and
openHelp("cookies") still opens cookies.xhtml.

And I still need openHelp(topic) to work because of bug 260054 (add Help buttons
to some dialogs).

Comment 5

[R.J. Keller]

(In reply to comment #4)
> showHelp? You probably mean openHelp?
> I don't understand you. openHelp() still opens the welcome page, and
> openHelp("cookies") still opens cookies.xhtml.
> 
> And I still need openHelp(topic) to work because of bug 260054 (add Help buttons
> to some dialogs).

oh wait, getting my variables mixed up :). We need some documentation in help
because reviewing this patch is way more of a pain then it should be. From first
glance it looks like helpTopic will be null, but all the functions set things in
line when they check for it and will end up using defaultTopic when helpTopic is
null. Totally forgot that it did that. Sorry! It's all good! Error on my part.

We should really throw a comment in somewhere noting that if helpTopic is null
then defaultTopic is used.

Comment 6

[Steffen Wilberg]

Attachment: also document a bit

I agree, writing the patch was a bit of an advventure. So I've added a comment
based on comment 2.

Comment 7

[Steffen Wilberg]

Attachment: stay with var helpTopic = defaultTopic;

It's even clearer with the original  var helpTopic = defaultTopic;.

Comment 8

[R.J. Keller]

In your patch, why not use preprocessor comments (# comments instead of // or
/**/)? It'll save a couple of bytes from the final JS file and will keep
consistency with the rest of the file.

Comment 9

[Steffen Wilberg]

- help.js is not at all consistent in its comment style.
- I don't like #-style comments that much since they make line numbers in the js
console pretty useless.
- My Editor doesn't recognize the #-style and colors the commments.
- browser.js only use //-style comments. Over 600 of them.

Comment 10

[R.J. Keller]

Comment on attachment 168170 [details] [diff] [review]
stay with var helpTopic = defaultTopic;

OK, sounds good. r=[e-mail]

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 168170 [details] [diff] [review]
stay with var helpTopic = defaultTopic;

r=me for checkin to toolkit

As a note, prevailing style throughout Firefox and toolkit UI code is to use
gDefaultTopic instead of defaultTopic for global vars, it makes code a LOT
easier to follow.  Obviously that isn't the case in Help code right now, but
going forward changing to that style is probably going to be expected/helpful.

Comment 12

[Steffen Wilberg]

Checking in mozilla/toolkit/components/help/content/help.js;
/cvsroot/mozilla/toolkit/components/help/content/help.js,v  <--  help.js
new revision: 1.17; previous revision: 1.16
done

Comment 13

[Steffen Wilberg]

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