Closed
Bug 70507
Opened 24 years ago
Closed 22 years ago
New Windows opened via Javascript do not inherit their parent's character set
Categories
(Core :: Internationalization, defect, P2)
Core
Internationalization
Tracking
()
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: mkaply, Assigned: shanjian)
References
()
Details
(Keywords: intl, Whiteboard: [adt2] [ETA 07/10])
Attachments
(1 file, 2 obsolete files)
2.18 KB,
patch
|
jag+mozilla
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
When you open a new window using open, the parents character set is not inherited. use the following HTML: <html> <head> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=Shift_JIS"> <script language="JavaScript"> function WinOpen() { msg=open("","DisplayWindow"); msg.document.write("<HEAD><TITLE>This window is opened by JavaScript</TITLE></HEAD>"); msg.document.write("<CENTER><H1>ABCDEFこのGHIJKL</H1></CENTER>"); } </script> </head> <body> <H1>ABCDEFこのGHIJKL</H1> <form> <input type="button" name="Button1" value="Push me" onclick="WinOpen()"> </form> </body> </html> Not, the window that comes up displays the Japanese correctly, but it is NOT in the correct character set. This can be verified by going to the character set pulldown. On platforms that don't display everything in Unicode (like OS/2) this becomes plainly obvious. We are told to display this page in x-western. This is probably related to http://bugzilla.mozilla.org/show_bug.cgi?id=45187 but this is worse, because you can't change the character set of the opened window via the menu.
<offtopic>i think there's a bug w/ patch for f10 to show the menu even when it's programatically hidden. although that doesn't actually solve this problem.</offtopic>
Comment 2•24 years ago
|
||
Do you know which charset is acutally used instead of the parent's, UTF-8?
Reporter | ||
Comment 3•24 years ago
|
||
In the charset menu, ISO-8859-1 is checked, and in the font code I see x-western, but I can't tell between UTF-8 and ISO-8859-1 with that. It's definitely not the "default" character set, because I see this problem on a Japanese system as well where the default is Shift_JIS. My guess is that it is ISO-8859-1 because that is what the character set menu indicates.
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•23 years ago
|
||
I took a look at this bug today. In my understanding, the charset information is needed for the system to choose the right font. However, I could not find an easy way to pass this information from laucher window to launchee window. The relationship between these two is not parent-and-child. I might need some time to think about a possible solution.
Comment 9•23 years ago
|
||
Not only that mozilla doesn't inherit the charset from its parent, even when the new window contains a charset defination it ignores it. It worked very fine until some days (or weeks? I don't think) ago.
Assignee | ||
Comment 10•23 years ago
|
||
I tried 60 RTM, it did not work either. If you can point me a version which works, that might help me find a solution.
Reporter | ||
Comment 11•23 years ago
|
||
NS 4.61 and Internet Explorer do this. There is no Moz release that has ever done this right
Comment 12•23 years ago
|
||
Shanjian - Please affix a target milestone and priority P3.
Comment 14•23 years ago
|
||
Shanjian - Status pls . . .
Comment 15•23 years ago
|
||
changing to nsbeta1-. this one does not meet beta stopper guidelines.
Comment 16•23 years ago
|
||
Setting TM = M0.9.2 | P3. FTang/Shanjian - Pls change this, if you do not think it is appropriate.
Priority: -- → P2
Target Milestone: --- → mozilla0.9.2
Assignee | ||
Comment 18•23 years ago
|
||
I haven't got time to look this yet. put it further.
Target Milestone: mozilla0.9.3 → mozilla1.0
Comment 19•23 years ago
|
||
*** Bug 94281 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla1.0 → mozilla1.0.1
Comment 22•23 years ago
|
||
nsbeta1- . if we have real case , then please ask again.
Reporter | ||
Comment 23•22 years ago
|
||
*** Bug 136352 has been marked as a duplicate of this bug. ***
Comment 24•22 years ago
|
||
Comment #22: > nsbeta1- . if we have real case , then please ask again. Renominate this as nsbeta1 - there are some real web sites has this problem with pop-up window. Here is an example: 1. Load page http://www.ntv.co.jp/3min/. 2. Click on the 3rd player button (bakku nann ba-). Then you will see the contains in pop-up window is displayed garbled.
Comment 25•22 years ago
|
||
Another example: Click on the links on the left of page http://www.the9.com, the pop-up window is displayed garbled.
Comment 26•22 years ago
|
||
page does not load with important sites. nsbeta1+
Assignee | ||
Comment 27•22 years ago
|
||
http://www.ntv.co.jp/3min I don't think this problem can be addressed soon. It is even arguable if this is a client problem at all. The javascript open a new blank window first, and then the html document is targetted to this new window. A straight solution should be to let each window openned by js inherit its launcher's charset, and current doc's charset will become next doc's default charset. Both of those must work, but the 2nd issue is arguable. To inherit previous doc's charset and default charset for current doc makes browser have a state, and that may confuse user in certain situation.
Reporter | ||
Comment 28•22 years ago
|
||
This is the way it works in all other browsers. So once again, Mozilla is the only broke one...
Comment 29•22 years ago
|
||
please take a look ag dom/base/src/nsGlobalWindow.cpp look at GlobalWindowImpl::Open
Comment 30•22 years ago
|
||
any progress? eta?
Assignee | ||
Comment 31•22 years ago
|
||
Assignee | ||
Comment 32•22 years ago
|
||
daniel, could you review my patch? For linked page, we use default charset to carry over the existing (launcher) doc charset. So I guess we should do the same thing here.
Comment 33•22 years ago
|
||
daniel is on vacatin. jud say ask alecf to review it.
Comment 34•22 years ago
|
||
I call alecf, he is on conference. Who else can review this?
Comment 35•22 years ago
|
||
Comment on attachment 87733 [details] [diff] [review] patch I've slightly changed your patch to be a little more readable and follow the file's variable naming conventions. It looks like this is the right place to do this, but I don't know this code well enough to be sure you're not doing it in cases where we don't want this to happen. I would appreciate it if you can find someone who knows this code better for an additional review. newDocShellItem->SetName(nameSpecified ? name.get() : nsnull); nsCOMPtr<nsIDocShell> newDocShell(do_QueryInterface(newDocShellItem)); + + // copy default charset to new docshell + nsCOMPtr<nsIScriptGlobalObject> parentSGO(do_QueryInterface(aParent)); + if (parentSGO) { + nsCOMPtr<nsIDocShell> parentDocshell; + parentSGO->GetDocShell(getter_AddRefs(parentDocshell)); + + nsCOMPtr<nsIContentViewer> parentContentViewer; + parentDocshell->GetContentViewer(getter_AddRefs(parentContentViewer)); + nsCOMPtr<nsIDocumentViewer> parentDocViewer(do_QueryInterface(parentContentViewer)); + if (parentDocViewer) { + nsCOMPtr<nsIDocument> doc; + rv = parentDocViewer->GetDocument(*getter_AddRefs(doc)); + + nsCOMPtr<nsIContentViewer> newContentViewer; + newDocShell->GetContentViewer(getter_AddRefs(newContentViewer)); + nsCOMPtr<nsIMarkupDocumentViewer> newMarkupDocViewer(do_QueryInterface(newContentViewer)); + if (doc && newMarkupDocViewer) { + nsXPIDLString defaultCharset; + rv = doc->GetDocumentCharacterSet(defaultCharset); + if (NS_SUCCEEDED(rv)) + newMarkupDocViewer->SetDefaultCharacterSet(defaultCharset); + } + } + } if (uriToLoad) { // get the script principal and pass it to docshell
Summary: New Windows opened via Javascript to not inherit the parents character set → New Windows opened via Javascript do not inherit their parent's character set
Comment 36•22 years ago
|
||
Comment on attachment 87733 [details] [diff] [review] patch The logic looks fine, but the code is somewhat confusing. you're mixing NS_ENSURE_SUCCESS with a bunch of nested if()'s... and its not clear why some errors are fatal and some allow the code to continue. Can you please add some comments to both describe the block and to describe what you're doing here?
Attachment #87733 -
Flags: needs-work+
Comment 37•22 years ago
|
||
ah, I just saw jag's comment. His code is more readable, but I still want comments. Why is this block of code important (obviously, we know because we're reading this bug... but imagine someone encountering this and just not knowing!)
Assignee | ||
Comment 38•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #87733 -
Attachment is obsolete: true
Assignee | ||
Comment 39•22 years ago
|
||
alecf, could you take one more look? thanks.
Comment 40•22 years ago
|
||
Comment on attachment 89284 [details] [diff] [review] patch v2 >+ >+ // When a new window is opened through Js, we want the new document use >+ // its launcher doc's charset as its default charset. This will make new >+ // window inherit parent window's charset if it has no charset specification >+ // of its own. Failed to set this charset is not fatal, so we want to continue. Nit: That last sentence should probably start with "Failure", not "Failed". >+ if (parentDocViewer) { >+ nsCOMPtr<nsIDocument> doc; >+ rv = parentDocViewer->GetDocument(*getter_AddRefs(doc)); Why set rv here if you're not going to use it? >+ >+ nsCOMPtr<nsIContentViewer> newContentViewer; >+ newDocShell->GetContentViewer(getter_AddRefs(newContentViewer)); >+ nsCOMPtr<nsIMarkupDocumentViewer> newMarkupDocViewer(do_QueryInterface(newContentViewer)); >+ if (doc && newMarkupDocViewer) { Test doc earlier? No matter, success case is very likely, this way you don't over-indent with if{...if{...}}. Never mind! >+ nsXPIDLString charset; >+ rv = doc->GetDocumentCharacterSet(charset); >+ if (NS_SUCCEEDED(rv)) >+ newMarkupDocViewer->SetDefaultCharacterSet(charset); >+ } >+ } >+ } > > if (uriToLoad) { // get the script principal and pass it to docshell > Might any code below test or return rv without resetting it? /be
Assignee | ||
Comment 41•22 years ago
|
||
Attachment #89284 -
Attachment is obsolete: true
Assignee | ||
Comment 42•22 years ago
|
||
comment corrected. extra rv assignment removed. rv is always assigned before check in following code, so not a problem.
Comment 43•22 years ago
|
||
please use: When a new window is opened through JavaScript, we want the it to use the charset of its opener as a fallback in the event the document being loaded does not specify a charset. Failing to set this charset is not fatal, so we want to continue. -- as a replacement for the paragraph brendan flagged.
Assignee | ||
Comment 44•22 years ago
|
||
I will replace the comment before checkin. If that is the only concern, please mark r=.
Comment 45•22 years ago
|
||
Comment on attachment 90026 [details] [diff] [review] correct comment Timeless's rewording is nice, but he has an added "the": When a new window is opened through JavaScript, we want >>>the<<< it to use the charset of its opener as a fallback in the event the document being loaded does not specify a charset. Failing to set this charset is not fatal, so we want to continue +++in the face of errors+++. I added the +++-delimited words at the end. If you like those, add them to the end of the comment, but don't include the extraneous >>>the<<<. sr=brendan@mozilla.org
Attachment #90026 -
Flags: superreview+
Comment 46•22 years ago
|
||
Comment on attachment 90026 [details] [diff] [review] correct comment Erh. r=jag I guess, even though the code is mine (and I didn't test it, I hope you did).
Attachment #90026 -
Flags: review+
Assignee | ||
Comment 47•22 years ago
|
||
fix checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 48•22 years ago
|
||
I verified this as fixed with 7-09 Win32, Mac, and Linux trunk builds.
Updated•22 years ago
|
Keywords: approval,
mozilla1.0.1
Whiteboard: [adt2] → [adt2] [ETA 07/10]
You need to log in
before you can comment on or make changes to this bug.
Description
•