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 on attachment 169610 [details] [diff] [review] patch(intl/) Please re-attach intl/ patch. This is dom/ patch.

Sorry

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 on attachment 169614 [details] [diff] [review] patch(layout/) This patch does not have layout/base.

Correction of indents.

Sorry

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.

O.K. I cancelled all reviews.

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.

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.

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|.

(In reply to comment #17) How to use eCSSUnit_Auto? What should I do next?

> 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.

(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.

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).

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.

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.

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 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 on attachment 8840712 [details] Bug 276079 - fix couple coding style in IsJustifiableCharacter. https://reviewboard.mozilla.org/r/115140/#review117064

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.

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.

Comment on attachment 8840713 [details] Bug 276079 - add layout support for CSS text-justify property. https://reviewboard.mozilla.org/r/115146/#review117312

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 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

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.

(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?

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.

(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.

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

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

MozReview-Commit-ID: DIGQXJkTftz

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.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d50d68150aba3350501d2cc4df8233fa021a08e5 Bug 276079 followup - Make check-for-references.sh accept single-quoted attribute values. No review.

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.