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 :-(

> 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.

> 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.

>> 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

add _blank to link target. I also tweak the phone support description to be more marketing friendly :-)

Comment on attachment 136775 [details] [diff] [review] add _blank to link target Looks good! r=rlk@trfenv.com

--> Browser: Help for review flags

Comment on attachment 136775 [details] [diff] [review] add _blank to link target a=asa (on behalf of drivers) for checkin to 1.6beta

Fix checked in.

Please remove this patch. _blank is not xhtml1.1 valid, or reformat the help files in html3.2.

> 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.

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.

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.

Note that this depends on an unwritten patch to bug 227758.

Comment on attachment 137017 [details] [diff] [review] Something like this Asking someone who hopefully knows about nsIContentListener

This should prevent external links loading in help content.

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 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 on attachment 137017 [details] [diff] [review] Something like this (clearing sr request, patch is obsolete)

This patch forces a new browser window to open for external help links.

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 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)

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?

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 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.

OK, I've given up on the uri content handler approach.

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 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 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

OK, so I figured out my typo and got loadURI to do my dirty work for me.

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 on attachment 168038 [details] [diff] [review] Final patch, surely ;-) sr=bzbarsky. I don't have any strong feelings on the notifications here....

Comment on attachment 168038 [details] [diff] [review] Final patch, surely ;-) I think I'd stay with _ALL... do you have to pass IS_LINK?

Fix checked in. This should enable us to back out attachment 136775 [details] [diff] [review].

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 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.

(> 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...

(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 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?

(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-﷒0﷓ (when target="_blank" is removed)the link opens in the Help window.

(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?

(In reply to comment #42) >It doesn't like the <tt> inside the <a>. No, actually, I need to fix that to work properly.

Beware that after bug 252780 is fixed, there will be more _blank love to do...

AFAIK all the issues mentioned in the original report are now fixed.