Closed
Bug 451426
Opened 16 years ago
Closed 16 years ago
Incorrect rendering of some Japanese characters when using downloadable fonts (@font-face)
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b3
People
(Reporter: a-yoshi, Assigned: jfkthame)
References
Details
(Keywords: fixed1.9.1)
Attachments
(3 files, 2 obsolete files)
(deleted),
text/html; charset=Shift_JIS
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080818032051 Minefield/3.1a2pre
Build Identifier:
Some Japanese characters were rendered incorrectly when using downloadble fonts.
This is how downloadable fonts work. At first, when the fonts are not downloaded, text will be rendered using default font. But once the download finishes, they will be rendered again using the downloaded font. The problem is that on this second rendering, some Japanese characters are rendered incorrectly, when downloaded fonts does not contain Japanese characters within it. Japanese characters should be rendered using default Japanese fonts (since the downloaded font does not contain Japanese characters), but it seems that they are using Chinese fonts instead of using Japanese fonts.
It would be easier to tell the difference if you compare this text with the same text without applying the downloadable fonts.
This only happens when you applied the patch v.0.8b for bug 441473 to the build tree.
Reproducible: Always
Steps to Reproduce:
1. Apply patch v.0.8b from bug 441473 and build
2. Make an HTML file in Japanese encoding (Shift_JIS, EUC-jp)
3. Write a text that contains Japanese characters in between Chinese characters and apply downloadable font to the text (make sure the font you download does not contain Japanese characters)
4. Open the HTML file from browser, and when the download is done, Japanese characters would be rendered incorrectly
Actual Results:
Japanese characters are rendered incorrectly, probably using Chinese fonts.
Expected Results:
Japanese characters should be rendered using Japanese fonts.
Reporter | ||
Updated•16 years ago
|
Comment 1•16 years ago
|
||
I'm not sure whether we generally want bugs against stuff you get when applying patches in other bugs, but for something this big it may be useful.
Assignee: nobody → jdaggett
(In reply to comment #1)
> I'm not sure whether we generally want bugs against stuff you get when applying
> patches in other bugs, but for something this big it may be useful.
>
Akira and I hemmed and hawed over just that issue until J. Daggett asked us to write it up: https://bugzilla.mozilla.org/show_bug.cgi?id=441473#c46
Comment 3•16 years ago
|
||
Yeah, normally I wouldn't ask that these be separated but the patch is fairly large and I didn't want these issues to get lost in the noise.
Comment 4•16 years ago
|
||
Akira, could you attach a testcase for this?
Reporter | ||
Comment 5•16 years ago
|
||
Last 8 characters of each sentence are Chinese characters. Among them, 3rd, 7th and 8th character are rendered thinner compared to other characters. It would be more obvious if you can compare the same sentence using .test { font-family: "Comic Sans MS"; } instead of using downloadable fonts.
Also, once you loaded this page, please focus on some other window. If you focus again of firefox window after about 1 minute, Japanese characters (フォントの拡張子って) in the middle of the sentence will suddenly be rendered in a different way. This only happens to the first sentence which is using downloadable font.
Flags: wanted1.9.1? → blocking1.9.1+
Priority: -- → P3
Updated•16 years ago
|
Attachment #335726 -
Attachment mime type: text/html → text/html; charset=Shift_JIS
Comment 6•16 years ago
|
||
Akira, not sure I see the problem here. If this problem still occurs, could you post a screenshot, along with the exact set of steps you went through to get to that state?
I see one line rendered in Comic Sans, one line rendered in the default font. In both cases fallback occurs for Japanese, Chinese and Korean characters. Given the defined styles, this appears correct.
Reporter | ||
Comment 7•16 years ago
|
||
(In reply to comment #6)
I cannot reproduce the bug right now. That is because I've finished my internship and I don't have a Mac OS build now (I forgot to mention, but this problem only occurs on Mac OS). I'm sorry that I cannot attach the screenshot.
If you could compare the two following test pages using reftest, the bug should reproduce. I'm sure it reproduced on patch v.0.9h. I hope you can see the difference.
Test case for @font-face rules v.0.6 (bug 452780)
https://bugzilla.mozilla.org/attachment.cgi?id=338203
In this attached test suite, you can find the following test cases.
downloadable-font-japanese.html - using downloadable font (since it's using url:local(), it uses the system font as a consequence)
downloadable-font-japanese-ref.html - using system font (ComicSansMS)
> I see one line rendered in Comic Sans, one line rendered in the default font.
> In both cases fallback occurs for Japanese, Chinese and Korean characters.
> Given the defined styles, this appears correct.
Yes, the fallback should occur. The point is at the last 8 characters (ABCDEFGH) in the sentence using Comic Sans. The 3rd, 7th and 8th characters should be rendered using Japanese fonts, since the encoding is specified as Shift_JIS. But when you use the @font-face rules, they are rendered using Chinese fonts. When you use the normal .test { font-family: "Comic Sans MS"; } rules, like in downloadable-font-japanese-ref.html, these characters are rendered using Japanese fonts.
Assignee | ||
Comment 8•16 years ago
|
||
Screenshot showing the incorrect rendering of Japanese characters, created with current Minefield build on Mac OS X (10.5.5). The first line uses @font-face to access Bitstream Vera Sans; the 3rd, 7th and 8th Japanese characters are rendered from a different fallback font than the rest. The second line of the sample specifies Bitstream Vera Sans directly using font-family: "Bitstream Vera Sans", and does not show the problem. (The font difference is most obvious in the final character.)
I observed that if I use Zoom In or Zoom Out to change the size of the displayed text, both lines render correctly; subsequently using Reload to refresh the page brings back the problem with those three characters on line 1.
The exact set of characters that are rendered incorrectly is variable; on some occasions, especially when switching back into Minefield after activating another application for some time, I have seen a different fallback font used for the first Japanese character as well. This behavior seems a bit unpredictable, however.
Safari on the same machine consistently renders both lines correctly (matching line 2 of the Minefield display).
Font selection happens in gfxFontGroup::ComputeRanges:
http://mxr.mozilla.org/mozilla-central/search?string=gfxfontgroup%3A%3Acomputeranges
Assignee | ||
Comment 10•16 years ago
|
||
Due to an uninitialized flag, there is a strong probability that in the case where a font is found in the mUserFontSet, gfxFontGroup::ForEachFontInternal will exit early while resolving font families (at line 1033 of gfxFont.cpp).
In the test case given, this was leading to a truncated FontList containing only the downloaded font, and not the language-appropriate font for the generic sans-serif family; subsequently, gfxFontGroup::FindFontForChar would fall back to WhichPrefFontSupportsChar() rather than finding the appropriate Japanese font from the active font group.
With this patch, which ensures that aborted is set to PR_FALSE until specifically used, the test document renders correctly and consistently. (An uninitialized local variable being the cause is also consistent with the observation that the behavior was not 100% predictable, and could vary with changing focus and redrawing of the frame even within a single run of the program.)
NB: while investigating this, I was also puzzled by lines 1124-1126 of gfxFont.cpp:
// if match, return
if (selectedFont)
return selectedFont.forget();
I don't see how selectedFont can have any useful value at this point in the function. Might this be an obsolete fragment of code that ought to be removed?
Attachment #346261 -
Flags: superreview?
Attachment #346261 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #346261 -
Flags: superreview?(roc)
Attachment #346261 -
Flags: superreview?
Attachment #346261 -
Flags: review?(roc)
Attachment #346261 -
Flags: review?
Comment 11•16 years ago
|
||
Patch looks good.
> NB: while investigating this, I was also puzzled by lines 1124-1126 of
> gfxFont.cpp:
>
> // if match, return
> if (selectedFont)
> return selectedFont.forget();
>
> I don't see how selectedFont can have any useful value at this point in the
> function. Might this be an obsolete fragment of code that ought to be removed?
This looks obsolete, feel free to trim it out.
One small nit, could you modify your .hgrc file to include the lines below, as noted in the Mercurial FAQ https://developer.mozilla.org/en/Mercurial_FAQ
[diff]
git = 1
[defaults]
diff=-p -U 8
This will give your diff's more context.
Attachment #346261 -
Flags: superreview?(roc)
Attachment #346261 -
Flags: superreview+
Attachment #346261 -
Flags: review?(roc)
Attachment #346261 -
Flags: review+
Comment on attachment 346261 [details] [diff] [review]
fix for uninitialized variable that caused premature return when resolving fonts
great!
Assignee | ||
Comment 13•16 years ago
|
||
Revised patch using preferred diff settings. Also eliminated the obsolete code mentioned in comment #10, although I believe that code was harmless in practice because of nsRefPtr's initialization to null.
Attachment #346261 -
Attachment is obsolete: true
Blocks: 452870
Attachment #346320 -
Flags: superreview+
Attachment #346320 -
Flags: review+
Assignee: jdaggett → jfkthame
Keywords: checkin-needed
Assignee | ||
Comment 14•16 years ago
|
||
See bug 452870#c17 for reftest related to this bug.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Updated•16 years ago
|
Whiteboard: [needs landing]
Updated•16 years ago
|
Whiteboard: [needs landing]
Version: unspecified → Trunk
Whiteboard: [needs landing]
Assignee | ||
Comment 15•16 years ago
|
||
Attachment #346320 -
Attachment is obsolete: true
Comment 17•16 years ago
|
||
Landed on mozilla-central (and thus also on mozilla-1.9.1):
http://hg.mozilla.org/mozilla-central/rev/77825d347650
It turns out that this made a bunch of reftests unexpectedly pass, showing that this fixed bug 465409:
http://hg.mozilla.org/mozilla-central/rev/a036f2285660
I still need to investigate whether it also fixed bug 465408.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.1b3
Comment 19•16 years ago
|
||
It also fixed bug 465408 and the test that was random on x86-Mac, so I adjusted the reftest.list appropriately; there are now no longer any random tests in reftests/font-face/reftest.list :
http://hg.mozilla.org/mozilla-central/rev/e29be5f4f686
Updated•16 years ago
|
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•