Closed Bug 276079 Opened 20 years ago Closed 7 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: masayuki, Assigned: chenpighead)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

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

Attachments

(5 files, 12 obsolete files)

59 bytes, text/x-review-board-request
xidorn
: review+
Details
745 bytes, text/html
Details
59 bytes, text/x-review-board-request
xidorn
: review+
Details
59 bytes, text/x-review-board-request
xidorn
: review+
Details
59 bytes, text/x-review-board-request
xidorn
: review+
Details
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.
Attached patch patch(intl/) (obsolete) — Splinter Review
Attached patch patch(dom/) (obsolete) — Splinter Review
Attached patch patch(layout/) (obsolete) — Splinter Review
Comment on attachment 169610 [details] [diff] [review]
patch(intl/)

Please re-attach intl/ patch. This is dom/ patch.
Attachment #169610 - Attachment is obsolete: true
Attachment #169611 - Flags: review?(peterv)
Status: NEW → ASSIGNED
Attached patch patch(intl/) (obsolete) — Splinter Review
Sorry
Attachment #169613 - Flags: review?(jshin)
Attached patch patch(layout/) (obsolete) — Splinter Review
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
Attached patch patch(layout/) (obsolete) — Splinter Review
Correction of indents.
Attached patch patch(layout/) (obsolete) — Splinter Review
Sorry
Attachment #169617 - Attachment is obsolete: true
Attached patch patch(intl/) (obsolete) — Splinter Review
Attachment #169618 - Flags: superreview?(roc)
Attachment #169618 - Flags: review?(roc)
Attachment #169619 - Flags: review?(jshin)
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)
Attachment #169618 - Flags: superreview?(roc)
Attachment #169618 - Flags: review?(roc)
Attachment #169611 - Flags: review?(peterv)
O.K. I cancelled all reviews.
Depends on: grapheme-breaker
You'll want to check with fantasai to make sure that she's not changing this
property in her CSS3 Text revamp.
(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.
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?
(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.
(In reply to comment #16)
Sorry,the utterance before one is a mistake.
Attached file Sample (obsolete) —
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;
(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.
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|.
Attached patch patch(dom/) v2 (obsolete) — Splinter Review
Attachment #169611 - Attachment is obsolete: true
Attached patch patch(layout/) v1.1 (obsolete) — Splinter Review
(In reply to comment #17)
How to use eCSSUnit_Auto?
What should I do next?
Attachment #169618 - Attachment is obsolete: true
Summary: Implement text-justify property(but exclude 'inter-cluster' and 'kashida') → Implement text-justify property(but 'auto', 'inter-word', 'inter-ideograph' and 'distribute' only)
Blocks: css-text-3
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]
Blocks: css3test
> 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.
(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)
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.
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
Attachment #169619 - Attachment is obsolete: true
Attachment #169679 - Attachment is obsolete: true
Attachment #169811 - Attachment is obsolete: true
Attachment #169812 - Attachment is obsolete: true
Depends on: 1342315
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.
(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...
(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.
Depends on: 1342835
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).
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 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 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+
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 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)
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 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 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 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 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 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)
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 on attachment 8841466 [details]
Bug 276079 - add reftests to CSSWG.

https://reviewboard.mozilla.org/r/115688/#review117334
Attachment #8841466 - Flags: review?(xidorn+moz) → review+
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.
Whiteboard: [evang-wanted] → [evang-wanted][behind pref layout.css.text-justify.enabled]
(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
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)
Blocks: 1343512
(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.
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
Attached patch update stylo-failures.md. (obsolete) — Splinter Review
MozReview-Commit-ID: DIGQXJkTftz
Attachment #8842500 - Flags: review?(manishearth)
Attachment #8842500 - Flags: review?(manishearth) → review+
Depends on: 1343593
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
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!
(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!!! :-)
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).
(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.