Closed Bug 546013 Opened 10 years ago Closed 10 years ago

Text rendering of unsupported characters should conform to http://unicode.org/faq/unsup_char.html

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: usenet, Assigned: emk)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

The core graphics text rendering fails to conform to http://unicode.org/faq/unsup_char.html ; see bug 289588 for an example with a test case that tests just a single character from that repertoire, and fails.

This problem manifests itself on both Linux and Windows; the test case in bug 289588 does not show any problems, but is not an exhaustive test of all the cases covered by the relevant parts of the Unicode spec.
Depends on: 289588
Summary: Text rendering should conform to http://unicode.org/faq/unsup_char.html → Text rendering of unsupported characters should conform to http://unicode.org/faq/unsup_char.html
the line above should read "the test case in bug 289588 does not show any problems in Mac OS X"
Blocks: unicode
Text layout issue, not graphics.
Component: Graphics → Layout: Text
QA Contact: thebes → layout.fonts-and-text
Only gfx knows which characters are supported, so this would be most easily done in gfx.
While there may be some characters that we want to special-case, it is not possible for gfx to reliably determine whether a given character code will be "correctly" rendered by the font chosen. We can - and do - detect the case when there is no font available that supports the character.

In the specific case of the Hangul "fillers", we might want to ensure that they are drawn with the same font as the surrounding text, and fall back to hexboxes rather than switching font ONLY for the filler characters. But more generally, if a font has blank (or incorrect) glyphs for what should be visible characters, that's simply a font bug.
According to http://unicode.org/faq/unsup_char.html , these characters should not be displayed under any circumstances "if not explicitly supported in rendering". 

Whether that has anything to do with whether or not a particular font contains a glyph for that character is a matter of interpretation; from a security viewpoint, I'd certainly want the behavior for default-ignorable characters to be the same across all fonts, which requires it to be enforced it at some level above the font itself.
Blocks: 315728
Blocks: 552460
Taking.
Status: NEW → ASSIGNED
QA Contact: layout.fonts-and-text → VYV03354
Attached patch patch (gfx part) (obsolete) — Splinter Review
Ignore default ignorable chars instead of drawing hexbox
Assignee: nobody → VYV03354
Attachment #438261 - Flags: review?(jfkthame)
Attached patch patch (layout part) (obsolete) — Splinter Review
Adding reflow notification on pref changing.
Pref change may cause a width of text.
Attachment #438261 - Attachment is obsolete: true
Attachment #438262 - Flags: review?(roc)
Attachment #438261 - Flags: review?(jfkthame)
Attached patch patch (browser part) (obsolete) — Splinter Review
We need to encode default ignorable characters on url bar to prevent spoofing.
Attachment #438263 - Flags: review?(gavin.sharp)
Comment on attachment 438261 [details] [diff] [review]
patch (gfx part)

Accidentaly obsoleted, sorry.
Attachment #438261 - Attachment is obsolete: false
Attachment #438261 - Flags: review?(jfkthame)
QA Contact: VYV03354 → layout.fonts-and-text
I took default ignorable characters list from
http://www.unicode.org/Public/UNIDATA/DerivedCoreProperties.txt
Why do we need a pref here? Shouldn't we just do the right thing?
Attachment #438261 - Flags: review?(jfkthame) → review-
Comment on attachment 438261 [details] [diff] [review]
patch (gfx part)

>+    /**
>+     * Whether to display default ignorable code points
>+     */
>+    static PRBool ShowIgnorables();

As roc says, why do we need a preference? Is there a significant use-case for it? If not, a substantial part of the code below can be omitted (and less code is a good thing!)

>                 if (glyphData->IsMissing()) {
>-                    if (!aDrawToPath) {
>+                    if (!aDrawToPath && advance > 0) {
>                         double glyphX = x;

Please add a comment to say that default-ignorable chars will have had the advance set to zero, so that in future we'll remember what's being done here....

>+    if (!gfxPlatform::ShowIgnorables() && IsDefaultIgnorable(aChar)) {
>+        details->mAdvance = 0;
>+    } else {
>+        gfxFloat width = PR_MAX(glyphRun->mFont->GetMetrics().aveCharWidth,
>+                                gfxFontMissingGlyphs::GetDesiredMinWidth(aChar));
>+        details->mAdvance = PRUint32(width*GetAppUnitsPerDevUnit());
>+    }

....and here, a comment noting that mAdvance = 0 will prevent us drawing the hexbox.

>+++ b/gfx/thebes/src/ignorable.x-ccmap
....
>+ * The Initial Developer of the Original Code is
>+ * Jungshik Shin <jshin@mailaps.org>
>+ * Portions created by the Initial Developer are Copyright (C) 2003
>+ * the Initial Developer. All Rights Reserved.

Is the developer name and copyright date here REALLY correct?!

Marking r- because we need to decide about the need for a preference; aside from that, though, the gfx code here seems fine.
Attached patch resolved review comments (obsolete) — Splinter Review
(In reply to comment #13)
> As roc says, why do we need a preference? Is there a significant use-case for
> it? If not, a substantial part of the code below can be omitted (and less code
> is a good thing!)
I added the pref for debugging purpose. I couldn't find any other practical use-case, so removed.

> >                 if (glyphData->IsMissing()) {
> >-                    if (!aDrawToPath) {
> >+                    if (!aDrawToPath && advance > 0) {
> >                         double glyphX = x;
> 
> Please add a comment to say that default-ignorable chars will have had the
> advance set to zero, so that in future we'll remember what's being done
> here....
> 
> >+    if (!gfxPlatform::ShowIgnorables() && IsDefaultIgnorable(aChar)) {
> >+        details->mAdvance = 0;
> >+    } else {
> >+        gfxFloat width = PR_MAX(glyphRun->mFont->GetMetrics().aveCharWidth,
> >+                                gfxFontMissingGlyphs::GetDesiredMinWidth(aChar));
> >+        details->mAdvance = PRUint32(width*GetAppUnitsPerDevUnit());
> >+    }
> 
> ....and here, a comment noting that mAdvance = 0 will prevent us drawing the
> hexbox.
Added.

> >+++ b/gfx/thebes/src/ignorable.x-ccmap
> ....
> >+ * The Initial Developer of the Original Code is
> >+ * Jungshik Shin <jshin@mailaps.org>
> >+ * Portions created by the Initial Developer are Copyright (C) 2003
> >+ * the Initial Developer. All Rights Reserved.
> 
> Is the developer name and copyright date here REALLY correct?!
It was generated by ccmapbin.pl. Fixed.
Attachment #438261 - Attachment is obsolete: true
Attachment #438262 - Attachment is obsolete: true
Attachment #439285 - Flags: review?(jfkthame)
Attachment #438262 - Flags: review?(roc)
Attached patch patch (browser part) v2 (obsolete) — Splinter Review
ECMA-262 RegExp didn't support non-BMP Unicode literals. Changed to surrogate pairs.
Attachment #438263 - Attachment is obsolete: true
Attachment #439286 - Flags: review?(gavin.sharp)
Attachment #438263 - Flags: review?(gavin.sharp)
Sorry, wrong patch attached.
Attachment #439285 - Attachment is obsolete: true
Attachment #439285 - Flags: review?(jfkthame)
Attachment #439287 - Flags: review?(jfkthame)
Attachment #439287 - Flags: review?(jfkthame) → review+
Comment on attachment 439286 [details] [diff] [review]
patch (browser part) v2

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  // Encode defult ignorable characters. (bug 546013)

typo: default
Attachment #439286 - Flags: review?(gavin.sharp) → review+
Fixed typo. Thank you!
Attachment #439286 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to comment #18)
> Created an attachment (id=441811) [details]

Similar patch seemd to be needed for Seamonkey.
(/suite/browser/navigator.js)
http://hg.mozilla.org/mozilla-central/rev/068276123219
http://hg.mozilla.org/mozilla-central/rev/068276123219

Leaving open until we have a bug for the SeaMonkey issue.
(It looks like perhaps there should be some service in core to do this rather than needing to keep up to date with Unicode changes.  I wonder whether nsIDNService is useful.)
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a5
Blocks: 455455
Marking fixed as bug 467530 tracks that this is wanted for SeaMonkey.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Can you add a reference to ignorable.x-ccmap to https://wiki.mozilla.org/I18n:Updating_Unicode_version? Thanks!
No longer blocks: FF2SM
(In reply to comment #21)
> Marking fixed as bug 467530 tracks that this is wanted for SeaMonkey.

I filed bug 562845.
Depends on: 562908
Comment on attachment 442398 [details]
Perl script to generate ignorable.x-ccmap and regular expression

This script has beed added on the tree.
Attachment #442398 - Attachment is obsolete: true
Depends on: 582186
You need to log in before you can comment on or make changes to this bug.