Last Comment Bug 276079 - Implement text-justify property (with values: auto | none | inter-word | inter-character)
: Implement text-justify property (with values: auto | none | inter-word | inte...
Status: ASSIGNED
[evang-wanted]
: css3, dev-doc-needed, intl
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
P3 enhancement with 22 votes (vote)
: ---
Assigned To: Jeremy Chen [:jeremychen] UTC+8
:
:
Mentors:
https://drafts.csswg.org/css-text-3/#...
Depends on: 1342835 grapheme-breaker 1342315
Blocks: css-text-3 css3test
  Show dependency treegraph
 
Reported: 2004-12-26 20:15 PST by Masayuki Nakano [:masayuki]
Modified: 2017-02-28 00:26 PST (History)
34 users (show)
roc: blocking1.9.2-
roc: wanted1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
patch(intl/) (1.33 KB, patch)
2004-12-26 20:21 PST, Nazo
no flags Details | Diff | Splinter Review
patch(dom/) (1.33 KB, patch)
2004-12-26 20:22 PST, Nazo
dbaron: review-
Details | Diff | Splinter Review
patch(layout/) (28.94 KB, patch)
2004-12-26 20:23 PST, Nazo
no flags Details | Diff | Splinter Review
patch(intl/) (6.51 KB, patch)
2004-12-26 20:38 PST, Nazo
no flags Details | Diff | Splinter Review
patch(layout/) (29.13 KB, patch)
2004-12-26 20:41 PST, Nazo
no flags Details | Diff | Splinter Review
patch(layout/) (29.13 KB, patch)
2004-12-26 21:08 PST, Nazo
no flags Details | Diff | Splinter Review
patch(layout/) (30.73 KB, patch)
2004-12-26 21:10 PST, Nazo
no flags Details | Diff | Splinter Review
patch(intl/) (4.90 KB, patch)
2004-12-26 21:11 PST, Nazo
no flags Details | Diff | Splinter Review
Sample (547 bytes, text/html)
2004-12-27 19:40 PST, Masayuki Nakano [:masayuki]
no flags Details
patch(dom/) v2 (1.20 KB, patch)
2004-12-29 04:13 PST, Nazo
no flags Details | Diff | Splinter Review
patch(layout/) v1.1 (31.37 KB, patch)
2004-12-29 04:22 PST, Nazo
no flags Details | Diff | Splinter Review
Bug 276079 - parse and compute CSS text-justify property. (59 bytes, text/x-review-board-request)
2017-02-23 02:34 PST, Jeremy Chen [:jeremychen] UTC+8
xidorn+moz: review+
Details | Review
Basic Functional Tests (745 bytes, text/html)
2017-02-23 18:54 PST, Jeremy Chen [:jeremychen] UTC+8
no flags Details
Bug 276079 - fix couple coding style in IsJustifiableCharacter. (59 bytes, text/x-review-board-request)
2017-02-23 19:24 PST, Jeremy Chen [:jeremychen] UTC+8
xidorn+moz: review+
Details | Review
Bug 276079 - add layout support for CSS text-justify property. (59 bytes, text/x-review-board-request)
2017-02-23 19:24 PST, Jeremy Chen [:jeremychen] UTC+8
xidorn+moz: review+
Details | Review
Bug 276079 - add reftests to CSSWG. (59 bytes, text/x-review-board-request)
2017-02-27 01:15 PST, Jeremy Chen [:jeremychen] UTC+8
xidorn+moz: review+
Details | Review

Description User image Masayuki Nakano [:masayuki] 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 User image Nazo 2004-12-26 20:21:41 PST
Created attachment 169610 [details] [diff] [review]
patch(intl/)
Comment 2 User image Nazo 2004-12-26 20:22:15 PST
Created attachment 169611 [details] [diff] [review]
patch(dom/)
Comment 3 User image Nazo 2004-12-26 20:23:08 PST
Created attachment 169612 [details] [diff] [review]
patch(layout/)
Comment 4 User image Masayuki Nakano [:masayuki] 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 User image Nazo 2004-12-26 20:38:19 PST
Created attachment 169613 [details] [diff] [review]
patch(intl/)

Sorry
Comment 6 User image Nazo 2004-12-26 20:41:54 PST
Created attachment 169614 [details] [diff] [review]
patch(layout/)
Comment 7 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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 User image Nazo 2004-12-26 21:08:10 PST
Created attachment 169617 [details] [diff] [review]
patch(layout/)

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

Sorry
Comment 11 User image Nazo 2004-12-26 21:11:26 PST
Created attachment 169619 [details] [diff] [review]
patch(intl/)
Comment 12 User image 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 User image Masayuki Nakano [:masayuki] 2004-12-27 00:32:59 PST
O.K. I cancelled all reviews.
Comment 14 User image 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 User image 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 User image David Baron :dbaron: ⌚️UTC-8 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 User image David Baron :dbaron: ⌚️UTC-8 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 User image David Baron :dbaron: ⌚️UTC-8 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 User image 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 User image 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 User image Nazo 2004-12-27 18:13:20 PST
(In reply to comment #16)
Sorry,the utterance before one is a mistake.
Comment 22 User image Masayuki Nakano [:masayuki] 2004-12-27 19:40:00 PST
Created attachment 169679 [details]
Sample
Comment 23 User image Masayuki Nakano [:masayuki] 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 User image 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 User image 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 User image Masayuki Nakano [:masayuki] 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 User image Nazo 2004-12-29 04:13:46 PST
Created attachment 169811 [details] [diff] [review]
patch(dom/) v2
Comment 28 User image 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 User image 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 User image 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 User image Masayuki Nakano [:masayuki] 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 User image 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.
Comment 33 User image Jonathan Kew (:jfkthame) 2016-12-13 14:41:11 PST
Currently, the values specified by https://drafts.csswg.org/css-text-3/#text-justify-property are

  auto | none | inter-word | inter-character

plus 'distribute' as a legacy alias for 'inter-character'.

We should probably go ahead and implement those. It seems like they should be relatively straightforward tweaks to our existing behavior (which will be the 'auto' value).
Comment 34 User image Xidorn Quan [:xidorn] (UTC+10) 2016-12-13 15:03:22 PST
Yeah, it should be staightforward given our current implementation. I think the majority of work would be in PropertyProvider::ComputeJustification.
Comment 35 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-07 23:57:59 PST
Start to work on this from today.

I'll study a bit about the spec. status and vendor support status.
Then, I shall send an intent-to-implement mail to dev-platform.
Comment 36 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-23 02:34:51 PST Comment hidden (mozreview-request)
Comment 37 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-23 18:54:43 PST
Created attachment 8840704 [details]
Basic Functional Tests
Comment 38 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-23 19:24:22 PST Comment hidden (mozreview-request)
Comment 39 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-23 19:24:22 PST Comment hidden (mozreview-request)
Comment 40 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-23 19:24:22 PST Comment hidden (mozreview-request)
Comment 41 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-24 01:37:52 PST
After applying current WIPs, the result of Basic Functional Test (as attached) looks almost fine, except two issues:

1. "fi" ligature is not justified properly

This seems like an existing issue for letter-spacing, I've filed Bug 1342315 for further investigation.

2. the justified space after "fi" is too big

An interesting thing is, if I try to select texts across the position of "fi", the space seems to be right at a moment. Once I pass the position to the rest of other texts, the space become too big again. Which means, if I drag a selection range back and forth at "fi" position, there's a jiggle effect for the justified space after "fi". I'll keep investigating this issue.
Comment 42 User image Jonathan Kew (:jfkthame) 2017-02-24 02:30:44 PST
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #41)
> After applying current WIPs, the result of Basic Functional Test (as
> attached) looks almost fine, except two issues:
> 
> 1. "fi" ligature is not justified properly
> 
> This seems like an existing issue for letter-spacing, I've filed Bug 1342315
> for further investigation.

That's a recent (Mac-only?) regression, as noted in that bug. I'm looking into it.

> 
> 2. the justified space after "fi" is too big
> 
> An interesting thing is, if I try to select texts across the position of
> "fi", the space seems to be right at a moment. Once I pass the position to
> the rest of other texts, the space become too big again. Which means, if I
> drag a selection range back and forth at "fi" position, there's a jiggle
> effect for the justified space after "fi". I'll keep investigating this
> issue.

It sounds like this may be a side-effect of (1), and I suspect the issue may disappear when that is fixed.
Comment 43 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-24 19:01:44 PST
(In reply to Jonathan Kew (:jfkthame) from comment #42)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #41)
> > After applying current WIPs, the result of Basic Functional Test (as
> > attached) looks almost fine, except two issues:
> > 
> > 1. "fi" ligature is not justified properly
> > 
> > This seems like an existing issue for letter-spacing, I've filed Bug 1342315
> > for further investigation.
> 
> That's a recent (Mac-only?) regression, as noted in that bug. I'm looking
> into it.
> 
> > 
> > 2. the justified space after "fi" is too big
> > 
> > An interesting thing is, if I try to select texts across the position of
> > "fi", the space seems to be right at a moment. Once I pass the position to
> > the rest of other texts, the space become too big again. Which means, if I
> > drag a selection range back and forth at "fi" position, there's a jiggle
> > effect for the justified space after "fi". I'll keep investigating this
> > issue.
> 
> It sounds like this may be a side-effect of (1), and I suspect the issue may
> disappear when that is fixed.

Nice, thank you for helping me out.
After I applied the patch from Bug 1342315 (which is now in inbound already), both issues are gone.
With my latest patchset (which I'll update very soon), the result of Basic Functional Test looks fine now.

I guess the next step would be the test coverage.
Not sure if I should take any more complicated tests (other than the Basic Functional Test I've uploaded) in this bug...
Comment 44 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-24 19:02:42 PST Comment hidden (mozreview-request)
Comment 45 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-24 19:02:42 PST Comment hidden (mozreview-request)
Comment 46 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-24 19:02:42 PST Comment hidden (mozreview-request)
Comment 47 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-24 19:22:35 PST
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #43)
> (In reply to Jonathan Kew (:jfkthame) from comment #42)
> > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #41)
> > > After applying current WIPs, the result of Basic Functional Test (as
> > > attached) looks almost fine, except two issues:
> > > 
> > > 1. "fi" ligature is not justified properly
> > > 
> > > This seems like an existing issue for letter-spacing, I've filed Bug 1342315
> > > for further investigation.
> > 
> > That's a recent (Mac-only?) regression, as noted in that bug. I'm looking
> > into it.
> > 
> > > 
> > > 2. the justified space after "fi" is too big
> > > 
> > > An interesting thing is, if I try to select texts across the position of
> > > "fi", the space seems to be right at a moment. Once I pass the position to
> > > the rest of other texts, the space become too big again. Which means, if I
> > > drag a selection range back and forth at "fi" position, there's a jiggle
> > > effect for the justified space after "fi". I'll keep investigating this
> > > issue.
> > 
> > It sounds like this may be a side-effect of (1), and I suspect the issue may
> > disappear when that is fixed.
> 
> Nice, thank you for helping me out.
> After I applied the patch from Bug 1342315 (which is now in inbound
> already), both issues are gone.
> With my latest patchset (which I'll update very soon), the result of Basic
> Functional Test looks fine now.
> 
> I guess the next step would be the test coverage.
> Not sure if I should take any more complicated tests (other than the Basic
> Functional Test I've uploaded) in this bug...

One thing I can think of is to include some CJK texts in the test.
Comment 48 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 01:15:41 PST Comment hidden (mozreview-request)
Comment 49 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 01:15:41 PST Comment hidden (mozreview-request)
Comment 50 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 01:15:41 PST Comment hidden (mozreview-request)
Comment 51 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 01:15:41 PST Comment hidden (mozreview-request)
Comment 52 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 01:23:27 PST
In the current version, rendering of cursive scripts such as Arabic is still buggy. Filed Bug 1342835 for this, since this is related to our current letter-spacing implementation as well.

As to the tests, since the rendering result of text-justify: auto is UA dependent, I'm not sure which test I should write for the "auto" keyword case. Please provide comment/feedback, and I'd be happy to increase the test coverage (no matter in this bug, or file another one to handle more advanced tests).
Comment 53 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-27 01:46:14 PST
Comment on attachment 8840375 [details]
Bug 276079 - parse and compute CSS text-justify property.

https://reviewboard.mozilla.org/r/114900/#review117062

r=me with the issue below addressed.

::: layout/style/nsStyleConsts.h:169
(Diff revision 4)
> +  Auto,
> +  InterWord,
> +  InterCharacter,
> +  // For legacy reasons, UAs must also support the keyword "distribute" with
> +  // the exact same meaning and behavior as "inter-character".
> +  Distribute

Given how the spec talk about this value, I think you can simply make the keyword an alias of inter-character in the KTable, so that you don't need to have anything specific for it after parsing.
Comment 54 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-27 01:47:00 PST
Comment on attachment 8840712 [details]
Bug 276079 - fix couple coding style in IsJustifiableCharacter.

https://reviewboard.mozilla.org/r/115140/#review117064
Comment 55 User image percyley 2017-02-27 01:47:31 PST
The spec said:

For legacy reasons, UAs must also support the alternate keyword distribute with the exact same meaning and behavior.

https://drafts.csswg.org/css-text-3/#valdef-text-justify-distribute
Comment 56 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-27 01:51:38 PST
Yes, so we should simply parse distribute as inter-character, rather than keeping that distribute as a separate value around.
Comment 57 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-27 03:38:15 PST
Comment on attachment 8840713 [details]
Bug 276079 - add layout support for CSS text-justify property.

https://reviewboard.mozilla.org/r/115146/#review117074

::: layout/base/nsLayoutUtils.cpp:6978
(Diff revision 3)
> +      aStyleText->mTextJustify == StyleTextJustify::InterCharacter ||
> +      aStyleText->mTextJustify == StyleTextJustify::Distribute) {

Make `distribute` a parse-time alias in part 1 so that you don't need to check two here.

::: layout/generic/nsTextFrame.cpp:2973
(Diff revision 3)
> +                                   const nsTextFragment* aFrag, int32_t aPos,
>                                     bool aLangIsCJ)
>  {
>    NS_ASSERTION(aPos >= 0, "negative position?!");
> +
> +  if (aTextStyle->mTextJustify == StyleTextJustify::None) {

It would probably be better to have a local variable record the value of text justify, so that code below can be simplified... but I can live either way.

::: layout/generic/nsTextFrame.cpp:2979
(Diff revision 3)
> -    return true;
> +    return aTextStyle->mTextJustify == StyleTextJustify::Auto ||
> +           aTextStyle->mTextJustify == StyleTextJustify::InterWord;

What about `inter-character`? Why spaces are not justifiable with `inter-character`? It seems to me expandable characters with `inter-character` should be a superset of those with `inter-word`.

::: layout/generic/nsTextFrame.cpp:2992
(Diff revision 3)
> +      // text-justify: inter-character or distribute
> +      return false;

Same here, I don't think `inter-character` should have different behavior than `inter-word` for spaces.
Comment 58 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 08:02:05 PST
Comment on attachment 8840375 [details]
Bug 276079 - parse and compute CSS text-justify property.

https://reviewboard.mozilla.org/r/114900/#review117062

> Given how the spec talk about this value, I think you can simply make the keyword an alias of inter-character in the KTable, so that you don't need to have anything specific for it after parsing.

I did thought about this, but I chose to use `distribute` as a separate keyword because I was not sure if we can use `inter-character` as the computed value of `distribute`. I guess maybe it's fine for both of `inter-character` and `distribute` to share the same computed value.
Comment 59 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 08:18:32 PST
Comment on attachment 8840713 [details]
Bug 276079 - add layout support for CSS text-justify property.

https://reviewboard.mozilla.org/r/115146/#review117074

> Make `distribute` a parse-time alias in part 1 so that you don't need to check two here.

Okay.

> It would probably be better to have a local variable record the value of text justify, so that code below can be simplified... but I can live either way.

Agree. Will use a local variable then.

> What about `inter-character`? Why spaces are not justifiable with `inter-character`? It seems to me expandable characters with `inter-character` should be a superset of those with `inter-word`.

Yeah, I re-read the specification again, and think you're right. I'll fix this in the next version.
Comment 60 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 09:03:01 PST Comment hidden (mozreview-request)
Comment 61 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 09:03:01 PST Comment hidden (mozreview-request)
Comment 62 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 09:03:01 PST Comment hidden (mozreview-request)
Comment 63 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 09:03:01 PST Comment hidden (mozreview-request)
Comment 64 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-27 15:13:43 PST
Comment on attachment 8840375 [details]
Bug 276079 - parse and compute CSS text-justify property.

https://reviewboard.mozilla.org/r/114900/#review117278

::: layout/style/nsStyleConsts.h:167
(Diff revision 5)
> +  // For legacy reasons, UAs must also support the keyword "distribute" with
> +  // the exact same meaning and behavior as "inter-character".
> +  Distribute = InterCharacter

You don't need this at all. You can simply make `nsCSSProps::kTextJustifyKTable` to do `{ eCSSKeyword_distribute, StyleTextJustify::InterCharacter },` and put the comment there.
Comment 65 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-27 15:15:14 PST
Comment on attachment 8840713 [details]
Bug 276079 - add layout support for CSS text-justify property.

https://reviewboard.mozilla.org/r/115146/#review117280

::: layout/base/nsLayoutUtils.cpp:6978
(Diff revision 4)
> +      aStyleText->mTextJustify == StyleTextJustify::InterCharacter ||
> +      aStyleText->mTextJustify == StyleTextJustify::Distribute) {

It's still redundant.

::: layout/generic/nsTextFrame.cpp:2981
(Diff revision 4)
> +  }
> +
>    char16_t ch = aFrag->CharAt(aPos);
>    if (ch == '\n' || ch == '\t' || ch == '\r') {
> -    return true;
> +    return justifyStyle == StyleTextJustify::Auto ||
> +           justifyStyle == StyleTextJustify::InterWord;

There is still no `InterCharacter`... Actually you should just not touch this line, as `return true;` here seems to be the right thing to do.
Comment 66 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 20:55:25 PST Comment hidden (mozreview-request)
Comment 67 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 20:55:25 PST Comment hidden (mozreview-request)
Comment 68 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 20:55:25 PST Comment hidden (mozreview-request)
Comment 69 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-27 20:55:25 PST Comment hidden (mozreview-request)
Comment 70 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-27 20:57:37 PST
Comment on attachment 8840713 [details]
Bug 276079 - add layout support for CSS text-justify property.

https://reviewboard.mozilla.org/r/115146/#review117312
Comment 71 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-27 21:04:40 PST
Comment on attachment 8841466 [details]
Bug 276079 - add reftests to CSSWG.

https://reviewboard.mozilla.org/r/115688/#review117314

::: layout/reftests/w3c-css/submitted/text3/text-justify-distribute-001.html:2
(Diff revision 3)
> +<!DOCTYPE html>
> +<html  lang="en" >

`<html lang="en">`.

::: layout/reftests/w3c-css/submitted/text3/text-justify-inter-word-001.html:24
(Diff revision 3)
> +<p class="test">日本 文字</p>
> +<p class="test">อักษรไทย อักษรไทย</p>

Please give them proper lang tag. Behavior of justification may depend on the content language, so we want to cover that case.

::: layout/reftests/w3c-css/submitted/text3/text-justify-none-001.html:23
(Diff revision 3)
> +  text-justify: none;
> +}
> +</style>
> +</head>
> +<body>
> +<p class="test">日本文字 Latin text อักษรไทย</p>

Please test this with different lang tag specified. You can simply add multiple `<p>`s in this file with different lang.
Comment 72 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-28 00:19:20 PST
Comment on attachment 8841466 [details]
Bug 276079 - add reftests to CSSWG.

https://reviewboard.mozilla.org/r/115688/#review117314

> `<html lang="en">`.

I'll remove all these global language attribute and use specific ones for each `<p>` element instead.

> Please give them proper lang tag. Behavior of justification may depend on the content language, so we want to cover that case.

Okay.

> Please test this with different lang tag specified. You can simply add multiple `<p>`s in this file with different lang.

Okay.
Comment 73 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-28 00:22:33 PST Comment hidden (mozreview-request)
Comment 74 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-28 00:22:33 PST Comment hidden (mozreview-request)
Comment 75 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-28 00:22:33 PST Comment hidden (mozreview-request)
Comment 76 User image Jeremy Chen [:jeremychen] UTC+8 2017-02-28 00:22:33 PST Comment hidden (mozreview-request)
Comment 77 User image Xidorn Quan [:xidorn] (UTC+10) 2017-02-28 00:26:01 PST
Comment on attachment 8841466 [details]
Bug 276079 - add reftests to CSSWG.

https://reviewboard.mozilla.org/r/115688/#review117334

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