Last Comment Bug 276079 - Implement text-justify property(but 'auto', 'inter-word', 'inter-ideograph' and 'distribute' only)
: Implement text-justify property(but 'auto', 'inter-word', 'inter-ideograph' a...
Status: ASSIGNED
[evang-wanted]
: css3, dev-doc-needed, intl
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- enhancement with 22 votes (vote)
: ---
Assigned To: Nazo
:
Mentors:
http://bugzilla.mozilla.gr.jp/attachm...
Depends on: grapheme-breaker
Blocks: css-text-3 css3test
  Show dependency treegraph
 
Reported: 2004-12-26 20:15 PST by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2016-06-08 04:50 PDT (History)
30 users (show)
roc: blocking1.9.2-
roc: wanted1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch(intl/) (1.33 KB, patch)
2004-12-26 20:21 PST, Nazo
no flags Details | Diff | Review
patch(dom/) (1.33 KB, patch)
2004-12-26 20:22 PST, Nazo
dbaron: review-
Details | Diff | Review
patch(layout/) (28.94 KB, patch)
2004-12-26 20:23 PST, Nazo
no flags Details | Diff | Review
patch(intl/) (6.51 KB, patch)
2004-12-26 20:38 PST, Nazo
no flags Details | Diff | Review
patch(layout/) (29.13 KB, patch)
2004-12-26 20:41 PST, Nazo
no flags Details | Diff | Review
patch(layout/) (29.13 KB, patch)
2004-12-26 21:08 PST, Nazo
no flags Details | Diff | Review
patch(layout/) (30.73 KB, patch)
2004-12-26 21:10 PST, Nazo
no flags Details | Diff | Review
patch(intl/) (4.90 KB, patch)
2004-12-26 21:11 PST, Nazo
no flags Details | Diff | Review
Sample (547 bytes, text/html)
2004-12-27 19:40 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details
patch(dom/) v2 (1.20 KB, patch)
2004-12-29 04:13 PST, Nazo
no flags Details | Diff | Review
patch(layout/) v1.1 (31.37 KB, patch)
2004-12-29 04:22 PST, Nazo
no flags Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-26 20:15:55 PST
Implement CSS3 text-justify property.

But we cannot implement 'inter-cluster' and 'kashida'.
Because we don't have knowledge for SE Asian language and Arabic.
We don't know how to add extra space to these charachters.

And currently, 'newspaper' is same as 'distribute'.
Because the value's layout need new architecture for justify.
We will re-implement the value after this bug fixed.
Comment 1 Nazo 2004-12-26 20:21:41 PST
Created attachment 169610 [details] [diff] [review]
patch(intl/)
Comment 2 Nazo 2004-12-26 20:22:15 PST
Created attachment 169611 [details] [diff] [review]
patch(dom/)
Comment 3 Nazo 2004-12-26 20:23:08 PST
Created attachment 169612 [details] [diff] [review]
patch(layout/)
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-26 20:31:14 PST
Comment on attachment 169610 [details] [diff] [review]
patch(intl/)

Please re-attach intl/ patch. This is dom/ patch.
Comment 5 Nazo 2004-12-26 20:38:19 PST
Created attachment 169613 [details] [diff] [review]
patch(intl/)

Sorry
Comment 6 Nazo 2004-12-26 20:41:54 PST
Created attachment 169614 [details] [diff] [review]
patch(layout/)
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-26 21:05:14 PST
Comment on attachment 169613 [details] [diff] [review]
patch(intl/)

This intl/ patch has layout/base patch.
Please re-attach intl/ patch and layout/ patch.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-26 21:05:59 PST
Comment on attachment 169614 [details] [diff] [review]
patch(layout/)

This patch does not have layout/base.
Comment 9 Nazo 2004-12-26 21:08:10 PST
Created attachment 169617 [details] [diff] [review]
patch(layout/)

Correction of indents.
Comment 10 Nazo 2004-12-26 21:10:33 PST
Created attachment 169618 [details] [diff] [review]
patch(layout/)

Sorry
Comment 11 Nazo 2004-12-26 21:11:26 PST
Created attachment 169619 [details] [diff] [review]
patch(intl/)
Comment 12 Jungshik Shin 2004-12-26 22:56:10 PST
Comment on attachment 169619 [details] [diff] [review]
patch(intl/)

We should take more time to handle properly grapheme clusters. In this patch,
Hangul Conjoining Jamos should NOT be counted as 'justifiable'. Neither should
characters for various South an Southeast Asian scripts. Latin/Cyrillic/Greek
letters are not so simple either if combining diacritic marks are involved. 
See bug 229896 an bug 260663.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-27 00:32:59 PST
O.K. I cancelled all reviews.
Comment 14 Hixie (not reading bugmail) 2004-12-27 02:00:23 PST
You'll want to check with fantasai to make sure that she's not changing this
property in her CSS3 Text revamp.
Comment 15 Jungshik Shin 2004-12-27 04:02:58 PST
(In reply to comment #13)
> O.K. I cancelled all reviews.

I didn't mean that you had to back out all until you got everythng perfect. You
can still go ahead after getting  fantasai's feedback if you get rid of two most
glaring problems I pointed out in the previous comment. 
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-12-27 06:58:09 PST
Comment on attachment 169611 [details] [diff] [review]
patch(dom/)

This should be added to nsIDOM**NS**CSS2Properties, and you should rev the
UUID.
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-12-27 07:05:34 PST
Comment on attachment 169618 [details] [diff] [review]
patch(layout/)

>+  mTextAlign.AppendToString(buffer, eCSSProperty_text_justify);

You want mTextJustify, not mTextAlign.

>+  else if (eCSSUnit_String == textData.mTextJustify.GetUnit()) {
>+    NS_NOTYETIMPLEMENTED("justify string");
>+  }

I don't see any proposed string value; this looks like it was copied from
'text-align', and should be removed.


Also, to be consistent with other properties, you should probably use the Auto
unit, i.e., using VARIANT_AHK rather than VARIANT_HK, remove Auto from your
keyword list, and check for eCSSUnit_Auto in nsRuleNode::ComputeTextData.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-12-27 07:29:10 PST
Also, if you're not implementing 'inter-cluster' and 'kashida', you shouldn't
parse them.  This allows authors to specify a fallback first.
Comment 19 fantasai 2004-12-27 10:09:46 PST
I'm seriously thinking of removing the 'newspaper' value in favor of controls
through the word-spacing and letter-spacing properties.

Can someone briefly summarize what is being implemented here?
Comment 20 Nazo 2004-12-27 17:59:35 PST
(In reply to comment #16)
>This should be added to nsIDOM**NS**CSS2Properties, and you should rev the
UUID.
OK,but why CSS2 Properties?
This property is in CSS3 properties.
(In reply to comment #17)
>You want mTextJustify, not mTextAlign.
>I don't see any proposed string value; this looks like it was copied from
'text-align', and should be removed.
OK.
I correct.
(In reply to comment #18)
>Also, if you're not implementing 'inter-cluster' and 'kashida', you shouldn't
parse them.  This allows authors to specify a fallback first.
mm..
I implemented.However, only Arabic Kashida and SE Asian Cluster do not correspond.
http://w3c.org/TR/2003/CR-css3-text-20030514/#justification-prop
See the table.
Comment 21 Nazo 2004-12-27 18:13:20 PST
(In reply to comment #16)
Sorry,the utterance before one is a mistake.
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-27 19:40:00 PST
Created attachment 169679 [details]
Sample
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-27 19:57:20 PST
Jungshik:

See attachment 169679 [details].
We don't support grapheme cluster on |PaintTextSlowly|.
Therefore, we should not think grapheme cluster in this bug.
Because I think we should care grapheme cluster on loop of |RenderString|,
|PrepareUnicodeText| and |GetTextDimensionsOrLength|.

i.e., the case of |RenderString|,

2683   for (; --aLength >= 0; aBuffer++) {
2684     nsIFontMetrics* nextFont;
2685     nscoord glyphWidth;
2686     PRUnichar ch = *aBuffer;
    +    PRUnichar nextCh = GetNextChar();
    +    if (IsSameGraphemeCluster(ch, nextCh))
    +      continue;
Comment 24 Jungshik Shin 2004-12-27 20:28:30 PST
(In reply to comment #23)

> We don't support grapheme cluster on |PaintTextSlowly|.

I'm  aware of that. The problem is that you're setting apart characters within a
single grapheme from each other by counting them as justifiable unless I'm
misreading patches. *Even* in 'justified' mode, they should *stay together*.(it
can be compensated down the road in Gfx - I'm doing it in my patch for bug
215219-, but it's kinda waste, isn't it?) That is, in attachment 169619 [details] [diff] [review], Hangul
Conjoining Jamos and letters of South/Southeast Asian scripts should be excluded
from the list of justifiable characters. Even Hangul syllables are not that
simple, but as an approximation, we can live with that for now.
 
   if (aJustifiableCharCount && textBuffer) {
     PRBool isCJ = IsChineseJapaneseLangGroup();
     PRIntn numJustifiableCharacter = 0;
     for (PRInt32 i = 0; i < textLength; i++) {
-      if (IsJustifiableCharacter(textBuffer->mBuffer[i], isCJ))
+      if (IsJustifiableCharacter(textBuffer->mBuffer[i], isCJ,
+          mStyleContext->GetStyleText()->mTextJustify))
         numJustifiableCharacter++;
     }
     *aJustifiableCharCount = numJustifiableCharacter;

+    if (justifying && IsJustifiableCharacter(ch, isCJ, aTextStyle.mTextJustify)) {
       glyphWidth += aTextStyle.mExtraSpacePerJustifiableCharacter;
       if ((PRUint32)--aTextStyle.mNumJustifiableCharacterToRender
             < (PRUint32)aTextStyle.mNumJustifiableCharacterReceivingExtraJot) {
         glyphWidth++;
       }
     }
   }
 }

> loop of |RenderString|,|PrepareUnicodeText| and |GetTextDimensionsOrLength|.

> 2683   for (; --aLength >= 0; aBuffer++) {
> 2684     nsIFontMetrics* nextFont;
> 2685     nscoord glyphWidth;
> 2686     PRUnichar ch = *aBuffer;
>     +    PRUnichar nextCh = GetNextChar();
>     +    if (IsSameGraphemeCluster(ch, nextCh))
>     +      continue;

The above may work in most cases, but in general just considering two adjacent
characters is not enough. However, it can be used for surrogate pair handling.
I'll take care of them (surrogate pairs) in another bug.
Comment 25 fantasai 2004-12-27 21:54:32 PST
Briefly, because I haven't started actually working on justification yet (I'm
just finishing up a first draft for text-wrapping) --

Relevant values for text-justify would be
  auto | inter-word | inter-ideograph | distribute

All values should allow letter-spacing justification as a second priority.
However setting 'letter-spacing' to a value other than 'normal' suppresses that
spacing. This is as specified for CSS2, but different from CSS3 Text CR. I
suspect that the proposed patch handles this already.

If I understand the spec (and the implementation) correctly, 'inter-ideograph'
distributes spacing equally between ideographs and at word spaces and
'distribute' distributes spacing equally among all justifiable characters.
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2004-12-28 07:51:41 PST
> The problem is that you're setting apart characters within a
> single grapheme from each other by counting them as justifiable unless I'm
> misreading patches.

I think |IsJustifiableCharacter| should care every charachter.
But if the charachter is a part of grapheme cluster and not the end charachter
of the cluster, the funcition should not be called.

> However, it can be used for surrogate pair handling.
> I'll take care of them (surrogate pairs) in another bug.

Currently, surrogate pair is supported on |RenderString|, but not supported on
|GetTextDemensionsOrLength|.
Comment 27 Nazo 2004-12-29 04:13:46 PST
Created attachment 169811 [details] [diff] [review]
patch(dom/) v2
Comment 28 Nazo 2004-12-29 04:22:08 PST
Created attachment 169812 [details] [diff] [review]
patch(layout/) v1.1

(In reply to comment #17)
How to use eCSSUnit_Auto?
What should I do next?
Comment 29 Karl Dubost :karlcow 2014-03-11 16:08:23 PDT
> Changes from the November 2012 CSS3 Text WD
> Removed ‘inter-ideograph’, ‘inter-cluster’, and ‘kashida’ values of ‘text-justify’. 

in W3C Working Draft 14 November 2013
http://dev.w3.org/csswg/css-text/

But I see it back in CSS Text Level 4
Editor's Draft 3 February 2014
http://dev.w3.org/csswg/css-text-4/#text-justify0

So I'm a bit confused. 
Fantasai do you know what is the status?


Helping a Web site with Web compat issues (http://tokyoartbeat.com/) I noticed their use of "inter-ideograph". I'm not sure if it's wide spread. It would require a survey on Web sites using kanjis/Chinese characters.

It seems implemented in IE11
http://msdn.microsoft.com/en-us/library/windows/apps/jj680759.aspx
http://connect.microsoft.com/IE/feedback/details/794073/ie11-desktop-crash-with-text-justify-inter-ideograph

Incomplete on Chromium
https://code.google.com/p/chromium/issues/detail?id=71022

Implemented on WebKit
https://bugs.webkit.org/show_bug.cgi?id=53184

Which could be an issue for Web compatibility because of the number of Web sites which are iOS minded on mobile, but as I said, there would need a survey to know if it's really an issue.
Comment 30 philippe (part-time) 2014-03-11 19:27:05 PDT
(In reply to Karl Dubost :karlcow from comment #29)

> It seems implemented in IE11
> http://msdn.microsoft.com/en-us/library/windows/apps/jj680759.aspx
> http://connect.microsoft.com/IE/feedback/details/794073/ie11-desktop-crash-
> with-text-justify-inter-ideograph

Yes that has been implemented in Trident for a long time (IE 6 timespan iirc).

> Implemented on WebKit
> https://bugs.webkit.org/show_bug.cgi?id=53184

Bizarre. It definitely doesn’t work in Safari 7 (10.9.2, iOS 7.1, Webkit nightly).
There is also this bug:
https://bugs.webkit.org/show_bug.cgi?id=99945
Comment 31 Masayuki Nakano [:masayuki] (Mozilla Japan) 2014-03-11 21:40:32 PDT
Our justification is referring the lang information of the element. If it's Japanese or Chinese, CJK characters are justified like inter-ideograph automatically.
Comment 32 fantasai 2014-04-24 12:46:33 PDT
(In reply to Karl Dubost :karlcow from comment #29)
> But I see it back in CSS Text Level 4
> Editor's Draft 3 February 2014
> http://dev.w3.org/csswg/css-text-4/#text-justify0
> 
> So I'm a bit confused. 
> Fantasai do you know what is the status?

Text Level 4 is mostly outdated scratch space at the moment. Level 3 is the mainline. Ignore L4 until it's published as FPWD.

The expectation is that 'auto' will fulfill most justification requirements correctly be default, so specifying inter-ideograph on CJK text will be effectively a no-op.

Note You need to log in before you can comment on or make changes to this bug.