226911 - Help and Support Center tracking bug

Status: RESOLVED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[Daniel Wang]

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]

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

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

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

Attachment: 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]

Comment on attachment 136775 [details] [diff] [review]
add _blank to link target

Looks good! r=[e-mail]

Comment 6

[R.J. Keller]

--> Browser: Help for review flags

Comment 7

[Asa Dotzler [:asa]]

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]

Fix checked in.

Comment 9

[Frédéric Chateaux]

Please remove this patch.

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

Comment 10

[Daniel Wang]

> 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

[[e-mail]]

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]

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

[[e-mail]]

Attachment: Something like this

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

Comment 14

[[e-mail]]

Comment on attachment 137017 [details] [diff] [review]
Something like this

Asking someone who hopefully knows about nsIContentListener

Comment 15

[[e-mail]]

Attachment: Proposed patch

This should prevent external links loading in help content.

Comment 16

[R.J. Keller]

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

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

Comment on attachment 137017 [details] [diff] [review]
Something like this

(clearing sr request, patch is obsolete)

Comment 19

[[e-mail]]

Attachment: 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)]

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

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]

Attachment: header for welcome page

Comment 23

[R.J. Keller]

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]

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]

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

[[e-mail]]

Attachment: New approach

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

Comment 27

[[e-mail]]

Attachment: Last try at a content listener

Comment 28

[[e-mail]]

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 [:bzbarsky]]

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

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

[[e-mail]]

Attachment: Final patch, surely ;-)

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

Comment 32

[[e-mail]]

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 [:bzbarsky]]

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

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

[[e-mail]]

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

Comment 36

[Stefan [:stefanh]]

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

[[e-mail]]

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

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

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

[[e-mail]]

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

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

Comment 42

[[e-mail]]

(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

[[e-mail]]

(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

[[e-mail]]

Attachment: Fix clicks on formatted external links

Comment 45

[Giacomo Magnini]

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

Comment 46

[Stefan [:stefanh]]

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