33032 - Display Unicode space characters as space

Status: RESOLVED FIXED

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

Description

[Erik van der Poel]

Taken from the following bug:

  http://bugzilla.mozilla.org/show_bug.cgi?id=6585

+ ------- Additional Comments From [e-mail]  2000-03-23 09:40 -------
+ However, we should do special process for
+ "space" characters in the range of U+2000-U+206F. (some of them, not all of
+ them). There are no reason we cannot "render"/"measure" a space characters, 
even
+ non of the font have these glyph (a glyph for a space character is simply a
+ matrix without any countor....)

Comment 1

[Frank Tang]

Change the summary to 
Display Unicode space characters as space
Should we split this to 3 bugs. one for each platform ?

Comment 2

[Frank Tang]

Mark M24 and reassign to bstell for linux part 

Comment 3

[Gervase Markham [:gerv]]

bstell: This bug seems to have acquired 37 votes. There is obviously demand here
:-) Is it on your radar at all?

Gerv

Comment 4

[kill this account]

ftang: can you set the priority on this?

Comment 5

[kill this account]

--> ftang

Comment 6

[Frank Tang]

bulk move NEW FUTURE bug to ASSIGN

Comment 7

[Frank Tang]

over to shanjian

Comment 8

[Shanjian Li]

accepting. 
We can probably do something in subsititute font. 

Comment 9

[Shanjian Li]

Does anybody still care about this bug? If the evolution of font on various
systems make this bug unnecessary, I will close it. If not, please post your
comment and a test case. 

Comment 10

[Alexey Chernyak]

*** Bug 215700 has been marked as a duplicate of this bug. ***

Comment 11

[Alexey Chernyak]

Attachment: UTF-8 plaintext testcase with paragraph and line separator characters

testcase from bug 215700

Actual Results:  
It shows a single line of text that looks like this:
First paragraph.?Second paragraph?Third paragraph, first line.?Third paragraph,

second line?


Expected Results:  
It should instead display as:

First paragraph.
Second paragraph
Third paragraph, first line.
Third paragraph, second line

Comment 12

[Jungshik Shin]

We have to deal with some of them (Unicode paragraph separator, line separator,
etc)  an XP-way in nsTextTransformeran while others have to be dealt with in
each Gfx port (em space, en space, quad-space?, etc). I guess em/en/quad spaces
are already handled 'correctly' by our trasliterator.

Comment 13

[Frank Tang]

shanjian is no longer working on mozilla for 2 years and these bugs are still
here. Mark them won't fix. If you want to reopen it, find a good owner first. 

Comment 14

[Travis Chase]

Mass Reassign Please excuse the spam

Comment 15

[Travis Chase]

Mass Re-opening Bugs Frank Tang Closed on Wensday March 02 for no reason, all
the spam is his fault feel free to tar and feather him

Comment 16

[Travis Chase]

Reassigning Franks old bugs to Jungshik Shin for triage - Sorry for spam

Comment 17

[Brandon Mitchell]

>I guess em/en/quad spaces are already handled 'correctly' by our trasliterator.

Would fixing this fix Bug 551734 - Unicode hair space (U+200A) is too wide if style="font: x-small sans-serif" (has test case)?

Comment 18

[Jonathan Kew [:jfkthame]]

Taking this bug - I think I can cook up a patch to make things work better.

(In reply to comment #17)
> >I guess em/en/quad spaces are already handled 'correctly' by our trasliterator.
> 
> Would fixing this fix Bug 551734 - Unicode hair space (U+200A) is too wide if
> style="font: x-small sans-serif" (has test case)?

That may depend on your system and browser font configuration. Specifically, it would depend whether the sans-serif font concerned actually has a (too-wide) glyph for U+200A. If so, we'll still use that glyph; but if what's happening is that the font does _not_ support U+200A, and so you're seeing fallback to a different font that is too wide in comparison, then I think we can fix that.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: patch, synthesize various Unicode spaces rather than falling back to other font or missing-glyph rendering

This resolves bug 653911 on my Android device, in addition to avoiding potentially expensive font fallback.

We could omit the part of this patch that short-circuits font search (in gfxFontGroup::FindFontForChar), and _only_ use the "fake" spaces in gfxFontGroup::InitScriptRun if font-matching fails to find any font at all (as happens on Android), but I don't see any good reason to fall back through the font list and system fonts for these characters; if they're not supported in the first-choice font, we might as well go directly to the option of using a standard space glyph with an appropriately-modified width.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: reftest for synthetic Unicode space characters

This tests a few of the defined-width spaces in the U+200x range to check that they render as the expected amount of space even when the current font does not include them.

Comment 21

[John Daggett (:jtd)]

Comment on attachment 529341 [details] [diff] [review]
patch, synthesize various Unicode spaces rather than falling back to other font or missing-glyph rendering

>      // 1. check fonts in the font group
>      for (PRUint32 i = 0; i < FontListLength(); i++) {
>          nsRefPtr<gfxFont> font = GetFontAt(i);
> -        if (font->HasCharacter(aCh))
> +        if (font->HasCharacter(aCh)) {
>              return font.forget();
> +        }
> +        if (i == 0 &&
> +            gfxUnicodeProperties::GetGeneralCategory(aCh) ==
> +                HB_CATEGORY_SPACE_SEPARATOR)
> +        {
> +            return nsnull;
> +        }
>      }

I really don't think we should apply this at this level, if we're going to do this we should apply it after pref fonts are referenced, before system font fallback occurs.  And doesn't this effectively apply to any codepoint with category == Zs?  That would appear to include to include characters like normal
spaces, full-width spaces:

  0020;SPACE;Zs;0;WS;;;;;N;;;;;
  00A0;NO-BREAK SPACE;Zs;0;CS;<noBreak> 0020;;;;N;NON-BREAKING SPACE;;;;
  1680;OGHAM SPACE MARK;Zs;0;WS;;;;;N;;;;;
  180E;MONGOLIAN VOWEL SEPARATOR;Zs;0;WS;;;;;N;;;;;
  205F;MEDIUM MATHEMATICAL SPACE;Zs;0;WS;<compat> 0020;;;;N;;;;;
  3000;IDEOGRAPHIC SPACE;Zs;0;WS;<wide> 0020;;;;N;;;;;
  
Maybe I'm not guessing correctly as to how GetGeneralCategory functions (reading the code in gfxUnicodePropertyData.cpp certainly doesn't help!).  Won't this patch also affect shaping behavior for Mongolian if U+180e is used, since with this patch we won't hit system fallback for that character?

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: patch, v2 - synthesize unicode spaces - less aggressive approach

Argh, yes, I was initially thinking I'd do _all_ the GC=Zs characters, but then backed off a bit from that as some of them are less clear-cut.

Here is an alternative version that only steps in before the final system-fallback search, and only for the characters that GetUnicodeSpaceWidth actually knows about.

Comment 23

[John Daggett (:jtd)]

Comment on attachment 529434 [details] [diff] [review]
patch, v2 - synthesize unicode spaces - less aggressive approach

+gfxFont::GetUnicodeSpaceWidth(PRUint32 aCh)

A better name here would be 'SynthesizeSpaceWidth', since you're not really "getting" the width in the sense of fetching it from the font.

+    // for known "space" characters, don't do a full system-fallback search;
+    // we'll synthesize appropriate-width spaces instead of missing-glyph boxes
+    if (GetFontAt(0)->GetUnicodeSpaceWidth(aCh) >= 0.0) {
+        return nsnull;
+    }

The switch statement in GetUnicodeSpaceWidth is relatively expensive, I think it might be better to use:

if (gfxUnicodeProperties::GetGeneralCategory(aCh) == HB_CATEGORY_SPACE_SEPARATOR && 
    GetFontAt(0)->GetUnicodeSpaceWidth(aCh) >= 0.0)
{
   ...
}

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to comment #23)
> A better name here would be 'SynthesizeSpaceWidth'

OK, I like that.

> The switch statement in GetUnicodeSpaceWidth is relatively expensive, I think
> it might be better to use:
> 
> if (gfxUnicodeProperties::GetGeneralCategory(aCh) ==
> HB_CATEGORY_SPACE_SEPARATOR && 
>     GetFontAt(0)->GetUnicodeSpaceWidth(aCh) >= 0.0)
> {
>    ...
> }

I'm not sure it matters much once we're down to this point in FindFontForChar, but FWIW I've included this, as GetGeneralCategory is designed to be a very fast table lookup.

Pushed:
http://hg.mozilla.org/mozilla-central/rev/9b312736f4ca