288439 - Justify space should be added both side of justifiable char

Status: RESOLVED DUPLICATE of bug 1063857

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

On CJ justification layout, the previous charachter of CJ justifiable charachter
should be justifiable.

testcase1
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2657&action=view
screenshot of textcase1
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2658&action=view

testcase2
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2659&action=view

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.1(work in progress)

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I added the "Extra Justifiable Character" in the patch.
That means that if the character is single, it is not a justifiable character.
but if the character is before *CJ* justifiable character, it becomes extra
justifiable character.

For example.
<p lang="ja"><span lang="en>a bc</span> def</p>

"c" is before the space that is justifiable character and is Japanese lang.
In this time, "c" is Extra Justifiable Character.
"a" is before the space. But this space is justifiable character and is *not*
Japanese lang and Chinese lang(that is english).
In this time, "a" is not Extra Justifiable Character.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Could we avoid this problem another way ... by putting half the justification
space for a character on one side of the glyph, and half on the other side?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Robert:

I should change gfx on All platforms?
e.g., On Windows
http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsRenderingContextWin.cpp#2207

2233       x = data->mX;                                    <---- Here?
2234       y = data->mY;
2235       data->mTranMatrix->TransformCoord(&x, &y);
2236       if (IS_HIGH_SURROGATE(*str) && 
2237           ((str+1)<end) && 
2238           IS_LOW_SURROGATE(*(str+1))) 
2239       {
2240         // special case for surrogate pair
2241         fontWin->DrawString(data->mDC, x, y, str, 2);
2242         // we need to advance data->mX and str twice
2243         data->mX += *data->mSpacing++;
2244         ++str;
2245       } else {
2246         fontWin->DrawString(data->mDC, x, y, str, 1);
2247       }
2248       data->mX += *data->mSpacing++;
2249       ++str;

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops..

Comment 5 is wrong way.
In this case, the first character of the line is not rendered correctly.
Umm... I should add the parameter "const nscoord* aPreSpacing" for |DrawString|?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that if we can fix this bug, we can implement text-autospace property in
bug 289130.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 182956 [details] [diff] [review]
Patch rv2.0

How about this?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1

fix the bustage on Linux with GTK.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.2

fix the bustage of cairo on Linux.

I tested on following toolkit and OS.
Win32, GTK without Xft, GTK2 with Xft, GTK2 with cairo and xlib.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.3

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 183183 [details] [diff] [review]
Patch rv2.3

I tested Qt too.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Roc:

I want your comment that the patch's approach is good or not good.
If you grant the approach, I will go to next step(test and separate the patch by
reviewer).

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.4

fix the bug of pango.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know why the aCharOffset array is needed. It seems like you could just
add extra space to the aSpacing array and adjust the initial aX parameter.

Furthermore, I wonder if the approach of associating justification space with
glyphs is correct. For example, if we have two CJK characters "J" and one
non-CJK letter "R", then given
RJJR
should we really have twice as much space between "JJ" as between "RJ"? Maybe a
better approach would be to identify points between glyphs where justification
space can be inserted, and evenly distribute the space among those points.

Masayuki, I think we should put this bug off until 1.9, and probably until we
have the new Gfx layer (i.e., mostly cairo only). Then this kind of thing will
be much easier to fix (and much less risky). Perhaps we can work out a better
way to handle all this processing. For example, we could have an API that asks
the native i18n font layer (Pango or Uniscribe or ATSUI) to convert the Unicode
text into a string of glyphs with information about each glyph (the glyph's
position, and whether space can be inserted before or after the glyph for
letter-spacing, word-spacing, justification and even line-breaking). Then we
could process the glyph string, repositioning glyphs according to the spacing.

Comment 17

[Simon Montagu :smontagu]

(In reply to comment #16)
> Perhaps we can work out a better
> way to handle all this processing. For example, we could have an API that asks
> the native i18n font layer (Pango or Uniscribe or ATSUI) to convert the Unicode
> text into a string of glyphs with information about each glyph (the glyph's
> position, and whether space can be inserted before or after the glyph for
> letter-spacing, word-spacing, justification and even line-breaking). Then we
> could process the glyph string, repositioning glyphs according to the spacing.

FYI, I'm working on something like this at the moment, which I hope will be
ready for the beginning of the 1.9 cycle. I don't see any other way to handle
justification of text with ligatures and combining characters.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See bug 157967 and http://wiki.mozilla.org/index.php?title=Gecko2:NewTextAPI

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Maybe a
> better approach would be to identify points between glyphs where justification
> space can be inserted, and evenly distribute the space among those points.

It is first approach. In the way, we need next character always.(I.e., if a
character is end of text frame, we should get a first character of next text frame.)

> I think we should put this bug off until 1.9,

I agree it. I have targeted this to 1.9a.

> and probably until we
> have the new Gfx layer (i.e., mostly cairo only).

O.K.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

fantasa and Ian:

I'll work on this for better justify implementation after nsTextFrame is refactored. But I have a question for the spec, CSS2.1 said:

http://www.w3.org/TR/CSS21/text.html#propdef-letter-spacing
> User agents may not further increase or decrease the inter-character space in order to justify text.

That meanes we cannot add the extra space to any character for the justification if the element has non-zero letter-spacing. This is bad spec for intl justification(i.e., for Japanese and Chinese). And we are adding the extra space to the SPACE(U+0020) and NBSP(U+00A0) if the element has non-zero letter-spacing. So, if the element has non-zero letter-spacing, we cannot add extra space for justification on every character...

Now, Gecko uses the letter-spacing value as minimum inter-character space in justify layout. I think that this behavior is better than spec. What do you think about this?

Comment 21

[fantasai]

CSS3 Text will introduce min/max controls on letter-spacing [1], so there will be finer control over this aspect in CSS3. Disallowing inter-character justification when letter-spacing is a <length> has been specced since CSS1, so it is unlikely to change.. Although I do agree that it makes more sense to allow it, especially in non-Latin contexts.

I have not seen Mozilla do inter-character justification before. What would trigger it?

[1] http://www.w3.org/TR/css3-text/#letter-spacing

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #21)
> I have not seen Mozilla do inter-character justification before. What would
> trigger it?

I have implemented justification for Japanese and Chinese. In these cases, extra spaces for justification are added between ideograph characters. See bug 36322.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

should fix only on new text frame.
# it already fixes the almost of this bug, but I need a little work (comment 16).

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This was fixed by a fix of bug 1063857.