Closed
Bug 226911
Opened 21 years ago
Closed 20 years ago
Help and Support Center tracking bug
Categories
(SeaMonkey :: Help Documentation, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: danielwang, Assigned: neil)
References
()
Details
Attachments
(4 files, 7 obsolete files)
7.02 KB,
patch
|
rjkeller
:
review-
|
Details | Diff | Splinter Review |
5.58 KB,
patch
|
Biesinger
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
22.85 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
timeless
:
review+
|
Details | Diff | Splinter Review |
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•21 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•21 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•21 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•21 years ago
|
||
add _blank to link target. I also tweak the phone support description to be
more marketing friendly :-)
Reporter | ||
Updated•21 years ago
|
Attachment #136775 -
Flags: review?(rlk)
Comment 5•21 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•21 years ago
|
||
--> Browser: Help for review flags
Component: User → Help
Product: Documentation → Browser
Version: unspecified → Trunk
Updated•21 years ago
|
Attachment #136775 -
Flags: approval1.6b?
Comment 7•21 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•21 years ago
|
||
Fix checked in.
Comment 9•21 years ago
|
||
Please remove this patch.
_blank is not xhtml1.1 valid, or reformat the help files in html3.2.
Reporter | ||
Comment 10•21 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•21 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•21 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•21 years ago
|
||
Note that this depends on an unwritten patch to bug 227758.
Assignee | ||
Comment 14•21 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•21 years ago
|
||
This should prevent external links loading in help content.
Attachment #136775 -
Attachment is obsolete: true
Attachment #137017 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #138040 -
Flags: superreview?(darin)
Attachment #138040 -
Flags: review?(rlk)
Comment 16•21 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 17•21 years ago
|
||
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 18•21 years ago
|
||
Comment on attachment 137017 [details] [diff] [review]
Something like this
(clearing sr request, patch is obsolete)
Attachment #137017 -
Flags: superreview?(darin)
Assignee | ||
Comment 19•21 years ago
|
||
This patch forces a new browser window to open for external help links.
Assignee | ||
Updated•21 years ago
|
Attachment #138070 -
Flags: review?(cbiesinger)
Comment 20•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Reporter | ||
Updated•21 years ago
|
Attachment #139222 -
Attachment is patch: true
Attachment #139222 -
Attachment mime type: image/jpeg → text/plain
Attachment #139222 -
Flags: review?(rlk)
Comment 23•21 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•21 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•21 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•20 years ago
|
||
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•20 years ago
|
Attachment #154595 -
Flags: review?(rlk)
Updated•20 years ago
|
Attachment #154595 -
Flags: review?(rlk) → review+
Updated•20 years ago
|
Product: Browser → Seamonkey
Assignee | ||
Comment 27•20 years ago
|
||
Assignee | ||
Comment 28•20 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 29•20 years ago
|
||
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 30•20 years ago
|
||
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•20 years ago
|
||
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•20 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 33•20 years ago
|
||
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 34•20 years ago
|
||
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•20 years ago
|
||
Fix checked in. This should enable us to back out attachment 136775 [details] [diff] [review].
Comment 36•20 years ago
|
||
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•20 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'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's Guide to the DOM Inspector</a></li>
>+ "http://gr.ayre.st/moz/evangelism/tutorials/dominspectortutorial.shtml">grayrest'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•20 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•20 years ago
|
||
(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's Guide to the DOM Inspector</a></li>
> >+
"http://gr.ayre.st/moz/evangelism/tutorials/dominspectortutorial.shtml">grayrest'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•20 years ago
|
Attachment #168709 -
Attachment is obsolete: true
Attachment #168786 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•20 years ago
|
Attachment #168709 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 40•20 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•20 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•20 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•20 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•20 years ago
|
||
Attachment #168853 -
Flags: review?(timeless)
Attachment #168853 -
Flags: review?(timeless) → review+
Comment 45•20 years ago
|
||
Beware that after bug 252780 is fixed, there will be more _blank love to do...
Comment 46•20 years ago
|
||
AFAIK all the issues mentioned in the original report are now fixed.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•