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: