Implement text-justify property (with values: auto | none | inter-word | inter-character)

RESOLVED FIXED in Firefox 54

Status

()

Core
Layout: Text
P3
enhancement
RESOLVED FIXED
13 years ago
4 months ago

People

(Reporter: masayuki, Assigned: jeremychen)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, 4 keywords)

Trunk
mozilla54
css3, dev-doc-complete, feature, intl
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 -
qe-verify -

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [evang-wanted][behind pref layout.css.text-justify.enabled], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 12 obsolete attachments)

59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
745 bytes, text/html
Details
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
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

13 years ago
Created attachment 169610 [details] [diff] [review]
patch(intl/)

Comment 2

13 years ago
Created attachment 169611 [details] [diff] [review]
patch(dom/)

Comment 3

13 years ago
Created attachment 169612 [details] [diff] [review]
patch(layout/)
Comment on attachment 169610 [details] [diff] [review]
patch(intl/)

Please re-attach intl/ patch. This is dom/ patch.
Attachment #169610 - Attachment is obsolete: true
(Reporter)

Updated

13 years ago
Attachment #169611 - Flags: review?(peterv)
(Reporter)

Updated

13 years ago
Status: NEW → ASSIGNED

Comment 5

13 years ago
Created attachment 169613 [details] [diff] [review]
patch(intl/)

Sorry
(Reporter)

Updated

13 years ago
Attachment #169613 - Flags: review?(jshin)

Comment 6

13 years ago
Created attachment 169614 [details] [diff] [review]
patch(layout/)
Attachment #169612 - Attachment is obsolete: true
Comment on attachment 169613 [details] [diff] [review]
patch(intl/)

This intl/ patch has layout/base patch.
Please re-attach intl/ patch and layout/ patch.
Attachment #169613 - Attachment is obsolete: true
Attachment #169613 - Flags: review?(jshin)
Comment on attachment 169614 [details] [diff] [review]
patch(layout/)

This patch does not have layout/base.
Attachment #169614 - Attachment is obsolete: true

Comment 9

13 years ago
Created attachment 169617 [details] [diff] [review]
patch(layout/)

Correction of indents.

Comment 10

13 years ago
Created attachment 169618 [details] [diff] [review]
patch(layout/)

Sorry
Attachment #169617 - Attachment is obsolete: true

Comment 11

13 years ago
Created attachment 169619 [details] [diff] [review]
patch(intl/)
(Reporter)

Updated

13 years ago
Attachment #169618 - Flags: superreview?(roc)
Attachment #169618 - Flags: review?(roc)
(Reporter)

Updated

13 years ago
Attachment #169619 - Flags: review?(jshin)

Comment 12

13 years ago
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.
Attachment #169619 - Flags: review?(jshin)
(Reporter)

Updated

13 years ago
Attachment #169618 - Flags: superreview?(roc)
Attachment #169618 - Flags: review?(roc)
(Reporter)

Updated

13 years ago
Attachment #169611 - Flags: review?(peterv)
O.K. I cancelled all reviews.
Depends on: 229896
You'll want to check with fantasai to make sure that she's not changing this
property in her CSS3 Text revamp.

Comment 15

13 years ago
(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 on attachment 169611 [details] [diff] [review]
patch(dom/)

This should be added to nsIDOM**NS**CSS2Properties, and you should rev the
UUID.
Attachment #169611 - Flags: review-
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.
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

13 years ago
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

13 years ago
(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

13 years ago
(In reply to comment #16)
Sorry,the utterance before one is a mistake.
Created attachment 169679 [details]
Sample
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

13 years ago
(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

13 years ago
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.
> 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

13 years ago
Created attachment 169811 [details] [diff] [review]
patch(dom/) v2
Attachment #169611 - Attachment is obsolete: true

Comment 28

13 years ago
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?
Attachment #169618 - Attachment is obsolete: true
(Reporter)

Updated

13 years ago
Summary: Implement text-justify property(but exclude 'inter-cluster' and 'kashida') → Implement text-justify property(but 'auto', 'inter-word', 'inter-ideograph' and 'distribute' only)

Updated

13 years ago
Blocks: 104960

Updated

8 years ago
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Whiteboard: [evang-wanted]

Updated

4 years ago
Blocks: 913153
> 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.
Flags: needinfo?(fantasai.bugs)
(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
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

3 years ago
(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.
Flags: needinfo?(fantasai.bugs)
Keywords: dev-doc-needed

Updated

9 months ago
Priority: -- → P3
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).
Summary: Implement text-justify property(but 'auto', 'inter-word', 'inter-ideograph' and 'distribute' only) → Implement text-justify property (with values: auto | none | inter-word | inter-character)
Yeah, it should be staightforward given our current implementation. I think the majority of work would be in PropertyProvider::ComputeJustification.
(Assignee)

Comment 35

6 months ago
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.
Assignee: lovesyao → jeremychen
Comment hidden (mozreview-request)
(Assignee)

Comment 37

6 months ago
Created attachment 8840704 [details]
Basic Functional Tests
(Assignee)

Updated

6 months ago
Attachment #169619 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #169679 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #169811 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #169812 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

6 months ago

Updated

6 months ago
(Assignee)

Updated

6 months ago
Depends on: 1342315
(Assignee)

Comment 41

6 months ago
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.
(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.
(Assignee)

Comment 43

6 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
(Assignee)

Comment 47

6 months ago
(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.
(Assignee)

Updated

6 months ago
Depends on: 1342835
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 52

6 months ago
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).
(Assignee)

Updated

6 months ago
Attachment #8840375 - Flags: review?(xidorn+moz)
Attachment #8840712 - Flags: review?(xidorn+moz)
Attachment #8840713 - Flags: review?(xidorn+moz)
Attachment #8841466 - Flags: review?(xidorn+moz)

Comment 53

6 months ago
mozreview-review
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.
Attachment #8840375 - Flags: review?(xidorn+moz) → review+

Comment 54

6 months ago
mozreview-review
Comment on attachment 8840712 [details]
Bug 276079 - fix couple coding style in IsJustifiableCharacter.

https://reviewboard.mozilla.org/r/115140/#review117064
Attachment #8840712 - Flags: review?(xidorn+moz) → review+

Comment 55

6 months ago
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
Yes, so we should simply parse distribute as inter-character, rather than keeping that distribute as a separate value around.

Comment 57

6 months ago
mozreview-review
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.
Attachment #8840713 - Flags: review?(xidorn+moz)
(Assignee)

Comment 58

6 months ago
mozreview-review-reply
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.
(Assignee)

Comment 59

6 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 64

6 months ago
mozreview-review
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

6 months ago
mozreview-review
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.
Attachment #8840713 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 70

6 months ago
mozreview-review
Comment on attachment 8840713 [details]
Bug 276079 - add layout support for CSS text-justify property.

https://reviewboard.mozilla.org/r/115146/#review117312
Attachment #8840713 - Flags: review?(xidorn+moz) → review+

Comment 71

6 months ago
mozreview-review
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.
Attachment #8841466 - Flags: review?(xidorn+moz)
(Assignee)

Comment 72

6 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 77

6 months ago
mozreview-review
Comment on attachment 8841466 [details]
Bug 276079 - add reftests to CSSWG.

https://reviewboard.mozilla.org/r/115688/#review117334
Attachment #8841466 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 78

6 months ago
Got a lot of reftest failures like https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff90d6884a7c
I can't reproduce any of them on both my Mac machine and Linux VM, so I'll try a real Linux machine tomorrow.

Updated

6 months ago
Whiteboard: [evang-wanted] → [evang-wanted][behind pref layout.css.text-justify.enabled]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 83

6 months ago
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #78)
> Got a lot of reftest failures like
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff90d6884a7c
> I can't reproduce any of them on both my Mac machine and Linux VM, so I'll
> try a real Linux machine tomorrow.

Hmmm... looks like I forgot to add copy constructor for mTextJustify member in parser patch...:(

Guess that DEBUG build would initialize mTextJustify for us anyway, but OPT build would not.
So, the uninitialized mTextJustify caused couple intermittents on try.

Try looks green on reftests now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68da60da1ffe
(Assignee)

Comment 84

6 months ago
While waiting for the try result, I wonder whether we could turn on the pref on Nightly in this bug?
If we do, we could either
1. pref-on on Nightly only, pref-off on others; or
2. pref-on on Nightly/Aurora, pref-off on Release/Beta.

Xidorn, what do you think?
Or, we should just land this bug w/ pref-off, and file another bug to discuss this?
Flags: needinfo?(xidorn+moz)
We should land it with pref off, and file another bug to discuss enabling.

You need to send an "intend to ship" to dev-platform before landing the pref on patch.

I don't think this feature needs a nightly/aurora-only strategy since its small enough and all known work has been done. So if we decide to pref on, it can just ride the train.
Flags: needinfo?(xidorn+moz)
(Assignee)

Updated

6 months ago
Blocks: 1343512
(Assignee)

Comment 86

6 months ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #85)
> We should land it with pref off, and file another bug to discuss enabling.
> 
> You need to send an "intend to ship" to dev-platform before landing the pref
> on patch.
> 
> I don't think this feature needs a nightly/aurora-only strategy since its
> small enough and all known work has been done. So if we decide to pref on,
> it can just ride the train.

Okay. Filed Bug 1343512.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 91

6 months ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5016caa4342e35722f5083503714ba5819500594

Comment 92

6 months ago
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb76940bbff6
parse and compute CSS text-justify property. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/17c186ca2567
fix couple coding style in IsJustifiableCharacter. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/534299ae8575
add layout support for CSS text-justify property. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/1e68627db428
add reftests to CSSWG. r=xidorn
(Assignee)

Comment 93

6 months ago
Created attachment 8842500 [details] [diff] [review]
update stylo-failures.md.

MozReview-Commit-ID: DIGQXJkTftz
(Assignee)

Updated

6 months ago
Attachment #8842500 - Flags: review?(manishearth)
Attachment #8842500 - Flags: review?(manishearth) → review+
(Assignee)

Updated

6 months ago
Depends on: 1343593
(Assignee)

Comment 94

6 months ago
Comment on attachment 8842500 [details] [diff] [review]
update stylo-failures.md.

This patch has been landed in Bug 1343593, so let's obsolete this here.
Attachment #8842500 - Attachment is obsolete: true

Comment 95

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb76940bbff6
https://hg.mozilla.org/mozilla-central/rev/17c186ca2567
https://hg.mozilla.org/mozilla-central/rev/534299ae8575
https://hg.mozilla.org/mozilla-central/rev/1e68627db428
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
https://hg.mozilla.org/integration/mozilla-inbound/rev/d50d68150aba3350501d2cc4df8233fa021a08e5
Bug 276079 followup - Make check-for-references.sh accept single-quoted attribute values.  No review.

Comment 97

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d50d68150aba
(Assignee)

Updated

5 months ago
Keywords: feature
I've documented this property on MDN.

I've added a property reference page:
https://developer.mozilla.org/en-US/docs/Web/CSS/text-justify

And a note to the Fx54 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/54#CSS

On the reference page, the formal syntax and property features have not yet appeared (see the "not found in DB!" messages); I've submitted a pull request to add them to the database:
https://github.com/mdn/data/pull/60

This should be reviewed and added soon.

Could you give them a technical review? Thanks!
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 99

4 months ago
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #98)
> I've documented this property on MDN.
> 
> I've added a property reference page:
> https://developer.mozilla.org/en-US/docs/Web/CSS/text-justify
> 
> And a note to the Fx54 release notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/54#CSS
> 
> On the reference page, the formal syntax and property features have not yet
> appeared (see the "not found in DB!" messages); I've submitted a pull
> request to add them to the database:
> https://github.com/mdn/data/pull/60
> 
> This should be reviewed and added soon.
> 
> Could you give them a technical review? Thanks!

All MDN pages and PR look great to me. Couple nits for the text-justify MDN page:

1. An extra bracket in the 2nd line of the summary should be removed,
2. If text-justify is not set at all, the default justification used is `auto`, not `inter-word`,
3. We've supported text-justify in FF54 with pref-off, and decided to pref-on in FF55 (see Bug 1343512),

I can correct the first 2 by myself. Not sure if there's some convention wording for the 3rd nit, so I'll leave that for you.

Thank you for documenting this!!! :-)
(Assignee)

Comment 100

4 months ago
One more thing that might worth adding to the MDN is the status of other browser vendors. You can refer to http://caniuse.com/#search=text-justify. I believe IE, Edge, and Chrome should be annotated as `partial support` at least.
Thanks for working on this, Jeremy.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #100)
> One more thing that might worth adding to the MDN is the status of other
> browser vendors. You can refer to http://caniuse.com/#search=text-justify. I
> believe IE, Edge, and Chrome should be annotated as `partial support` at
> least.

Cheers for the tech review, Jeremy. I've updated the support section according to the caniuse notes:

https://developer.mozilla.org/en-US/docs/Web/CSS/text-justify

I've also moved the release note to the Fx 55 page:

https://developer.mozilla.org/en-US/Firefox/Releases/55#CSS

(We don't announce feature support when the feature is preffed off, only when it is turned on by default).
(Assignee)

Comment 103

4 months ago
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #102)
> 
> Cheers for the tech review, Jeremy. I've updated the support section
> according to the caniuse notes:
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/text-justify
> 
> I've also moved the release note to the Fx 55 page:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/55#CSS
> 
> (We don't announce feature support when the feature is preffed off, only
> when it is turned on by default).

Nice work. Thank you!
Setting qe-verify- since this has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.