Closed Bug 222346 Opened 21 years ago Closed 21 years ago

Switching character coding changes background color at Yahoo Mail

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: david.haas, Assigned: smontagu)

References

(
URL
)

Details

(Keywords: testcase)

Attachments

(2 files, 5 obsolete files)

Test case to reproduce yahoo bug
14.86 KB, application/zip
Details
a new patch (with assertion in Set..Char..Set in debug build)
7.51 KB, patch
smontagu
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
1) Login to a Yahoo Mail account.
2) Click on "Mail Options" -> "Colors" -> Lime Green theme.
3) Click on "Mail" -> pick a folder with an e-mail message in it.
4) Open up message.
5) In the menu, select View -> Character Coding -> More -> East Asian ->
Japanese EUC (Doesn't actually matter what you choose, as long as it's not the
current encoding). 

What you'd expect:
Characters now displayed with new encoding, otherwise no change

What actually happens:
Characters now displayed with new encoding, background color switches from lime
green to blue.

Happens with Mozila 1.5rc2, ~1 month old Firebird, recent Camino (all OS X). 
Not sure if it affects other platforms.
Does the lime green stylesheet come with a charset param in the http header?  Or
a @charset style rule?

I bet the problem is that the stylesheet can only be parsed in whatever the
default encoding of the page is and does not provide us with that information...
So the webpage source looks like this to begin:

**********************
<!--web40411-->
<html> 
<head> 
<title>Yahoo! Mail - foo@yahoo.com</title>
<script>
<!-- 
if (typeof top.frames["wmailmain"] != "undefined") {
    window.open("http://mail.yahoo.com", "_top");
}
var ypim_color = "lime";
// -->
</script>
<script src="http://us.js1.yimg.com/us.yimg.com/lib/pim/c3/ylib_dom.js"></script>
<script language=JavaScript
src="http://us.js1.yimg.com/us.yimg.com/lib/pim/j3/pim.js"></script>
<script language=JavaScript
src="http://us.js1.yimg.com/us.yimg.com/lib/pim/css2/pim_css.js"></script>

**************
The ypim_color variable is important.

ylib_dom.js appears to work with the DOM (oddly enough).
pim.js looks to be dropdown menu/button code.
pim_css.js I think is the relevant script.  It uses ypim_color to pick a CSS.
If ypim_color = null, it sets it to blue.  

Assuming the JS gets fed ypim_color lime, the following stylesheet gets called:
http://us.js1.yimg.com/us.yimg.com/lib/pim/css2/pim_style_lime.css

I didn't notice a @charset style rule.  Here's what the server sent back to me:
haasd@rmm014 /haasd % curl --head
http://us.js1.yimg.com/us.yimg.com/lib/pim/css2/pim_style_lime.css
HTTP/1.0 200 OK
Content-Type: text/css
Content-Length: 3803
Last-Modified: Tue, 25 Jun 2002 23:35:23 GMT
Date: Thu, 16 Oct 2003 16:40:23 GMT
Connection: keep-alive



Yeah, if the sheet specifies no other encoding in any way and there is no
charset attribute on the <link> that loads it, then we assume that it needs to
be parsed in the same encoding as the document.  But that sheet looks like plain
ascii, so it should parse in all 1-byte encodings, really...

Is there any way to reproduce this without a yahoo mail account?  Does saving
the page as "web page, complete", then opening it from the hard drive and
switching encodings cause the same problem?
No, saving as Web Page complete, opening the disk file, then changing the
charset does not cause the color change.
Could you possibly zip up (or tar up) the saved files for "web page, complete"
and attach them to this bug?
Attached file "Web Page, Complete" (obsolete) — 
tar'd, zipped folder.  Inside is web page & web page complete folder (if that
makes sense).
Yeah, loading this from local disk does not cause a color change (even after I
removed the <link>s that mozilla inserted in the source)... This is not a
painting bug; not sure what's going on here.
Assignee: roc → general
Component: Layout: View Rendering → Browser-General
Keywords: qawanted
QA Contact: ian → general
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031028

I've been trying to get a test case working for this; hasn't worked out so far
but here are some observations:

I have a yahoo account so I tried reproducing and was able to do so reliably. 
It may be worthwhile to note that I also tried earlier today at work, where I
use Mozilla 1.3.1 on Windows 2000, and could not reproduce it.

When I reproduce the bug, I've noticed that it doesn't really appear to just
have switched themes.  All the fonts appear in Times New Roman, and the
formatting is missing (similar to if you had a broken stylesheet link).

Using Jesse Ruderman's bookmarklet for viewing style sheets
(http://www.squarefree.com/bookmarklets/webdevel.html) I tried looking at the
style information for the page both when the bug appears, and when it renders
normally.  On the normal page, all the inline css as well as the
"pim_style_lime" and "pim_style_ie_lime" stylesheets are captured.  However, on
the page the bug appears on, only the hardcoded inline css is captured, and
neither lime stylesheet appears.  No blue stylesheet appears either!

As noted earlier, the link tags for the "pim_style" stylesheets and some of the
inline css are generated by pim_css.js.  It is this generated css info that is
missing from the buggy page.  Is it possible that the script is not even being run? 

As a side note, whenever I change the character coding on this page (either the
real yahoo site or a saved copy) I get the following errors in the Javascript
console:

Error: ylib_keyevt is not defined

Error: oBw is not defined

These appear regardless of whether the bug is reproduced.

Please let me know if more info is necessary...I'd be happy to put up
screenshots of the bug if anyone would find it helpful.   
This test case should reproduce the bug.  After unzipping:

1. Navigate to the ShowLetter.htm file.
2. Verify that green theme appears.
3. Switch character coding to Japanese (EUC-JP)
4. Verify that green theme is no longer applied.

This test case could be reduced further.

Another observation regarding pim_css.js:  When I used Jesse Ruderman's
bookmarklet for viewing scripts
(http://www.squarefree.com/bookmarklets/webdevel.html) on the buggy page in the
Japanese (EUC-JP) coding, the backslashes used in pim_css.js to escape double
quotes in strings appeared as a weird character I didn't recognize.  In
US-ASCII coding the backslashes appeared correctly.
Original report on a newsgroup & my own investigation indicates this bug is not
present in Moz 1.4 or 1.4.1 but is present in 1.5 alpha.  That should narrow it
down.

I tried to find some possibilities:

bug 206780 was playing with font settings & a patch was checked in 7/19. 
apparently there was at least 1 problem with this original patch.  perhaps this
is a second.  If I could find an older build, I'd check it out.

bug 211440: mentioned "font" and "color", so why not.
Attachment #134592 - Attachment mime type: application/octet-stream → application/zip
Attached file JS for testcase (obsolete) — 

        Attachment #134592 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase (obsolete)
        — 
      

    


  

        Attachment #133454 -
        Attachment is obsolete: true

        Attachment #134592 -
        Attachment is obsolete: false

    
    

    
  
Comment on attachment 134718 [details]
JS for testcase

This needs more work...

        Attachment #134718 -
        Attachment is obsolete: true

        Attachment #134719 -
        Attachment is obsolete: true

    
    

    
  
Actually, if you load the testcase in attachment 134719 [details], you get an alert.  Now
if you select "EUC-JP" from the charset section of the view menu, you do NOT get
an alert.  In fact, the GetUnicodeDecoderRaw() call (with "euc-jp" as
characterSet.get()) on line 839 of nsScriptLoader.cpp fails, and we don't load
the script at all.

The problem, of course, is that the charset on the document is "euc_jp", not
"EUC_JP" (which is what it should be).  I bet the UI is passing in strings in
the wrong case, but I can't figure out where that menu is actually constructed.

In any case, I am seeing this bug on Linux, both with my testcase and with
David's (and thanks a ton for creating a testcase that could be used to
reproduce this, David!)
Assignee: general → smontagu
Component: Browser-General → Internationalization
Keywords: qawanted → testcase
OS: MacOS X → All
QA Contact: general → amyy
Hardware: Macintosh → All

    
    

    
  
It's strange. With 1.5, I can't reproduce the bug. I did get 'alert' (in JS)
even with charset set to EUC-JP (in View | Character Coding). David's test case
didn't change color at all when I changed Character Coding. It always remains
'green'(lime) (never blue). 



    
    

    
  
jshin, could you test with current trunk instead of 1.5?  There have been some
changes to nsScriptLoader since 1.5, and it could well be that one of them is
responsible...

    
    

    
  
I was able to reproduce the bug. It's not due to any recent check-in. It's been
present since May/June. 

The UI hands over non-canonical names ONLY when 'charset' is selected from
View|Character Coding | More. Once it's  added to the main list (via View |
Character Coding | Customize), the UI gives us the canonical name, which is why
I couldn't reproduce it at first.

 http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/locale/en-US/navigator.properties#25

An easy fix is to turn all non-canonical names to canonical ones in the above
properties file. Alternatives include fixing nsCharsetMenu.cpp (or related
files) and making sure that mCharacterSet member of nsDocument always store
canonical names only. Or, we may just use GetUnicodeDecoder instead of
GetUnicodeDecoderRaw in nsScriptLoader.cpp.

    
    

    
  
That last fix may be best (and get rid of all that charset alias crud that
happens in there...)  But there are other places that assume that
nsIDocument::GetDocumentCharacterSet is canonical, and the API claims that it
always will be.  I don't know whether SetDocumentCharacterSet should handle
canonicalization; we should decide on that, document nsIDocument accordingly,
and then fix either nsDocument or the callers as needed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        a patch (obsolete)
        — 
          — Splinter Review
      

    


  
As bz suggested, I got rid of alias resolution and replace GetUnicodeDecoderRaw
with GetUnicodeDecoder in nsScriptLoader.cpp

    
    

    
  
Note that the charset selector still needs fixing per comment 18...

    
    

    
  
Oh, and can't you eliminate the charset temporary?

    
    

    
  
Sure, I can get rid of temp's. 

re the second half of comment #18

I'm not sure whether I should fix SetDocumentCharacterSet, either. It appears
that it's kinda waste to do that because most of time it's handed a
canonicalized charset (I believe). 

I'm wondering why non-canonical names are passed thru ONLY for View |
Char.Coding | More (as I wrote in comment #17). nsCharsetMenu.cpp doesn't seem
to do anything special...


    
    

    
  


  
nsAString has to be used for |GetCharset| of nsIDOMElement(?) so that I can't
help using one temp.  Perhaps, we have to fix nsIDomElement(?):GetCharset and
its callers in a separate bug. 
  
This, alone, will fix this bug. 

bz, do you want to deal with nsIDocument issue here as well? I guess we'd
better file a new bug for that. I also filed bug 224748 for non-canonical names
in our resource files.

        Attachment #134772 -
        Attachment is obsolete: true

    
    

    
  
I think we should just document the nsIDocument getter and setter to only take
canonical names (and maybe add asserts to that effect in debug builds).

    
    

    
  
My prefs.js has the following:

user_pref("intl.charset.default", "EUC-KR");
user_pref("intl.charset.detector", "");
user_pref("intl.charsetmenu.browser.cache", "iso-8859-2, euc-jp, windows-1252, i
so-2022-cn");
user_pref("intl.charsetmenu.browser.static", "ISO-8859-1, EUC-KR, ISO-8859-15, U
TF-8, gb18030, GB2312, HZ-GB-2312, ISO-8859-16, x-mac-cyrillic");
user_pref("intl.charsetmenu.mailview.cache", "GB2312, ISO-8859-2, ISO-2022-JP, w
indows-1252, x-windows-949");

Some charset names are canonical while others are not[1]. It turned out that
it's not 'More' vs 'Static' but when they're added to the static list. By
'when', I mean whether they're added to my static list before May/June (when
GetUnicodeDecoderRaw was introduce) or after. Creating a new profile validated
my theory. So, we need to fix bug 224748 (nsCharsetMenu.cpp as well) not for
this bug but for our 'sanity' :-)


[1] Not all canonical names are in caps. However, 'euc-jp' and 'iso-8859-2'
should be 'EUC-JP' and 'ISO-8859-2'.

    
    

    
  


  
I added assert and documented the requirement for canonical names. I didn't add
assert to GetDocumentCharacterSet.

        Attachment #134815 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 134843 [details] [diff] [review]
a new patch (with assertion in Set..Char..Set in debug build)

asking for r/sr

        Attachment #134843 -
        Flags: superreview?(bz-vacation)

        Attachment #134843 -
        Flags: review?(smontagu)

    
    

    
  
Comment on attachment 134843 [details] [diff] [review]
a new patch (with assertion in Set..Char..Set in debug build)

Looks great.

        Attachment #134843 -
        Flags: superreview?(bz-vacation) → superreview+

    
    

    
  
Comment on attachment 134843 [details] [diff] [review]
a new patch (with assertion in Set..Char..Set in debug build)

>+#ifdef DEBUG
>+    nsCOMPtr<nsICharsetAlias> calias(do_GetService(kCharsetAliasCID));
>+    if (calias) {
>+      nsCAutoString canonicalName;
>+      calias->GetPreferred(aCharSetID, canonicalName);
>+      NS_ASSERTION(canonicalName.Equals(aCharSetID),
>+                   "canonical name should be passed");
>+    }
>+#endif

Nit: can you make that message clearer? e.g. "charset name must be canonical".
r=smontagu

        Attachment #134843 -
        Flags: review?(smontagu) → review+

    
    

    
  
Thanks for r/sr. Fix checked into the trunk with the assertion change.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: