Switching character coding changes background color at Yahoo Mail

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: David Haas (gone, not reading mail), Assigned: smontagu)

Tracking

({testcase})

Trunk
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

14 years ago
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...
(Reporter)

Comment 2

14 years ago
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?
(Reporter)

Comment 4

14 years ago
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?
(Reporter)

Comment 6

14 years ago
Created attachment 133454 [details]
 "Web Page, Complete"

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

Comment 8

14 years ago
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.   

Comment 9

14 years ago
Created attachment 134592 [details]
Test case to reproduce yahoo bug

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.
(Reporter)

Comment 10

14 years ago
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
Created attachment 134718 [details]
JS for testcase
Attachment #134592 - Attachment is obsolete: true
Created attachment 134719 [details]
Testcase
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

Comment 15

14 years ago
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...

Comment 17

14 years ago
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.

Comment 19

14 years ago
Created attachment 134772 [details] [diff] [review]
a patch

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?

Comment 22

14 years ago
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...

Comment 23

14 years ago
Created attachment 134815 [details] [diff] [review]
a new patch with temp's removed (except for one)

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).

Comment 25

14 years ago
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'.

Comment 26

14 years ago
Created attachment 134843 [details] [diff] [review]
a new patch (with assertion in Set..Char..Set in debug build)

I added assert and documented the requirement for canonical names. I didn't add
assert to GetDocumentCharacterSet.
Attachment #134815 - Attachment is obsolete: true

Comment 27

14 years ago
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+
(Assignee)

Comment 29

14 years ago
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+

Comment 30

14 years ago
Thanks for r/sr. Fix checked into the trunk with the assertion change.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.