The default bug view has changed. See this FAQ.

Help and Support Center tracking bug

RESOLVED FIXED

Status

SeaMonkey
Help Documentation
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: Daniel Wang, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

14 years ago
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

14 years ago
> 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.
Status: NEW → ASSIGNED

Comment 2

14 years ago
> 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.
(Reporter)

Comment 3

14 years ago
>> 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
(Reporter)

Comment 4

14 years ago
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 :-)
(Reporter)

Updated

14 years ago
Attachment #136775 - Flags: review?(rlk)

Comment 5

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

Looks good! r=rlk@trfenv.com
Attachment #136775 - Flags: review?(rlk) → review+

Comment 6

14 years ago
--> Browser: Help for review flags
Component: User → Help
Product: Documentation → Browser
Version: unspecified → Trunk

Updated

14 years ago
Attachment #136775 - Flags: approval1.6b?

Comment 7

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

a=asa (on behalf of drivers) for checkin to 1.6beta
Attachment #136775 - Flags: approval1.6b? → approval1.6b+

Comment 8

14 years ago
Fix checked in.

Comment 9

14 years ago
Please remove this patch.

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

Comment 10

14 years ago
> 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.
(Assignee)

Comment 11

14 years ago
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

14 years ago
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.
(Assignee)

Comment 13

14 years ago
Created attachment 137017 [details] [diff] [review]
Something like this

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

Comment 14

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

Asking someone who hopefully knows about nsIContentListener
Attachment #137017 - Flags: superreview?(darin)
(Assignee)

Comment 15

13 years ago
Created attachment 138040 [details] [diff] [review]
Proposed patch

This should prevent external links loading in help content.
Attachment #136775 - Attachment is obsolete: true
Attachment #137017 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #138040 - Flags: superreview?(darin)
Attachment #138040 - Flags: review?(rlk)

Comment 16

13 years ago
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>
Attachment #138040 - Flags: review?(rlk) → review?(caillon)
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)
Attachment #138040 - Flags: superreview?(darin)
Attachment #138040 - Flags: review?(caillon)
Attachment #138040 - Flags: review-
Comment on attachment 137017 [details] [diff] [review]
Something like this

(clearing sr request, patch is obsolete)
Attachment #137017 - Flags: superreview?(darin)
(Assignee)

Comment 19

13 years ago
Created attachment 138070 [details] [diff] [review]
Alternate patch

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

Updated

13 years ago
Attachment #138070 - Flags: review?(cbiesinger)
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)
Attachment #138070 - Flags: review?(cbiesinger) → review-
(Reporter)

Comment 22

13 years ago
Created attachment 139222 [details] [diff] [review]
header for welcome page
(Reporter)

Updated

13 years ago
Attachment #139222 - Attachment is patch: true
Attachment #139222 - Attachment mime type: image/jpeg → text/plain
Attachment #139222 - Flags: review?(rlk)

Comment 23

13 years ago
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

13 years ago
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

13 years ago
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.
Attachment #139222 - Flags: review?(rlk) → review-
(Assignee)

Comment 26

13 years ago
Created attachment 154595 [details] [diff] [review]
New approach

OK, I've given up on the uri content handler approach.
Assignee: rlk → neil.parkwaycc.co.uk
Attachment #138040 - Attachment is obsolete: true
Attachment #138070 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #154595 - Flags: review?(rlk)

Updated

13 years ago
Attachment #154595 - Flags: review?(rlk) → review+
Product: Browser → Seamonkey
(Assignee)

Comment 27

13 years ago
Created attachment 167919 [details] [diff] [review]
Last try at a content listener
(Assignee)

Comment 28

13 years ago
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.
Attachment #167919 - Flags: superreview?(bzbarsky)
Attachment #167919 - Flags: review?(cbiesinger)
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.
Attachment #167919 - Flags: superreview?(bzbarsky) → superreview-
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
Attachment #167919 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 31

13 years ago
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.
Attachment #154595 - Attachment is obsolete: true
Attachment #167919 - Attachment is obsolete: true
(Assignee)

Comment 32

13 years ago
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.
Attachment #168038 - Flags: superreview?(bzbarsky)
Attachment #168038 - Flags: review?(cbiesinger)
Comment on attachment 168038 [details] [diff] [review]
Final patch, surely ;-)

sr=bzbarsky.  I don't have any strong feelings on the notifications here....
Attachment #168038 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 168038 [details] [diff] [review]
Final patch, surely ;-)

I think I'd stay with _ALL...

do you have to pass IS_LINK?
Attachment #168038 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 35

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

Comment 36

12 years ago
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...
Attachment #168709 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 37

12 years ago
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

12 years ago
(> 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

12 years ago
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.

Updated

12 years ago
Attachment #168709 - Attachment is obsolete: true
Attachment #168786 - Flags: review?(neil.parkwaycc.co.uk)

Updated

12 years ago
Attachment #168709 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 40

12 years ago
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?
Attachment #168786 - Flags: review?(neil.parkwaycc.co.uk) → review+

Comment 41

12 years ago
(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.
(Assignee)

Comment 42

12 years ago
(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?
(Assignee)

Comment 43

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

Comment 44

12 years ago
Created attachment 168853 [details] [diff] [review]
Fix clicks on formatted external links
Attachment #168853 - Flags: review?(timeless)

Updated

12 years ago
Attachment #168853 - Flags: review?(timeless) → review+

Comment 45

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

Comment 46

12 years ago
AFAIK all the issues mentioned in the original report are now fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.