** Observed with 11/29/99 Win32 build (1999112908) ** ** This was contributed orginally by a Mozilla user via the Mozilla I18n newsgroup ** The above site is encoded in Big5 and contains 3 frames -- the top, the middle,and the bottom frames. The middle frame contains a pop-up menu using FORM. The menu options all have Chinese names. These names don't show correctly even when the Encoding menu is set to Chinese Traditional (Big5). To reproduce this bug: 1. Have Traditional Chinsese fonts installed on your Windows system. 2. Go access the above URL after having set "View | Character Set: Multibyte | Chinese Traditional (Big 5)". 3. Observe that the middle frame which contains pop-up menu does not show any of the menu items in Chinese. If you access the middle frame directly under Big5 encoding, Mozilla can display Chinese characters correctly: http://us01.www.appledaily.com.hk/page/19991130/newm.htm

Reassigning to Eric.

I think this is related to bug 20315.

Hehe, me too. ;) Perhaps you meant another bug number?

Sorry, I meant the bug 20290

I created the simple non-meta frame test case in http://babel/tests/browser/bft/sjis_no_meta/bft_frame_index_sjis.html I tested this page in 112908 Win32 build. After I visited this page and changed character set to Japanese (Shift_JIS), the Japanese characters are displayed as garbage. I changed QA contact to teruko@netscape.com

cc to cata@netscape.com.

This seems NOT related to bug 20290 because it occurs on non-frame pages.

I think we should keep this bug for the frame case and keep the other one for non-frame case.

Putting on PDT+ radar.

Marking M12 for the bug PDT+ push.

I am taking a look at this bug now. I've got Big5 fonts installed and working on my Linux box. Got started here: http://www.mozilla.org/quality/intl/m11intlstatus.html But the fonts are no longer at: ftp://ftp.etl.go.jp/pub/mule/fonts/ and have moved to: ftp://ftp.etl.go.jp/pub/mule/intlfonts-1.2-split/ Just grabbed Chinese.BIG.tar.gz gunzip Chinese.BIG.tar.gz cd intlfonts-1.2/Chinese.BIG bdftopcf cc40s.bdf > cc40s.pcf (repeat for each bdf file) mkfontdir xset +fp `pwd` start apprunner, and away you go. :) Comboboxes showing Chinese fonts works okay. I am able to reproduce the bug and am taking a look. Can anyone point me to what functions are called when View->CharsetMultibyte->Big 5 is chosen?

Ah... BrowserSetDefaultCharacterSet() in resources/content/navagator.js Which does SetDocumentCharset on the appCore then reloads the page...

Looking at our implementation of SetDocumentCharset in: mozilla/xpfe/browser/src/nsBrowserInstance.cpp It seems we are setting the charset on the top web shell, but not traversing to get all of the child webshells and setting the charset on them as well.

Oops, this is not the problem. The child webshells have their charset set by the document viewer. My next best guess is that the reload destroys the child webshells and recreates them, but does not pass default charset information down to them on creation. (???) Investigating this...

I have confirmed the above. I'm thinking of fixing this in nsHTMLFrameInnerFrame. After a webshell is created, I can get the parent document viewer's default character set, then set the new child's document viewer's character set to be the same.

Is the charset not passed down when the child webshells are added to the parent webshell? Check out nsWebShell::AddChild.

Ah, great! Thanks for the pointer, I'll debug this. :)

Reassigning to Cata due to Frank Tang's suggestion. There is a META charset loading bug that is supposedly required to get this fixed...

One thing I did notice was that in nsWebShell::AddChild, when the parent's charset it supposed to be passed down to the child, the child does not have a content viewer, so this code doesn't execute: 4559 nsCOMPtr<nsIContentViewer> childCV; 4560 NS_ENSURE_SUCCESS(childAsDocShell->GetContentViewer(getter_AddRefs(childCV)), 4561 NS_ERROR_FAILURE); 4562 if(childCV) <<<<<<< childCV is null here so we don't set the charset 4563 { 4564 nsCOMPtr<nsIMarkupDocumentViewer> childmuDV = do_QueryInterface(childCV); 4565 if(childmuDV) 4566 { 4567 NS_ENSURE_SUCCESS(childmuDV->SetDefaultCharacterSet(defaultCharset), 4568 NS_ERROR_FAILURE); 4569 NS_ENSURE_SUCCESS(childmuDV->SetForceCharacterSet(forceCharset), 4570 NS_ERROR_FAILURE); 4571 } 4572 }

Reassigned to cata, so clearing date until he has new estimate.

I also noticed that in nsWebShell::Embed, where the new content viewer is embeded in the web shell, it fails to get the charset "Big5" and set it on the new content viewer. This is because mChrsetReloadState == eCharsetReloadInit: if (mContentViewer && (eCharsetReloadInit!=mCharsetReloadState)) { // get any interesting state from the old content viewer // XXX: it would be far better to just reuse the document viewer , // since we know we're just displaying the same document as before ... (get charset from old content viewer and set in new content viewer) ... Because of this, in nsWebShell::AddChild, the charset that the parent is trying to set on the child is "ISO-8895-1" instead of "Big5", as it should be. I commented out the last half of this condition: if (mContentViewer) // && (eCharsetReloadInit!=mCharsetReloadState)) { // get any interesting state from the old content viewer // XXX: it would be far better to just reuse the document viewer , // since we know we're just displaying the same document as before This caused the new content viewer to get the old content viewer's charset "Big5" in the reload. Then in nsWebShell::AddChild, when it gets the charset it get's the correct one. We still have the problem that the child content viewer is not created until after the AddChild call. Perhaps the solution would be for the child to get it's parent's charset in the child's Embed call? This would ensure that the content viewer was already created. I'll try this out real quick and see if it does the trick... Attaching a simplified test case...

:p Nope, that didn't work either. It does set the child doc viewer's charset to "Big5", but it's still displaying with the wrong font! Here's the logic I tried for nsWebShell::Embed that didn't work (and may be egregiously wrong, I don't know): 835 nsCOMPtr<nsIContentViewer> oldContentViewer; 836 if (mContentViewer) 837 { // get any interesting state from the old content viewer 838 oldContentViewer = mContentViewer; 839 } 840 else { // This is our first content viewer... 841 // get any interesting state from our parent's content viewer 842 nsCOMPtr<nsIWebShell> parent; 843 nsresult res = GetParent(*getter_AddRefs(parent)); 844 if (NS_SUCCEEDED(res) && parent) { 845 nsCOMPtr<nsIContentViewer> parentContentViewer; 846 res = parent->GetContentViewer(getter_AddRefs(parentContentViewer)); 847 if (NS_SUCCEEDED(res)) { 848 oldContentViewer = parentContentViewer; 849 } 850 } 851 } 852 853 if (oldContentViewer) // && (eCharsetReloadInit!=mCharsetReloadState)) 854 { 855 // XXX: it would be far better to just reuse the document viewer , 856 // since we know we're just displaying the same document as before ... use oldContentViewer where mContentViewer was used below... I'll attach a patch. It doesn't seem to solve the problem at hand, but I don't know where to go from here.

looks like this got broken when I switched the charset data from webshell to content viewer. I did test this case, maybe something else changed too? In any event, if the content viewer is not available at the time of nsWebShell->AddChild, then we can do the same work (setting the various pieces of charset data) to the time when the content viewer is hooked up to the webshell. The code that hooks the two objects up would just look at the parent webshell and get the charset data from it.

after apply the same debug patch (debug patch 1, and 2) in bug 21429, loading the test cases produce the following- From default charset, charset = ISO-8859-1 set to parser charset = ISO-8859-1 source 2 From default charset, charset = set to parser charset = source 2 From default charset, charset = set to parser charset = source 2 Notice the defaul charset for the inner frame is null

Buster, if I'm reading code correctly, it looks like nsWebShell::Embed is the routine that hooks up the content viewer and the webshell. Is that right? My patch (attached above) does what you suggest, but for some reason it's not working. In nsWebShell::Embed, when a child's content viewer is embeded, I made it grab it's parent and get the charset from there. I stepped through the code and it appears to be working, but the font that's chosen is still wrong. (Where is the content viewer's charset used?) I'll try the debug charset patch Frank mentions (bug 21429) to see what the results are both before and after adding the changes to nsWebShell::Embed.

Before and after my patch no change. I did see the charset being set to Big5 in both cases when I changed it using the View menu. I did notice this though. In nsHTMLDocument::StartDocumentLoad(): 538 nsCOMPtr<nsIMarkupDocumentViewer> muCV; 539 nsCOMPtr<nsIContentViewer> cv; 540 webShell->GetContentViewer(getter_AddRefs(cv)); 541 if (cv) { 542 muCV = do_QueryInterface(cv); 543 } When we call webShell->GetContentViewer, it returns null but also returns NS_OK. This prevents us from getting a muCV and even though we're failing all along here, we proceed to set the charset to null and mark the source as kCharsetFromUserDefault. The code that does this is not protected by the if (muCV) as it apparently should be? I also noticed that the code I hacked in nsWebShell::Embed is called AFTER nsHTMLDocument::StartDocumentLoad, and I was assuming the opposite. This means that even though the charset gets passed down happily to the content viewer in nsWebShell::Embed, it's too late because the parser and document have already had their charset assigned the value "" (due to the aforementioned bug). Perhaps we need to do all of this work in nsHTMLDocument::StartDocumentLoad. If we don't have an old content viewer in our webshell, get our parent's content viewer and get the charset from there. Is this reasonable? This would make the charset code in nsWebShell::AddChild and nsWebShell::Embed unneccessary, I think. We also probably don't need the logic in nsDocumentViewer that recurses through all of the children and sets their charsets if we're just going to destroy them...

Yes, this fixes the bug (and bug 21429 and possibly 21234 and 20290). I'll post a patch in a moment...

I'd love to get this reviewed and checked in this weekend. Can someone take a look and tell me if it is sane? (Total bugfix = part one plus part two)

I will review the fix tonight. Sorry I couldn't get to it earlier, I was unavailable for the last 2 days.

The changes look great. I applied them and tested the various related bugs and everything seems to work (though my chinese is a little rusty.) I'd talk to frank before checking in, just to make sure he doesn't see any obvious holes with the new strategy. Also, I will attach a slightly modified version that does a little better error checking and reporting. The truth is, that whole file needs a facelift with respect to error handling, but every little bit helps. You can accept it or not, in either event the code looks great. Thanks for tackling this.

steve- can you do a cvs -c ? It is not easy to fiture where you diff should go by just the line number .... I am adding code to that file too...

It looks like buster's patch and the nsWebShell.cpp changes fix the problem. Great job. said r=ftang in your cvs check in log.

Thanks guys! I'm going to ask for approval using Steve's modified changes to nsHTMLDocument in addition to the little fix in nsWebShell.

Just checked in the fixes.

I verified this in 121608 Win32, 121612 Linux and Mac build.

<HTML> <FRAMESET COLS=*,*> <FRAME SRC="http://bugzilla.mozilla.org/showattachment.cgi?attach_id=3374"> <FRAME SRC="http://bugzilla.mozilla.org/showattachment.cgi?attach_id=3375"> </FRAMESET> </HTML> I have to reopen this bug. The check in fix the problem with 1 level depth frame . But if the frame is another frameset, the problem is still exist. See the above html. The first one is ok, but not the 2nd one. Reopen and makr M13.

*** Bug 22318 has been marked as a duplicate of this bug. ***

Removing the outdated entry on the whiteboard about "fix in hand" since this has reopened etc.

I tested this with multiply nested framesets in one document e.g. <HTML> <FRAMESET> <FRAMESET> ... And that works okay, it seems the problem is only if you have a frameset doucment that contains a frame pointing to a frameset document e.g. <HTML> <FRAMESET> <FRAME SRC="foo.html"> (where foo.html is a frameset document) I'll take a look.

I have a fix for this bug - it is to mirror the logic I added to nsHTMLDocument::StartDocumentLoad to originally fix this bug to nsWebShell::Embed A summary of what was going wrong in Test case #2: Webshell A Webshell B (frame 1 in test case #2) Webshell C (frame containing chinese characters) Webshell (about:blank) Webshell (about:blank) Before loading Webshell B ( StartDocumentLoad() ), we were getting the charset information from it's parent Webshell A (Big 5) and handing it off to the parser. This is enough to cause any documents in Webshell B to be displayed correctly. Unfortunately, when we later created a content viewer for Webshell B and attached it to a webshell ( Embed() ) we never copied down the parent charset information from A to B. Thus, when Webshell C is going to be loaded ( StartDocumentLoad() ), we get it's parent charset information from Webshell B (ISO 8859-1) and pass that off to the parser. Since we never set the charset on Webshell B, it passes of the default charset to the parser, and will display with ISO 8859-1 fonts. The fix is to copy the charset information from A to the content viewer for B when we create a content viewer for B (nsWebShell::Embed). I'll attach my proposed patch. It looks a lot like the patch to nsHTMLDocument::StartDocumentLoad that was added earlier, including Steve's debugging enhancements.

changes look good. r=buster.

Thanks Steve! I just checked in the fix. 1. Have Traditional Chinese fonts installed on your system. 2. Go access Test case #2 after having set "View | Character Set: Multibyte | Chinese Traditional (Big 5)". 3. Observe that the top left frame does show text in Chinese. Thanks!

I had to back this change out as it was causing some major regressions (Javascript was not executing, event handlers were not executing). Will try to get it in later today if I can find a way to fix the problem...

*** Bug 24262 has been marked as a duplicate of this bug. ***

I have a fix for this bug that does not cause the JS regression. It is logically equivalent to the first fix, but makes sure that the old content viewer is destroyed at the right time (it was a scoping issue of an nsCOMPtr just as Tom Pixley suspected all along.) Will attach for review momentarily.

r=buster (although I missed the regression on my last review, so trust me at your peril!)

Fix checked in. Thanks!

I verified this in 2000012008 Win32, Mac, and Linux build.