Last Comment Bug 226911 - Help and Support Center tracking bug
: Help and Support Center tracking bug
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Help Documentation (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
: Daniel Wang
:
Mentors:
chrome://help/locale/welcome_help.xhtml
Depends on:
Blocks: 46917
  Show dependency treegraph
 
Reported: 2003-11-27 00:31 PST by Daniel Wang
Modified: 2005-01-28 08:27 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add _blank to link target (3.51 KB, patch)
2003-12-03 23:48 PST, Daniel Wang
rjkeller: review+
asa: approval1.6b+
Details | Diff | Splinter Review
Something like this (2.29 KB, patch)
2003-12-07 15:13 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Proposed patch (5.84 KB, patch)
2003-12-27 14:08 PST, neil@parkwaycc.co.uk
cbiesinger: review-
Details | Diff | Splinter Review
Alternate patch (5.78 KB, patch)
2003-12-28 09:05 PST, neil@parkwaycc.co.uk
cbiesinger: review-
Details | Diff | Splinter Review
header for welcome page (7.02 KB, patch)
2004-01-16 13:02 PST, Daniel Wang
rjkeller: review-
Details | Diff | Splinter Review
New approach (5.53 KB, patch)
2004-07-28 15:26 PDT, neil@parkwaycc.co.uk
rjkeller: review+
Details | Diff | Splinter Review
Last try at a content listener (4.99 KB, patch)
2004-12-05 06:43 PST, neil@parkwaycc.co.uk
cbiesinger: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
Final patch, surely ;-) (5.58 KB, patch)
2004-12-06 09:42 PST, neil@parkwaycc.co.uk
cbiesinger: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Remove target="_blank" from Help files (21.99 KB, patch)
2004-12-14 09:48 PST, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
New version of target="_blank" removal (22.85 KB, patch)
2004-12-15 12:12 PST, Stefan [:stefanh]
neil: review+
Details | Diff | Splinter Review
Fix clicks on formatted external links (1.28 KB, patch)
2004-12-16 02:35 PST, neil@parkwaycc.co.uk
timeless: review+
Details | Diff | Splinter Review

Description Daniel Wang 2003-11-27 00:31:25 PST
Need to resolve all issues in bug 220395:
1. We need to set up a server redirect for start page
2. Release Notes doesn't go to the real Release Note page
   (see patch in 150900)
3. Start center links open in the Help window. This could potentially be
   a security problem
4. The design is ugly :-(
Comment 1 R.J. Keller 2003-11-27 07:02:22 PST
> 3. Start center links open in the Help window. This could potentially be
>    a security problem

yeah, but not sure how to make it open in the browser. The problem is that it
doesn't seem possible to detect it.
Comment 2 R.J. Keller 2003-11-27 08:53:00 PST
> 4. The design is ugly :-(

Got any suggestions for a better design? I'm not the best artist, but if you
have some ideas, I'd love to put them in.
Comment 3 Daniel Wang 2003-12-03 21:37:59 PST
>> 3. Start center links open in the Help window. This could potentially be
>>    a security problem
>
> yeah, but not sure how to make it open in the browser. The problem is that it
> doesn't seem possible to detect it.

use target="_blank"
see privacy_help.html for example
Comment 4 Daniel Wang 2003-12-03 23:48:59 PST
Created attachment 136775 [details] [diff] [review]
add _blank to link target

add _blank to link target. I also tweak the phone support description to be
more marketing friendly :-)
Comment 5 R.J. Keller 2003-12-04 04:21:31 PST
Comment on attachment 136775 [details] [diff] [review]
add _blank to link target

Looks good! r=rlk@trfenv.com
Comment 6 R.J. Keller 2003-12-04 04:22:32 PST
--> Browser: Help for review flags
Comment 7 Asa Dotzler [:asa] 2003-12-04 15:23:20 PST
Comment on attachment 136775 [details] [diff] [review]
add _blank to link target

a=asa (on behalf of drivers) for checkin to 1.6beta
Comment 8 R.J. Keller 2003-12-04 15:27:00 PST
Fix checked in.
Comment 9 Frédéric Chateaux 2003-12-07 03:11:35 PST
Please remove this patch.

_blank is not xhtml1.1 valid, or reformat the help files in html3.2.
Comment 10 Daniel Wang 2003-12-07 04:44:32 PST
> Please remove this patch.
> 
> _blank is not xhtml1.1 valid, or reformat the help files in html3.2.

ummm... no. The patch is to prevent loading of remote pages in Help. I think the
better solution is to file a bug to prevent loading of remote page with a new
chrome-only page container.

CC'ing Neil, who might know if there's any security risk in loading remote pages
in Help.
Comment 11 neil@parkwaycc.co.uk 2003-12-07 06:42:15 PST
It's certainly possible. For instance, there used to be a bug that when you had
editor open but not navigator, then any internal or external link would open in
that editor window. That was solved by registering a URI content listener on the
content's document shell. I think we can do the same here.
Comment 12 R.J. Keller 2003-12-07 07:11:28 PST
personally, I think that using target="_blank" is just fine. I have no issues
with it. The only alternative is to use JavaScript to open it in a new navigator
window, which is something that I would not like to do.

I prefer to keep things simple. If it's that big of an issue, we'll deal with it
during the next alpha cycle, but for now, this patch is fine.
Comment 13 neil@parkwaycc.co.uk 2003-12-07 15:13:49 PST
Created attachment 137017 [details] [diff] [review]
Something like this

Note that this depends on an unwritten patch to bug 227758.
Comment 14 neil@parkwaycc.co.uk 2003-12-07 15:14:47 PST
Comment on attachment 137017 [details] [diff] [review]
Something like this

Asking someone who hopefully knows about nsIContentListener
Comment 15 neil@parkwaycc.co.uk 2003-12-27 14:08:57 PST
Created attachment 138040 [details] [diff] [review]
Proposed patch

This should prevent external links loading in help content.
Comment 16 R.J. Keller 2003-12-27 14:15:04 PST
Comment on attachment 138040 [details] [diff] [review]
Proposed patch

I'm not very familiar with content listeners, so I'm going to ask that caillon
review this.

<beingPicky>
Just to note, I prefer brackets on its own line, meaning:

try
{
}
catch (ex)
{
}

instead of 
try {
} catch (ex) {
}

It's not a huge deal if you don't change it, but if caillon gives a review- for
some reason and you want a new patch, I'd like it to be changed (it should be
quick and makes it a lot easier to read).
</beingPicky>
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2003-12-27 16:30:50 PST
Comment on attachment 138040 [details] [diff] [review]
Proposed patch

it would be better to open a new window in onStartURIOpen (like we do now),
instead of reusing existing browser windows (which is what this patch does)
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2003-12-27 16:31:14 PST
Comment on attachment 137017 [details] [diff] [review]
Something like this

(clearing sr request, patch is obsolete)
Comment 19 neil@parkwaycc.co.uk 2003-12-28 09:05:15 PST
Created attachment 138070 [details] [diff] [review]
Alternate patch

This patch forces a new browser window to open for external help links.
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2003-12-28 10:37:16 PST
Comment on attachment 138070 [details] [diff] [review]
Alternate patch

hmm. this has the side effect that javascript console shows a security error
when I click an http link in the help viewer. that's not great...
Comment 21 Christian :Biesinger (don't email me, ping me on IRC) 2003-12-28 10:53:40 PST
Comment on attachment 138070 [details] [diff] [review]
Alternate patch

other than that error in js console, this looks great, but the error should
really be avoided. maybe you should just check aURI.schemeIs("chrome") (or jar,
not sure)
Comment 22 Daniel Wang 2004-01-16 13:02:45 PST
Created attachment 139222 [details] [diff] [review]
header for welcome page
Comment 23 R.J. Keller 2004-01-17 17:48:47 PST
hmm. I'm not really a big fan of the lizard on the left or the color choice for
this image. Can we have some consistency with the mozilla.org website?
Comment 24 R.J. Keller 2004-01-21 13:20:29 PST
I'll r= this image if it appears on the 1.7 (or 1.6) start page, since it'd be 
best to have consistency between the two pages. Let's wait until I hear more on 
what they're doing with that.
Comment 25 R.J. Keller 2004-02-06 19:19:14 PST
Comment on attachment 139222 [details] [diff] [review]
header for welcome page

Daniel, it is a nice try but I agree with what Bart said about the images on
the 1.6 start page (bug 224352 comment 41). We need more professional images
for the main Help page.
Comment 26 neil@parkwaycc.co.uk 2004-07-28 15:26:13 PDT
Created attachment 154595 [details] [diff] [review]
New approach

OK, I've given up on the uri content handler approach.
Comment 27 neil@parkwaycc.co.uk 2004-12-05 06:43:59 PST
Created attachment 167919 [details] [diff] [review]
Last try at a content listener
Comment 28 neil@parkwaycc.co.uk 2004-12-05 06:46:36 PST
Comment on attachment 167919 [details] [diff] [review]
Last try at a content listener

This is the only way I can get everything to work including dead links spinning
the throbber before alerting once the connection times out.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2004-12-05 09:56:11 PST
Comment on attachment 167919 [details] [diff] [review]
Last try at a content listener

>Index: help.js

>+function contentClick(event) {

>+  // is this an internal link?
>+  var href = event.target.href;
>+  if (href.lastIndexOf("chrome:", 0) == 0)
>+    return true;

Doesn't this need to rewrap the object with that xpc wrapper thing before
getting .href?

>+  var channel = ioService.newChannel(href, null, null);

Please pass in the origin charset here?  Use the document charset of
event.target.ownerDocument.

>+  channel.loadFlags = channel.LOAD_DOCUMENT_URI | channel.VALIDATE_NEVER;

Why VALIDATE_NEVER?  That doesn't seem necessary...

>+  Components.classes['@mozilla.org/uriloader;1']
>+            .getService(Components.interfaces.nsIURILoader)
>+            .openURI(channel, true, helpExternal.docShell);

File a followup bug on removing the hack that forces you to use a docshell here
once we've sorted out the actual dependencies of openURI()?

>+  return false;

This will open URIs in some other random tab on right clicks, etc, won't it. 
Also on shift-click, alt-click, middle-click, and so forth....	 Is that
desirable?

You really want nsIURIContentListener2 here, eh?  (Being able to tell the URI
from the isPreferred request, eg... could you also file a followup bug on this,
cc me, biesi, and darin?)

sr-, because of the right-click issue.
Comment 30 Christian :Biesinger (don't email me, ping me on IRC) 2004-12-06 07:38:09 PST
Comment on attachment 167919 [details] [diff] [review]
Last try at a content listener

what bz said, plus:

why are you moving this line:
-    helpBaseURI = helpFileURI.substring(0, helpFileURI.lastIndexOf("/")+1); //
trailing "/" included.

Don't you need to check that it's the left mouse button that was clicked?


r- for some of the issues bz mentioned
Comment 31 neil@parkwaycc.co.uk 2004-12-06 09:42:28 PST
Created attachment 168038 [details] [diff] [review]
Final patch, surely ;-)

OK, so I figured out my typo and got loadURI to do my dirty work for me.
Comment 32 neil@parkwaycc.co.uk 2004-12-06 09:45:39 PST
Comment on attachment 168038 [details] [diff] [review]
Final patch, surely ;-)

>+  helpBrowser.addProgressListener(window.XULBrowserWindow, Components.interfaces.nsIWebProgress.NOTIFY_ALL);
>+  helpExternal.addProgressListener(window.XULBrowserWindow, Components.interfaces.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
Sorry about this... I was fiddling about with the flags trying to get my other
variations to work; just let me know which flag you think I should use.
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2004-12-06 10:57:26 PST
Comment on attachment 168038 [details] [diff] [review]
Final patch, surely ;-)

sr=bzbarsky.  I don't have any strong feelings on the notifications here....
Comment 34 Christian :Biesinger (don't email me, ping me on IRC) 2004-12-06 11:01:53 PST
Comment on attachment 168038 [details] [diff] [review]
Final patch, surely ;-)

I think I'd stay with _ALL...

do you have to pass IS_LINK?
Comment 35 neil@parkwaycc.co.uk 2004-12-06 15:51:06 PST
Fix checked in. This should enable us to back out attachment 136775 [details] [diff] [review].
Comment 36 Stefan [:stefanh] 2004-12-14 09:48:58 PST
Created attachment 168709 [details] [diff] [review]
Remove target="_blank" from Help files

I haven't included any doctype changes, if we add xul elements (bug 249744) to
the files we'll probably have to remove the doctypes anyway...
Comment 37 neil@parkwaycc.co.uk 2004-12-14 16:36:13 PST
Comment on attachment 168709 [details] [diff] [review]
Remove target="_blank" from Help files

Index: line missing!
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/help/resources/locale/en-US/cert_dialog_help.xhtml,v
>retrieving revision 1.11
>diff -u -r1.11 cert_dialog_help.xhtml

> picker. If you are familiar with HTML hexadecimal color codes, you
> can type a specific code or you can just type a color name (for
> example, <q>blue</q>). You&apos;ll find the official W3C list of CSS supported
>-color names <a href="http://www.w3.org/TR/CSS21/syndata.html#color-units" 
>-target="_blank">here</a>, and another list of commonly supported color names 
>-<a href="http://www.w3schools.com/html/html_colornames.asp" target="_blank">here</a>.</li>
>+color names
>+<a href="http://www.w3.org/TR/CSS21/syndata.html#color-units">here</a>, and
>+another list of commonly supported color names 
>+<a href="http://www.w3schools.com/html/html_colornames.asp">here</a>.</li>
Perhaps if you rewrapped the previous paragraph more aggressively you wouldn't
get the silly looking short lines...

>   <li>Use &brandShortName; My Webpage
>-    <a href="http://mywebpage.netscape.com/" target="_blank"><tt>http://mywebpage.netscape.com/</tt></a>
>+    <a href="http://mywebpage.netscape.com/"><tt>http://mywebpage.netscape.com/</tt></a>
Whoops, someone overdid the search and replace ;-) would you mind fixing it?

>-      <li><a href="http://www.brownhen.com/DI.html" target="_blank">
>+      <li><a href="http://www.brownhen.com/DI.html">
>         Introduction to the DOM Inspector</a> (Ian Oeschger)</li>
I wonder why the line was broken before Introduction...

>       <li><a href=
>-        "http://gr.ayre.st/moz/evangelism/tutorials/dominspectortutorial.shtml"
>-        target="_blank">grayrest&apos;s Guide to the DOM Inspector</a></li>
>+        "http://gr.ayre.st/moz/evangelism/tutorials/dominspectortutorial.shtml">grayrest&apos;s
>+	Guide to the DOM Inspector</a></li>
I don't really like breaking the line just because of a long URL :-/

>+  the online web page,
>+  <a href="http://home.netscape.com/plugins/manager.html">&brandShortName;
>+  Plug-in Manager</a>.</p>
Again, fix the description please.

>     <a href=
>-    "http://www.mozilla.org/projects/security/pki/nss/nss-3.4/nss-3.4-algorithms.html"
>-    target="_blank">Encryption Technologies Available in NSS 3.4</a>.</li>
>+    "http://www.mozilla.org/projects/security/pki/nss/nss-3.4/nss-3.4-algorithms.html">Encryption
>+    Technologies Available in NSS 3.4</a>.</li>
Again, line broken in the middle of a tag.

>-  <a href="https://certs.netscape.com/" target="_blank">Client
>+  <a href="https://certs.netscape.com/">Client
>   Certificates</a>.</p>
Might this fit on one line now?

>+      <p><strong><a href="http://www.mozilla.org/support/#community">User Newsgroups</a></strong></p>
This should have been broken between User and Newsgroups.

Most of these comments don't make sense if this was in fact a direct backout of
attachment 136775 [details] [diff] [review], so it's fine if you don't want to fix them.
Comment 38 Stefan [:stefanh] 2004-12-15 03:05:26 PST
(> Most of these comments don't make sense if this was in fact a direct backout 
of
> attachment 136775 [details] [diff] [review] [edit], so it's fine if you don't want to fix them.

Well, backing out attachment 136775 [details] [diff] [review] will only fix the welcome page...

Comment 39 Stefan [:stefanh] 2004-12-15 12:12:51 PST
Created attachment 168786 [details] [diff] [review]
New version of target="_blank" removal

(In reply to comment #37)
> (From update of attachment 168709 [details] [diff] [review] [edit])
> Index: line missing!
> >===================================================================
Sorry about that!
> 
> >+color names
> >+<a href="http://www.w3.org/TR/CSS21/syndata.html#color-units">here</a>, and

> >+another list of commonly supported color names 
> >+<a href="http://www.w3schools.com/html/html_colornames.asp">here</a>.</li>
> Perhaps if you rewrapped the previous paragraph more aggressively you
wouldn't
> get the silly looking short lines...
Looks a bit better now, I think.
 
> >   <li>Use &brandShortName; My Webpage
> >-	<a href="http://mywebpage.netscape.com/"
target="_blank"><tt>http://mywebpage.netscape.com/</tt></a>
> >+	<a
href="http://mywebpage.netscape.com/"><tt>http://mywebpage.netscape.com/</tt></a>

> Whoops, someone overdid the search and replace ;-) would you mind fixing it?
Done.

> >-	  <li><a href="http://www.brownhen.com/DI.html" target="_blank">
> >+	  <li><a href="http://www.brownhen.com/DI.html">
> >	    Introduction to the DOM Inspector</a> (Ian Oeschger)</li>
> I wonder why the line was broken before Introduction...
Fixed. 
 
> >	  <li><a href=
> >-	   
"http://gr.ayre.st/moz/evangelism/tutorials/dominspectortutorial.shtml"
> >-	    target="_blank">grayrest&apos;s Guide to the DOM Inspector</a></li>

> >+	   
"http://gr.ayre.st/moz/evangelism/tutorials/dominspectortutorial.shtml">grayrest&apos;s

> >+	Guide to the DOM Inspector</a></li>
> I don't really like breaking the line just because of a long URL :-/
Fixed.
 
> >+  the online web page,
> >+  <a href="http://home.netscape.com/plugins/manager.html">&brandShortName;
> >+  Plug-in Manager</a>.</p>
> Again, fix the description please.
Done.

> 
> >	<a href=
> >-   
"http://www.mozilla.org/projects/security/pki/nss/nss-3.4/nss-3.4-algorithms.html"

> >-	target="_blank">Encryption Technologies Available in NSS 3.4</a>.</li>
> >+   
"http://www.mozilla.org/projects/security/pki/nss/nss-3.4/nss-3.4-algorithms.html">Encryption

> >+	Technologies Available in NSS 3.4</a>.</li>
> Again, line broken in the middle of a tag.
Fixed.

> >-  <a href="https://certs.netscape.com/" target="_blank">Client
> >+  <a href="https://certs.netscape.com/">Client
> >   Certificates</a>.</p>
> Might this fit on one line now?

I've hand-edited this particular file in order to keep every line<80 char width
and it's still intact, so if you don't mind I'll keep it as it is ;)
> 
> >+	  <p><strong><a href="http://www.mozilla.org/support/#community">User
Newsgroups</a></strong></p>
> This should have been broken between User and Newsgroups.
Fixed.
Comment 40 neil@parkwaycc.co.uk 2004-12-15 15:56:42 PST
Comment on attachment 168786 [details] [diff] [review]
New version of target="_blank" removal

What was the issue about these links that you mentioned on IRC when I was
unavailable?
Comment 41 Stefan [:stefanh] 2004-12-15 22:50:38 PST
(In reply to comment #40)
> (From update of attachment 168786 [details] [diff] [review] [edit])
> What was the issue about these links that you mentioned on IRC when I was
> unavailable?

It's odd, but when I hit the link at 
http://lxr.mozilla.org/seamonkey/source/extensions/help/resources/locale/en-
US/composer_help.xhtml#1954 (when target="_blank" is removed)the link opens in 
the Help window.
Comment 42 neil@parkwaycc.co.uk 2004-12-16 02:14:47 PST
(In reply to comment #41)
>(In reply to comment #40)
>>(From update of attachment 168786 [details] [diff] [review])
>>What was the issue about these links that you mentioned on IRC when I was
>>unavailable?
>It's odd, but when I hit the link at 
>http://lxr.mozilla.org/seamonkey/source/extensions/help/resources/locale/en-
>US/composer_help.xhtml#1954 the link opens in the Help window.
I see the problem. It doesn't like the <tt> inside the <a>. Swap them please?
Comment 43 neil@parkwaycc.co.uk 2004-12-16 02:27:53 PST
(In reply to comment #42)
>It doesn't like the <tt> inside the <a>.
No, actually, I need to fix that to work properly.
Comment 44 neil@parkwaycc.co.uk 2004-12-16 02:35:58 PST
Created attachment 168853 [details] [diff] [review]
Fix clicks on formatted external links
Comment 45 Giacomo Magnini 2005-01-28 02:06:52 PST
Beware that after bug 252780 is fixed, there will be more _blank love to do...
Comment 46 Stefan [:stefanh] 2005-01-28 08:27:12 PST
AFAIK all the issues mentioned in the original report are now fixed.

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