New Windows opened via Javascript do not inherit their parent's character set

VERIFIED FIXED in mozilla1.0.1

Status

()

defect
P2
normal
VERIFIED FIXED
19 years ago
17 years ago

People

(Reporter: mkaply, Assigned: shanjian)

Tracking

({intl})

Trunk
mozilla1.0.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] [ETA 07/10], )

Attachments

(1 attachment, 2 obsolete attachments)

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>
Do you know which charset is acutally used instead of the parent's, UTF-8?
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.
adding keywords intl and nsbeta1
Keywords: intl, nsbeta1
*** Bug 71272 has been marked as a duplicate of this bug. ***
error: Bug 71272 is not a dup of this, my mistake.
Shanjian, could you take a look at this?
Assignee: nhotta → shanjian
Status: NEW → ASSIGNED
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.
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.
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.
NS 4.61 and Internet Explorer do this.

There is no Moz release that has ever done this right
Shanjian - Please affix a target milestone and priority P3.
Changing QA contact to teruko@netscape.com.  
QA Contact: andreasb → teruko
Shanjian - Status pls . . .
changing to nsbeta1-. this one does not meet beta stopper guidelines.
Keywords: nsbeta1nsbeta1-
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
move to moz0.9.3
Target Milestone: mozilla0.9.2 → mozilla0.9.3
I haven't got time to look this yet. put it further.
Target Milestone: mozilla0.9.3 → mozilla1.0
*** Bug 94281 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.0 → mozilla1.0.1
Nominating this to nsbeta1.
Keywords: nsbeta1
nsbeta1+ per i18n triage
Keywords: nsbeta1nsbeta1+
nsbeta1- . if we have real case , then please ask again. 
Keywords: nsbeta1+nsbeta1-
*** Bug 136352 has been marked as a duplicate of this bug. ***
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.
Another example:
Click on the links on the left of page http://www.the9.com, the pop-up window is
displayed garbled.
page does not load with important sites. nsbeta1+
Blocks: 141008
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt2]
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. 
This is the way it works in all other browsers. So once again, Mozilla is the 
only broke one...
please take a look ag dom/base/src/nsGlobalWindow.cpp look at GlobalWindowImpl::Open
any progress? eta?
Posted patch patch (obsolete) — Splinter Review
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. 
daniel is on vacatin. jud say ask alecf to review it.
I call alecf, he is on conference. Who else can review this?
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 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+
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!)
Posted patch patch v2 (obsolete) — Splinter Review
Attachment #87733 - Attachment is obsolete: true
alecf, could you take one more look? thanks.
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
Attachment #89284 - Attachment is obsolete: true
comment corrected. 
extra rv assignment removed.
rv is always assigned before check in following code, so not a problem.
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.
I will replace the comment before checkin. If that is the only concern, please
mark r=.
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 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+
fix checked in to trunk. 
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
I verified this as fixed with 7-09 Win32, Mac, and Linux trunk builds.
Verified.
Status: RESOLVED → VERIFIED
Whiteboard: [adt2] → [adt2] [ETA 07/10]
adding adt1.0.1-.  Let's get this in the next release.
Keywords: adt1.0.1-
Blocks: 157673
Depends on: 180372
No longer blocks: 157673
You need to log in before you can comment on or make changes to this bug.