Closed Bug 1097499 Opened 5 years ago Closed 4 years ago

Add layout support for "all" value of text-combine-upright (tate-chu-yoko)

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed
relnote-firefox --- 48+

People

(Reporter: smontagu, Assigned: xidorn)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-edge][parity-blink][parity-webkit])

Attachments

(16 files)

58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
emk
: review+
Details
58 bytes, text/x-review-board-request
emk
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
We have the CSS infrastructure for the text-combine-horizontal property already, but we need to add support for it in layout and text-runs.
Depends on: 1134849
Presumably we need to do something like what we do with bidi in terms of splitting some runs of text into separate frames, although the splitting may not need to go up the tree through inline elements, which seems likely to make it a bit easier.   Then once we have that we can just apply the relevant font properties, and then some alignment in layout code.
Version: unspecified → Trunk
Currently, IE and Edge have the whole support of this property under property -ms-text-combine-horizontal, WebKit and Blink support only "all" (which is called "horizontal" for prefixed -webkit-text-combine, but Blink recently unprefixed this property and start accepting "all").

I got query from Koji (in Chrome team) and Hiroshi (from BPS) about our plan of implementing this. According to them, Japanese publishers really want to have this.

jfkthame, I heard from jet that this is currently in your list. Do you have plan working on this? If you are busy now and do not have plan implementing this, I can probably take it.

It seems to me the hardest part would probably be the interaction with text decorations.
Flags: needinfo?(jfkthame)
Whiteboard: [parity-edge][parity-blink][parity-webkit]
It's on my list of "things we ought to do", but I don't have any current plans to work on this, so if you have time to work on it that would be awesome.

ISTM that 'all' should be fairly straightforward to implement (yes, decorations may need some work). The trickier part is the 'digits' values, because this will require scanning the content during frame construction time to detect and split out runs that need to be wrapped in a separate anonymous inline-block, or something like that. A bit like the code we have for first-letter, except that it can happen anywhere in the text, not just at the beginning.
Flags: needinfo?(jfkthame)
Release Note Request (optional, but appreciated)
[Why is this notable]: According to some people from Japanese ebook companies, text-combine-upright (a.k.a horizontal-in-vertical, or tate-chu-yoko) is a must for Japanese vertical layout. Without the proper support of this feature, a book may not be considered complete, and thus cannot be sold online. This is also a common pattern for layout of Traditional Chinese.

There are two values for text-combine-upright, one is "all", the other is "digits <number>". We are currently going to implement only "all", because that's much easier to implement and can solve the majority of the problem people have.

[Suggested wording]: Added vertical text layout support with the "text-combine-upright: all" CSS property
[Links (documentation, blog post, etc)]:

Link to standard:
https://drafts.csswg.org/css-writing-modes-3/#text-combine-upright

Platform coverage: all

Estimated or target release: Firefox 48 (or 49 if missed the cycle of 48)

Preference behind which this will be implemented:
layout.css.text-combine-upright.enabled

DevTools bug: This doesn't seem to need any additional support from
DevTools.

Do other browser engines implement this?
All other browser engines have had the support of the functionality of
"text-combine-upright: all". Edge and Blink have already been shipping it
unprefixed.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/RKTuYDrsnvo
relnote-firefox: --- → ?
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #6)
> Release Note Request (optional, but appreciated)
> [Suggested wording]: Added vertical text layout support with the
> "text-combine-upright: all" CSS property

We've supported vertical text layout.

Suggested wording: Added support for horizontal-in-vertical (tate-chu-yoko) text via "text-combine-upright: all" CSS property.
What are the plans for testing this? Automated tests, manual? If I'm not mistaking there were tests (from Jonathan Kew?) for CSS writing-mode support.
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #8)
> What are the plans for testing this? Automated tests, manual? If I'm not
> mistaking there were tests (from Jonathan Kew?) for CSS writing-mode support.

Rendering part of this would be tested via automated reftests. There are probably some tests from CSSWG. If the tests are not enough, I'll add some.
Depends on: 1257121
Summary: Add layout support for text-combine-upright (tate-chu-yoko) → Add layout support for "all" value of text-combine-upright (tate-chu-yoko)
Blocks: 1258635
Depends on: 1251995
Depends on: 1258636
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #8)
> there were tests (from Jonathan Kew?) for CSS writing-mode support.

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #9)
> There are probably some tests from CSSWG. If the tests are not enough, I'll add some.

27 Tests from Writing Modes Test suite on text-combine-upright:
http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/chapter-9.htm#s9.1

27 Tests from Writing Modes Test suite on text-combine-upright using the test harness:
http://test.csswg.org/harness/test/css-writing-modes-3_dev/section/9.1/full-width-002/

15 out of the 27 tests have been reviewed and approved:
http://test.csswg.org/shepherd/search/spec/css-writing-modes-3/section/9.1/
and are the parsing tests and the 'digits' value tests.

text-combine-upright test results:
http://test.csswg.org/harness/results/css-writing-modes-3_dev/grouped/section/9.1/

Before running the text-combine-upright tests, it is *_preferable_* that you install the font tcu-font.otf which you can find and download at (direct link):
http://test.csswg.org/source/css-writing-modes-3/support/tcu-font.otf

For tests requiring the Ahem font ( AHEM____.TTF ), you can find and download it from
https://www.w3.org/Style/CSS/Test/Fonts/Ahem/
or from
http://test.csswg.org/source/fonts/ahem/

If you see problems with any of those text-combine-upright tests, please report this to
http://lists.w3.org/Archives/Public/public-css-testsuite/

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #9)
> If the tests are not enough, I'll add some.

Xidorn, let me know if you need help if/when creating more text-combine-upright tests. Mozilla tests are ideal when they meet CSSWG guidelines and when they are ported to CSSWG test suites. It would be best for such tests to re-use the tcu-font.otf for automatability of tests.
Depends on: 1258916
(In reply to Gérard Talbot from comment #10)
> (In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #9)
> > If the tests are not enough, I'll add some.
> 
> Xidorn, let me know if you need help if/when creating more
> text-combine-upright tests. Mozilla tests are ideal when they meet CSSWG
> guidelines and when they are ported to CSSWG test suites. It would be best
> for such tests to re-use the tcu-font.otf for automatability of tests.

It seems the tests are in good shape in general. They seem to cover most of the things.

There is one issue in full-width-001~003 that it doesn't seem to me an upright full-width character in vertical flow is guaranteed to have 1em height, but the spec requires box of text-combine-upright to be a 1em square, so there can be a mismatch. It would need fix.

In addition, use of hwid/twid/qwid is not covered by any of the tests there. Although the exact behavior is somehow impl-dependent, I think the spec can deduce an interoperable behavior for a well-designed font. Font support is probably necessary for testing this. I hope someone could help making tests for that, since I'm not quite familiar with developing fonts.
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #11)
(...)
> There is one issue in full-width-001~003 that it doesn't seem to me an
> upright full-width character in vertical flow is guaranteed to have 1em
> height, but the spec requires box of text-combine-upright to be a 1em
> square, so there can be a mismatch. It would need fix.

I think (and I could be wrong here) you may be referring to this message
[css-writing-modes] on using the advance height of U+6C34
http://lists.w3.org/Archives/Public/www-style/2014Jul/0310.html

I do not understand why the "6" is actually part of the full-width-002 test ...
http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/full-width-002.htm

That test is not precise and not reliable. 

First, that full-width-002.htm uses a very misleading class name "tcy"

.tcy {
  text-transform: full-width;
}

Second, the "6" (6 == &#54; or &x36; or U+0036: In basic latin range: ASCII Digits) versus "6" (6 == &#65302;  or &xFF16; or U+FF16: full-width 6 : In Halfwidth and Fullwidth Forms http://unicode.org/charts/PDF/UFF00.pdf) (you have to have good eyes to notice the difference of glyphs): one is the full-width version of the other!

Third, as coded, it looks like both <p> are tests but they are not (because the wrapping div has the classname "test"). The first (in source code order) <p> should be the test and is the test; the second (in source code order) <p> should be the reference and is the comparing reference. What you should have in that full-width-002.htm test is a structure like this:

<div>
  <p id="test"> ...</p>
  <p id="reference"> ...</p>
</div>

Fourth, as I suspected, the "6" should not be part of that full-width-002.htm test because it is not what's being tested, what is the goal, target of the test (as stated by the test's text assert). The "19" is the sole goal of the test.

That full-width-002.htm test is not reliable precisely because some browsers (Chrome 49+) supports and implements 'text-combine-upright: all' while some others (Firefox 45+) supports and implements 'text-transform: full-width'. By breaking the test into 2 separate and distinct sub-tests, we would clearly and cleanly see which browsers support which properties.

Draft 'text-transform: full-width' test applied on a single ASCII digit:
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/full-width-00x.xht
Firefox 45 passes that full-width-00x.xht test while Chrome 49 and Chrome 51.0.2687.0 fail that full-width-00x.xht test.
I still don't have idea how to make the decoration line work.

Currently, how text-combine-upright works is:
* The writing-mode of text frame is recomputed to horizontal-tb if its parent has vertical writing mode, and text-combine-upright is all. At the same time, a style context flag is set for this. (part 3)
* In text frame code, when reflow, if the width is larger than 1em, it forces the width to 1em and store a scaling factor. The height is always forced to 1em. (part 4)

It effectively makes the tcy text frame an inline-block who is aligned to the dominant baseline, and its ascent is useless.

I have no idea how can I calculate the position of decoration line for this.

jfkthame, are you aware of any document describing how the position of decoration line is computed?
Flags: needinfo?(jfkthame)
I finally understand the math there, and I would try again to make it work. I'd probably fix bug 1220438 first.
Flags: needinfo?(jfkthame)
Depends on: 1220438
No longer depends on: 1258916
Depends on: 1229743
Assignee: nobody → bugzilla
Review commit: https://reviewboard.mozilla.org/r/46643/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46643/
Attachment #8736636 - Attachment description: MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. → MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r?heycam
Attachment #8736637 - Attachment description: MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. → MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r?heycam
Attachment #8736638 - Attachment description: MozReview Request: Bug 1097499 part 3 - Recompute writing-mode of text frame for text-combine-upright. → MozReview Request: Bug 1097499 part 3 - Recompute writing-mode of text frame for text-combine-upright. r?heycam
Attachment #8736639 - Attachment description: MozReview Request: Bug 1097499 part 4 - Layout text combine upright. → MozReview Request: Bug 1097499 part 4 - Layout text combine upright. r?jfkthame
Attachment #8736640 - Attachment description: MozReview Request: Bug 1097499 part 5 - Inherit move direction from parent for horizontal-in-vertical text. → MozReview Request: Bug 1097499 part 5 - Inherit move direction from parent for horizontal-in-vertical text. r?jfkthame
Attachment #8736641 - Attachment description: MozReview Request: Bug 1097499 part 6 - Add reverse function of GetFullWidth. → MozReview Request: Bug 1097499 part 6 - Add reverse function of GetFullWidth. r?emk
Attachment #8736642 - Attachment description: MozReview Request: Bug 1097499 part 7 - Move CountGraphemeClusters to mozilla::unicode. → MozReview Request: Bug 1097499 part 7 - Move CountGraphemeClusters to mozilla::unicode. r?emk
Attachment #8736643 - Attachment description: MozReview Request: Bug 1097499 part 8 - Transform full-width characters to non-full-width correspondents for combined text. → MozReview Request: Bug 1097499 part 8 - Transform full-width characters to non-full-width correspondents for combined text. r?jfkthame
Attachment #8736644 - Attachment description: MozReview Request: Bug 1097499 part 9 - Add fwid/hwid/twid/qwid font feature support to gfx. → MozReview Request: Bug 1097499 part 9 - Add fwid/hwid/twid/qwid font feature support to gfx. r?jfkthame
Attachment #8736645 - Attachment description: MozReview Request: Bug 1097499 part 10 - Set width variant for text-combined frame. → MozReview Request: Bug 1097499 part 10 - Set width variant for text-combined frame. r?jfkthame
Attachment #8736646 - Attachment description: MozReview Request: Bug 1097499 part 11 - Handle spacing sensibly for text-combine-upright. → MozReview Request: Bug 1097499 part 11 - Handle spacing sensibly for text-combine-upright. r?jfkthame
Attachment #8741628 - Flags: review?(jfkthame)
Attachment #8741629 - Flags: review?(jfkthame)
Attachment #8741630 - Flags: review?(jfkthame)
Attachment #8741631 - Flags: review?(jfkthame)
Attachment #8736636 - Flags: review?(cam)
Attachment #8736637 - Flags: review?(cam)
Attachment #8736638 - Flags: review?(cam)
Attachment #8736639 - Flags: review?(jfkthame)
Attachment #8736640 - Flags: review?(jfkthame)
Attachment #8736641 - Flags: review?(VYV03354)
Attachment #8736642 - Flags: review?(VYV03354)
Attachment #8736643 - Flags: review?(jfkthame)
Attachment #8736644 - Flags: review?(jfkthame)
Attachment #8736645 - Flags: review?(jfkthame)
Attachment #8736646 - Flags: review?(jfkthame)
Comment on attachment 8736636 [details]
MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43473/diff/1-2/
Comment on attachment 8736637 [details]
MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43475/diff/1-2/
Comment on attachment 8736638 [details]
MozReview Request: Bug 1097499 part 4 - Adjust computed value of writing-mode on text frames when text-combine-upright is used. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43477/diff/1-2/
Comment on attachment 8736639 [details]
MozReview Request: Bug 1097499 part 5 - Layout text combine upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43479/diff/1-2/
Comment on attachment 8736640 [details]
MozReview Request: Bug 1097499 part 6 - Inherit move direction from parent for horizontal-in-vertical text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43481/diff/1-2/
Comment on attachment 8736641 [details]
MozReview Request: Bug 1097499 part 7 - Add reverse function of GetFullWidth. r=emk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43483/diff/1-2/
Comment on attachment 8736642 [details]
MozReview Request: Bug 1097499 part 8 - Move CountGraphemeClusters to mozilla::unicode. r=emk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43485/diff/1-2/
Comment on attachment 8736643 [details]
MozReview Request: Bug 1097499 part 9 - Transform full-width characters to non-full-width correspondents for combined text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43487/diff/1-2/
Comment on attachment 8736644 [details]
MozReview Request: Bug 1097499 part 10 - Add fwid/hwid/twid/qwid font feature support to gfx. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43489/diff/1-2/
Comment on attachment 8736645 [details]
MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43491/diff/1-2/
Comment on attachment 8736646 [details]
MozReview Request: Bug 1097499 part 12 - Handle spacing sensibly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43493/diff/1-2/
Comment on attachment 8736643 [details]
MozReview Request: Bug 1097499 part 9 - Transform full-width characters to non-full-width correspondents for combined text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43487/diff/2-3/
Comment on attachment 8736644 [details]
MozReview Request: Bug 1097499 part 10 - Add fwid/hwid/twid/qwid font feature support to gfx. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43489/diff/2-3/
Comment on attachment 8736645 [details]
MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43491/diff/2-3/
Comment on attachment 8736646 [details]
MozReview Request: Bug 1097499 part 12 - Handle spacing sensibly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43493/diff/2-3/
Comment on attachment 8741628 [details]
MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46643/diff/1-2/
Comment on attachment 8741629 [details]
MozReview Request: Bug 1097499 part 14 - Draw emphasis marks properly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46645/diff/1-2/
Comment on attachment 8741630 [details]
MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46647/diff/1-2/
Comment on attachment 8741631 [details]
MozReview Request: Bug 1097499 part 16 - Enable text-combine-upright by default. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46649/diff/1-2/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41a6134552c8

One failure so far. It seems the WidthTest font doesn't work as expected on Linux for 'twid'. I'll investigate this issue next week. But it shouldn't be a major issue anyway.
Comment on attachment 8736641 [details]
MozReview Request: Bug 1097499 part 7 - Add reverse function of GetFullWidth. r=emk

https://reviewboard.mozilla.org/r/43483/#review43329

::: intl/unicharutil/util/nsUnicodePropertyData.cpp:1377
(Diff revision 2)
> +  {0x0000,0x0000,0xffe8,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000},
> +  {0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0xffed,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000},
> +  {0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0xffee,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000},
> +  {0x0020,0xff64,0xff61,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0xff62,0xff63,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000},
> +  {0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0xff9e,0xff9f,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0xff67,0xff71,0xff68,0xff72,0xff69,0xff73,0xff6a,0xff74,0xff6b,0xff75,0xff76,0x0000,0xff77,0x0000,0xff78,0x0000,0xff79,0x0000,0xff7a,0x0000,0xff7b,0x0000,0xff7c,0x0000,0xff7d,0x0000,0xff7e,0x0000,0xff7f,0x0000,0xff80},
> +  {0x0000,0xff81,0x0000,0xff6f,0xff82,0x0000,0xff83,0x0000,0xff84,0x0000,0xff85,0xff86,0xff87,0xff88,0xff89,0xff8a,0x0000,0x0000,0xff8b,0x0000,0x0000,0xff8c,0x0000,0x0000,0xff8d,0x0000,0x0000,0xff8e,0x0000,0x0000,0xff8f,0xff90,0xff91,0xff92,0xff93,0xff6c,0xff94,0xff6d,0xff95,0xff6e,0xff96,0xff97,0xff98,0xff99,0xff9a,0xff9b,0x0000,0xff9c,0x0000,0x0000,0xff66,0xff9d,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0x0000,0xff65,0xff70,0x0000,0x0000,0x0000},

Is this conversion useful? For example, this table will not convert U+30AC (ガ) to U+FF76 U+FF9E (ガ).
Attachment #8736641 - Flags: review?(VYV03354)
https://reviewboard.mozilla.org/r/43483/#review43329

> Is this conversion useful? For example, this table will not convert U+30AC (ガ) to U+FF76 U+FF9E (ガ).

The spec only says the reverse algorithm of 'text-transform: full-width' should be applied, so I suppose it doesn't matter a lot. Also it seems to me the main usecase of text-combine-upright doesn't use kanas. But if you have any suggestion about how to provide a better coverage, I'm happy to take as well if not too complicated.
Attachment #8736641 - Flags: review?(VYV03354)
Where is the text-transform: full-width algorithm defined? [CSS3TEXT] is quite vague... But the spec states about the notion of typographic character units, so "ガ" should be considered as a unit.

(I didn't mean to cancel the review request. MozReview sucks.)
I agree that CSS3TEXT is vague on this, and yes, "ガ" should be considered as a unit. And it also says
> This value is typically used to typeset Latin letters and digits as if they were ideographic characters.
so I guess our current implementation at least fulfill this usecase, and the newly-added table at least match our impl of full-width transformation.

Given that the table generating code and query code are put together, we could easily change both in the future when we know how to make it better.

Does it sound reasonable enough to you?
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #54)
> so I guess our current implementation at least fulfill this usecase, and the
> newly-added table at least match our impl of full-width transformation.

Could you make the function name more specific to the current usecase? The current name is too generic.
(In reply to Masatoshi Kimura [:emk] from comment #55)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #54)
> > so I guess our current implementation at least fulfill this usecase, and the
> > newly-added table at least match our impl of full-width transformation.
> 
> Could you make the function name more specific to the current usecase? The
> current name is too generic.

Probably FullWidthInverse? Does it make sense to you?
> and the newly-added table at least match our impl of full-width transformation.

The halfwidth-to-fullwidth conversion is fine because it is 1:n mappings and the implementation will have to select one destination anyway (although it is a bit awkward especially on non-OS X platforms).
But the fullwidth-to-halfwidth conversion is n:1 mappings and the current implementation will convert only one of n possibilities.
The function name should indicate that it is only meant to be used for text-combine-upright.
Then I still think FullWidthInverse should make enough sense, probably with a comment explaning the situation about the 1:n mappings. This is an inverse of FullWidth function anyway, given it keeps the invariant that for every Unicode codepoint c, FullWidthInverse(FullWidth(c)) == c.
What's the reason for putting "digits <integer>?" values behind a separate pref?  Do we plan to ship text-combine-upright:all before digits?  Is it because digits is at risk in the spec?
Comment on attachment 8736637 [details]
MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r=heycam

https://reviewboard.mozilla.org/r/43475/#review43545
Attachment #8736637 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #60)
> What's the reason for putting "digits <integer>?" values behind a separate
> pref?  Do we plan to ship text-combine-upright:all before digits?  Is it
> because digits is at risk in the spec?

Yes, we want to ship "text-combine-upright: all" before "digits", because "digits" is more difficult to implement (it requires splitting text frames based on the content), and less desired according to different sources including people from Japanese ebook companies. Blink doesn't have plan to implement it currently either, and it is at risk in the spec.

It seems the way to spec it isn't completely clear yet as well.

But once we have this patchset land, the only thing necessary for "digits" is splitting frames. All layout parts here should be reusable.
Comment on attachment 8736638 [details]
MozReview Request: Bug 1097499 part 4 - Adjust computed value of writing-mode on text frames when text-combine-upright is used. r=heycam

https://reviewboard.mozilla.org/r/43477/#review43549

::: layout/style/nsStyleContext.cpp:836
(Diff revision 2)
> +  // Change writing mode of text frame for text-combine-upright. It is
> +  // intended to do this after the fixup of display above, because text
> +  // frame never wants a different display value.

Beware that ::-moz-non-element is also used on nsLetterFrames, for the parts that are not ::first-letter.

  <style>
  p { writing-mode: vertical-rl; text-combine-upright: all; }
  p::first-letter { color: blue; }
  </style>
  <p>ab</p>

I think you'll get an nsFirstLetterFrame::-moz-non-element with a child nsTextFrame::-moz-non-element.  Will this do the right thing?

(We should probably have a unique pseudo for text frames.)

::: layout/style/nsStyleContext.cpp:840
(Diff revision 2)
> +      mParent->StyleVisibility()->mWritingMode !=
> +        NS_STYLE_WRITING_MODE_HORIZONTAL_TB &&
> +      mParent->StyleText()->mTextCombineUpright ==
> +        NS_STYLE_TEXT_COMBINE_UPRIGHT_ALL) {

I assume here you are relying on the fact that the values for inherited properties must be the same on the parent as on this ::-moz-non-element.  But what is the advantage to doing this rather than looking up the style on this style context?

::: layout/style/nsStyleContext.cpp:846
(Diff revision 2)
> +        NS_STYLE_WRITING_MODE_HORIZONTAL_TB &&
> +      mParent->StyleText()->mTextCombineUpright ==
> +        NS_STYLE_TEXT_COMBINE_UPRIGHT_ALL) {
> +    nsStyleVisibility* mutableVis = GET_UNIQUE_STYLE_DATA(Visibility);
> +    mutableVis->mWritingMode = NS_STYLE_WRITING_MODE_HORIZONTAL_TB;
> +    AddStyleBit(NS_STYLE_IS_TEXT_COMBINED);

You will need to add a case to ElementRestyler::ComputeRestyleResultFromNewContext for this bit, just like the other four in that function.  Otherwise we might end up deciding to stop restyling at a text frame and leave the old style context on it with an incorrect NS_STYLE_IS_TEXT_COMBINED flag value.
Attachment #8736638 - Flags: review?(cam)
Comment on attachment 8736636 [details]
MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r=heycam

https://reviewboard.mozilla.org/r/43473/#review43565
Attachment #8736636 - Flags: review?(cam) → review+
https://reviewboard.mozilla.org/r/43477/#review43549

> Beware that ::-moz-non-element is also used on nsLetterFrames, for the parts that are not ::first-letter.
> 
>   <style>
>   p { writing-mode: vertical-rl; text-combine-upright: all; }
>   p::first-letter { color: blue; }
>   </style>
>   <p>ab</p>
> 
> I think you'll get an nsFirstLetterFrame::-moz-non-element with a child nsTextFrame::-moz-non-element.  Will this do the right thing?
> 
> (We should probably have a unique pseudo for text frames.)

::first-letter should use CSSPseudoElementType::firstLetter, no? Viewing all callsites of ResolveStyleForNonElement, it seems the only place which is not for text frame is for placeholder. I'm a bit surprised, since I think the last time I checked, the function is only called for text frame... Probably I was wrong.

Anyway, I'm not too worry about placeholder here, though we may want to create a unique pseudo for placeholder and rename -moz-non-element to something like -moz-text... But that can probably be done in a separate bug I suppose?

> I assume here you are relying on the fact that the values for inherited properties must be the same on the parent as on this ::-moz-non-element.  But what is the advantage to doing this rather than looking up the style on this style context?

The StyleVisibility of this style context is probably fine to use, but values inside StyleText could be affected by writing-mode we are changing below.

Reset properties are fine because authors cannot specify them to non-element, but StyleText is an inherited struct, so that's a problem.

Probably I should move this to the top of this function, and asserts if StyleVisibility is already computed.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #65)
> ::first-letter should use CSSPseudoElementType::firstLetter, no? Viewing all
> callsites of ResolveStyleForNonElement, it seems the only place which is not
> for text frame is for placeholder. I'm a bit surprised, since I think the
> last time I checked, the function is only called for text frame... Probably
> I was wrong.

It uses CSSPseudoElementType::firstLetter for the contents that are considered within the ::first-letter, and CSSPseudoElementType::mozNonElement for the rest:

  https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFirstLetterFrame.cpp#67
  https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFirstLetterFrame.cpp#328
Comment on attachment 8736639 [details]
MozReview Request: Bug 1097499 part 5 - Layout text combine upright. r=jfkthame

https://reviewboard.mozilla.org/r/43479/#review43639
Attachment #8736639 - Flags: review?(jfkthame) → review+
Comment on attachment 8736640 [details]
MozReview Request: Bug 1097499 part 6 - Inherit move direction from parent for horizontal-in-vertical text. r=jfkthame

https://reviewboard.mozilla.org/r/43481/#review43641
Attachment #8736640 - Flags: review?(jfkthame) → review+
Comment on attachment 8736643 [details]
MozReview Request: Bug 1097499 part 9 - Transform full-width characters to non-full-width correspondents for combined text. r=jfkthame

https://reviewboard.mozilla.org/r/43487/#review43643
Attachment #8736643 - Flags: review?(jfkthame) → review+
Comment on attachment 8736644 [details]
MozReview Request: Bug 1097499 part 10 - Add fwid/hwid/twid/qwid font feature support to gfx. r=jfkthame

https://reviewboard.mozilla.org/r/43489/#review43645
Attachment #8736644 - Flags: review?(jfkthame) → review+
Comment on attachment 8736645 [details]
MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame

https://reviewboard.mozilla.org/r/43491/#review43647

::: layout/base/nsLayoutUtils.cpp:4112
(Diff revision 3)
> +    } else if (clusters >= 4) {
> +      variantWidth = NS_FONT_VARIANT_WIDTH_QUARTER;

I wonder if it's a good idea to do this for longer strings of text. (They shouldn't usually occur, but on principle....)

The trouble is that if the string is > 4 chars, we're (probably) going to end up having to use some serious scaling to compress it. If it contained some characters (e.g. digits) that support the qwid feature and some that don't (e.g. ideographs) -- which seems plausible if we're dealing with a longer string -- the qwid forms will probably look very compressed already, in comparison to the default glyphs. When we then compress the whole thing, the already-qwid-compressed digits will look out-of-place among their neighbours.

So I wonder if it would generally look better to NOT use qwid on strings of > 4 chars, so that we keep the default glyphs and then just compress them all uniformly?
Attachment #8736645 - Flags: review?(jfkthame)
Comment on attachment 8736646 [details]
MozReview Request: Bug 1097499 part 12 - Handle spacing sensibly for text-combine-upright. r=jfkthame

https://reviewboard.mozilla.org/r/43493/#review43649
Attachment #8736646 - Flags: review?(jfkthame) → review+
Comment on attachment 8741628 [details]
MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r=jfkthame

https://reviewboard.mozilla.org/r/46643/#review43651

::: gfx/thebes/gfxContext.h:646
(Diff revision 2)
>      {
>          MOZ_ASSERT(mContext, "mMatrix doesn't contain a useful matrix");
>          return mMatrix;
>      }
>  
> +    explicit operator bool() const { return !!mContext; }

I'd prefer an explicit method such as

    bool HasContext() const { return mContext != nullptr; }

rather than the `operator bool()` here. I think that makes it clearer what we're doing (both here and at the callsite).

::: layout/generic/nsTextFrame.cpp:5014
(Diff revision 2)
>  
>    bool useOverride = false;
>    nscolor overrideColor = NS_RGBA(0, 0, 0, 0);
>  
>    bool nearestBlockFound = false;
> -  WritingMode wm = GetWritingMode();
> +  WritingMode wm = GetParent()->GetWritingMode();

Please add a comment noting why you're getting the WM from the parent instead of this frame.

(Oh, I see you've got one in DrawTextRunAndDecorations. No need to repeat it at length each time, but a brief note as a reminder would be good.)
Attachment #8741628 - Flags: review?(jfkthame)
Comment on attachment 8741629 [details]
MozReview Request: Bug 1097499 part 14 - Draw emphasis marks properly for text-combine-upright. r=jfkthame

https://reviewboard.mozilla.org/r/46645/#review43659
Attachment #8741629 - Flags: review?(jfkthame) → review+
Comment on attachment 8736641 [details]
MozReview Request: Bug 1097499 part 7 - Add reverse function of GetFullWidth. r=emk

https://reviewboard.mozilla.org/r/43483/#review43661

OK, let's use FullWidthInverse.
Attachment #8736641 - Flags: review?(VYV03354) → review+
Comment on attachment 8736642 [details]
MozReview Request: Bug 1097499 part 8 - Move CountGraphemeClusters to mozilla::unicode. r=emk

https://reviewboard.mozilla.org/r/43485/#review43663
Attachment #8736642 - Flags: review?(VYV03354) → review+
Comment on attachment 8741630 [details]
MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r=jfkthame

https://reviewboard.mozilla.org/r/46647/#review43665

::: layout/reftests/w3c-css/submitted/writing-modes-3/text-combine-upright-compression-004.html:9
(Diff revision 2)
> +<meta charset="UTF-8">
> +<title>CSS Test: text-combine-upright, compression of four characters</title>
> +<link rel="author" href="Xidorn Quan" href="https://www.upsuper.org">
> +<link rel="help" href="https://drafts.csswg.org/css-writing-modes-3/#text-combine-compression">
> +<link rel="match" href="text-combine-upright-compression-004-ref.html">
> +<meta name="assert" href="text-combine-upright should try applying 'fwid' feature if the width is wider than 1em">

s/fwid/qwid/, I believe :)

::: layout/reftests/w3c-css/submitted/writing-modes-3/text-combine-upright-compression-007.html:9
(Diff revision 2)
> +<meta charset="UTF-8">
> +<title>CSS Test: text-combine-upright: all, fit any number of characters</title>
> +<link rel="author" href="Xidorn Quan" href="https://www.upsuper.org">
> +<link rel="help" href="https://drafts.csswg.org/css-writing-modes-3/#text-combine-compression">
> +<link rel="match" href="text-combine-upright-compression-001-ref.html">
> +<meta name="assert" href="text-combine-upright with character not wider than 1em should not trigger compression.">

This looks like the wrong assertion for this test.
Attachment #8741630 - Flags: review?(jfkthame)
Attachment #8741631 - Flags: review?(jfkthame) → review+
Comment on attachment 8741631 [details]
MozReview Request: Bug 1097499 part 16 - Enable text-combine-upright by default. r=jfkthame

https://reviewboard.mozilla.org/r/46649/#review43669

Awesome!
https://reviewboard.mozilla.org/r/43477/#review43549

> You will need to add a case to ElementRestyler::ComputeRestyleResultFromNewContext for this bit, just like the other four in that function.  Otherwise we might end up deciding to stop restyling at a text frame and leave the old style context on it with an incorrect NS_STYLE_IS_TEXT_COMBINED flag value.

I don't understand how could that happen? Could you explain a bit in what condition could that happen?

Also, if this could happen, I suppose HAS_DECORATION_LINES and ruby's SUPPRESS_LINEBREAK would also be affected? I don't see there is code for them in ElementRestyler::ComputeRestyleResultFromNewContext.
Flags: needinfo?(cam)
When I ran the new reftests, I noticed that there are lots of
> WARNING: have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation: 'NS_UNCONSTRAINEDSIZE != aReflowState.AvailableISize()', file c:/mozilla-source/central/layout/generic/nsLineLayout.cpp, line 1199

I investigated a bit, and the code triggering the warning is in nsLineLayout::AllowForStartMargin:
>   } else {
>     NS_WARN_IF_FALSE(NS_UNCONSTRAINEDSIZE != aReflowState.AvailableISize(),
>                      "have unconstrained inline-size; this should only result "
>                      "from very large sizes, not attempts at intrinsic "
>                      "inline-size calculation");
>     if (NS_UNCONSTRAINEDSIZE == aReflowState.ComputedISize()) {
>       // For inline-ish and text-ish things (which don't compute widths
>       // in the reflow state), adjust available inline-size to account for the
>       // start margin. The end margin will be accounted for when we
>       // finish flowing the frame.
>       WritingMode wm = aReflowState.GetWritingMode();
>       aReflowState.AvailableISize() -=
>           pfd->mMargin.ConvertTo(wm, lineWM).IStart(wm);
>     }
>   }
which is called from nsLineLayout::ReflowFrame which includes the following code just before calling that function:
>   if (reflowState.ComputedISize() == NS_UNCONSTRAINEDSIZE) {
>     reflowState.AvailableISize() = availableSpaceOnLine;
>   }
and since ComputedISize() is not NS_UNCONSTRAINEDSIZE, AvailableISize() is not set to something else here.

Given the code listed above, I guess the warning isn't really useful. It should probably be moved into the if-block (and we can merge the "if" with the "else"). Alternatively, the code in nsLineLayout::ReflowFrame should probably set AvailableISize() unconditionally.

jfkthame, what do you think?
Flags: needinfo?(jfkthame)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #79)
> https://reviewboard.mozilla.org/r/43477/#review43549
> 
> Also, if this could happen, I suppose HAS_DECORATION_LINES and ruby's
> SUPPRESS_LINEBREAK would also be affected? I don't see there is code for
> them in ElementRestyler::ComputeRestyleResultFromNewContext.

It seems I was wrong. Both of those flags have a check in ElementRestyler::ComputeRestyleResultFromNewContext.

And according to the discussion in IRC, it seems we wouldn't leave an incorrect state of style context, having a check in that function may make restyling faster because we can skip comparing the style structs. So it's probably still worth adding one.
Flags: needinfo?(cam)
Review commit: https://reviewboard.mozilla.org/r/47597/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47597/
Attachment #8736636 - Attachment description: MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r?heycam → MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r=heycam
Attachment #8736637 - Attachment description: MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r?heycam → MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r=heycam
Attachment #8736638 - Attachment description: MozReview Request: Bug 1097499 part 3 - Recompute writing-mode of text frame for text-combine-upright. r?heycam → MozReview Request: Bug 1097499 part 4 - Compute style context of text frame for text-combine-upright. r?heycam
Attachment #8736639 - Attachment description: MozReview Request: Bug 1097499 part 4 - Layout text combine upright. r?jfkthame → MozReview Request: Bug 1097499 part 5 - Layout text combine upright. r=jfkthame
Attachment #8736640 - Attachment description: MozReview Request: Bug 1097499 part 5 - Inherit move direction from parent for horizontal-in-vertical text. r?jfkthame → MozReview Request: Bug 1097499 part 6 - Inherit move direction from parent for horizontal-in-vertical text. r=jfkthame
Attachment #8736641 - Attachment description: MozReview Request: Bug 1097499 part 6 - Add reverse function of GetFullWidth. r?emk → MozReview Request: Bug 1097499 part 7 - Add reverse function of GetFullWidth. r=emk
Attachment #8736642 - Attachment description: MozReview Request: Bug 1097499 part 7 - Move CountGraphemeClusters to mozilla::unicode. r?emk → MozReview Request: Bug 1097499 part 8 - Move CountGraphemeClusters to mozilla::unicode. r=emk
Attachment #8736643 - Attachment description: MozReview Request: Bug 1097499 part 8 - Transform full-width characters to non-full-width correspondents for combined text. r?jfkthame → MozReview Request: Bug 1097499 part 9 - Transform full-width characters to non-full-width correspondents for combined text. r=jfkthame
Attachment #8736644 - Attachment description: MozReview Request: Bug 1097499 part 9 - Add fwid/hwid/twid/qwid font feature support to gfx. r?jfkthame → MozReview Request: Bug 1097499 part 10 - Add fwid/hwid/twid/qwid font feature support to gfx. r=jfkthame
Attachment #8736645 - Attachment description: MozReview Request: Bug 1097499 part 10 - Set width variant for text-combined frame. r?jfkthame → MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r?jfkthame
Attachment #8736646 - Attachment description: MozReview Request: Bug 1097499 part 11 - Handle spacing sensibly for text-combine-upright. r?jfkthame → MozReview Request: Bug 1097499 part 12 - Handle spacing sensibly for text-combine-upright. r=jfkthame
Attachment #8741628 - Attachment description: MozReview Request: Bug 1097499 part 12 - Draw decoration line properly for text-combine-upright. r?jfkthame → MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r?jfkthame
Attachment #8741629 - Attachment description: MozReview Request: Bug 1097499 part 13 - Draw emphasis marks properly for text-combine-upright. r?jfkthame → MozReview Request: Bug 1097499 part 14 - Draw emphasis marks properly for text-combine-upright. r=jfkthame
Attachment #8741630 - Attachment description: MozReview Request: Bug 1097499 part 14 - Add reftests for text-combine-upright. r?jfkthame → MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r?jfkthame
Attachment #8741631 - Attachment description: MozReview Request: Bug 1097499 part 15 - Enable text-combine-upright by default. r?jfkthame → MozReview Request: Bug 1097499 part 16 - Enable text-combine-upright by default. r=jfkthame
Attachment #8743136 - Flags: review?(cam)
Attachment #8736638 - Flags: review?(cam)
Attachment #8736645 - Flags: review?(jfkthame)
Attachment #8741628 - Flags: review?(jfkthame)
Attachment #8741630 - Flags: review?(jfkthame)
Comment on attachment 8736636 [details]
MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43473/diff/2-3/
Comment on attachment 8736637 [details]
MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43475/diff/2-3/
Comment on attachment 8736638 [details]
MozReview Request: Bug 1097499 part 4 - Adjust computed value of writing-mode on text frames when text-combine-upright is used. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43477/diff/2-3/
Comment on attachment 8736639 [details]
MozReview Request: Bug 1097499 part 5 - Layout text combine upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43479/diff/2-3/
Comment on attachment 8736640 [details]
MozReview Request: Bug 1097499 part 6 - Inherit move direction from parent for horizontal-in-vertical text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43481/diff/2-3/
Comment on attachment 8736641 [details]
MozReview Request: Bug 1097499 part 7 - Add reverse function of GetFullWidth. r=emk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43483/diff/2-3/
Comment on attachment 8736642 [details]
MozReview Request: Bug 1097499 part 8 - Move CountGraphemeClusters to mozilla::unicode. r=emk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43485/diff/2-3/
Comment on attachment 8736643 [details]
MozReview Request: Bug 1097499 part 9 - Transform full-width characters to non-full-width correspondents for combined text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43487/diff/3-4/
Comment on attachment 8736644 [details]
MozReview Request: Bug 1097499 part 10 - Add fwid/hwid/twid/qwid font feature support to gfx. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43489/diff/3-4/
Comment on attachment 8736645 [details]
MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43491/diff/3-4/
Comment on attachment 8736646 [details]
MozReview Request: Bug 1097499 part 12 - Handle spacing sensibly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43493/diff/3-4/
Comment on attachment 8741628 [details]
MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46643/diff/2-3/
Comment on attachment 8741629 [details]
MozReview Request: Bug 1097499 part 14 - Draw emphasis marks properly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46645/diff/2-3/
Comment on attachment 8741630 [details]
MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46647/diff/2-3/
Comment on attachment 8741631 [details]
MozReview Request: Bug 1097499 part 16 - Enable text-combine-upright by default. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46649/diff/2-3/
Attachment #8736645 - Flags: review?(jfkthame) → review+
Comment on attachment 8736645 [details]
MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame

https://reviewboard.mozilla.org/r/43491/#review44401
Comment on attachment 8741628 [details]
MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r=jfkthame

https://reviewboard.mozilla.org/r/46643/#review44403

::: gfx/thebes/gfxContext.h:646
(Diff revision 3)
>      {
>          MOZ_ASSERT(mContext, "mMatrix doesn't contain a useful matrix");
>          return mMatrix;
>      }
>  
> +    bool HasContext() const { return !!mContext; }

Hmm, I know I suggested this, but on re-reading the code (at the callsite, in particular), I wonder if calling it HasMatrix() would be better? That's what we actually care about there... the presence of a non-null mContext just serves (internally in the restorer) as the easy way to test whether we've initialized the matrix to something useful or not.

See what you think - go with whichever you prefer, I can live with it either way.
Attachment #8741628 - Flags: review?(jfkthame) → review+
Comment on attachment 8741630 [details]
MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r=jfkthame

https://reviewboard.mozilla.org/r/46647/#review44405
Attachment #8741630 - Flags: review?(jfkthame) → review+
Hmm, looks like something is busted in nsCaseTransformTextRunFactory, according to try. But I'm sure you're looking into that.
(In reply to Jonathan Kew (:jfkthame) from comment #102)
> Hmm, looks like something is busted in nsCaseTransformTextRunFactory,
> according to try. But I'm sure you're looking into that.

That has been fixed long ago :) The latest try result is in comment 82.
Cool, I didn't think you'd overlook it! :) I guess I followed an older try link from mozreview, which looked a bit alarming.
Attachment #8743136 - Flags: review?(cam)
Comment on attachment 8743136 [details]
MozReview Request: Bug 1097499 part 3 - Add a separate anonbox for text nodes. r=heycam

https://reviewboard.mozilla.org/r/47597/#review44723

::: layout/base/RestyleManager.cpp:3927
(Diff revision 1)
> -  }
> -  else if (pseudoTag == nsCSSAnonBoxes::mozNonElement) {
> +  } else if (pseudoTag == nsCSSAnonBoxes::mozText) {
> +    NS_ASSERTION(aSelf->GetContent(),
> +                 "non pseudo-element frame without content node");
> +    newContext = styleSet->ResolveStyleForText(parentContext);
> +  } else if (pseudoTag == nsCSSAnonBoxes::mozNonElement) {
>      NS_ASSERTION(aSelf->GetContent(),
>                   "non pseudo-element frame without content node");
>      newContext = styleSet->ResolveStyleForNonElement(parentContext);
>    }

Per a later comment to just keep a single ResolveStyleForNonElement method, re-merge these two branches.

::: layout/generic/nsFlexContainerFrame.cpp:998
(Diff revision 1)
>      nsIAtom* pseudoTag = aFrame->StyleContext()->GetPseudo();
>  
>      // If aFrame isn't an anonymous container, then it'll do.
>      if (!pseudoTag ||                                 // No pseudotag.
>          !nsCSSAnonBoxes::IsAnonBox(pseudoTag) ||      // Pseudotag isn't anon.
> -        pseudoTag == nsCSSAnonBoxes::mozNonElement) { // Text, not a container.
> +        nsCSSAnonBoxes::IsNonElement(pseudoTag)) {    // Text, not a container.

I wonder if we can get in here for an nsFirstLetterFrame?  If so, it might be an underyling bug, but I'm not sure how flexbox and ::first-letter interact here.  Ask dholbert?

::: layout/style/nsCSSAnonBoxList.h:22
(Diff revision 1)
> +CSS_ANON_BOX(mozText, ":-moz-text")
>  CSS_ANON_BOX(mozNonElement, ":-moz-non-element")

We use "non-element" as a term to describe these pseudos that don't match any rules, which is what your new nsCSSAnonBoxes::IsNonElement method is testing.  But nsCSSAnonBoxes::mozNonElement is only one of the specific types of non-elements, so I think this could be confusing.

How about we keep the term "non-element" as meaning either of these two pseudos, and rename ::-moz-non-element to ::-moz-other-non-element.

And can you please add a comment above these two entries describing how they are used (as non-styled things), including what existing uses we have for the ::-moz-other-non-element (which I think is just (a) placeholder frames, and (b) nsFirstLetterFrames for content outside the ::first-letter).


Since we rely on these never matching any rules, can you please change CSSParserImpl::ParsePseudoSelector to prevent these from matching (and add a warning, too, in case someone does add one to a UA sheet).

::: layout/style/nsStyleSet.h:165
(Diff revision 1)
> -  // Get a style context for a non-element (which no rules will match),
> -  // such as text nodes, placeholder frames, and the nsFirstLetterFrame
> -  // for everything after the first letter.
> +  // Get a style context for a text node which no rules will match.
> +  //
> +  // It is listed separately from non-element because we sometimes want
> +  // to specify a different style to the text frame.
> +  already_AddRefed<nsStyleContext>
> +  ResolveStyleForText(nsStyleContext* aParentContext);

I think we should just keep a single method, ResolveStyleForNonElement, but pass in the nsIAtom* for one of the two anon boxes we can use.  And then assert that it's IsNonElement().
Comment on attachment 8736638 [details]
MozReview Request: Bug 1097499 part 4 - Adjust computed value of writing-mode on text frames when text-combine-upright is used. r=heycam

https://reviewboard.mozilla.org/r/43477/#review44715

Can you improve the commit message?  Maybe "Adjust computed value of writing-mode on text frames when text-combine-upright is used."?

::: layout/style/nsStyleContext.h:207
(Diff revision 3)
>    // Does this style context or any of its ancestors have display:none set?
>    bool IsInDisplayNoneSubtree() const
>      { return !!(mBits & NS_STYLE_IN_DISPLAY_NONE_SUBTREE); }
>  
> +  // Is this horizontal-in-vertical (tate-chu-yoko) text? This flag can
> +  // only be set for non-element pseudo.

To be more specific, can you say:

This flag is only set on style contexts whose pseudo is nsCSSAnonBoxes::mozText.

("can only be set" sounds like something is preventing this flag from being set on other pseudos, but I don't think that's the case.)

::: layout/style/nsStyleContext.cpp:645
(Diff revision 3)
> +  // Change writing mode of text frame for text-combine-upright. We use
> +  // style structs of the parent to avoid triggering computation before
> +  // we change the writing mode.

Mention in the comment that it's safe to look at the parent style because (a) we're looking at inherited properties, and (b) ::-moz-text never matches any rules.

::: layout/style/nsStyleContext.cpp:653
(Diff revision 3)
> +    MOZ_ASSERT(!PeekStyleVisibility(),
> +               "StyleVisibility must not have been computed");

Does this mean "We don't expect StyleVisibility to be computed yet, which makes this GET_UNQIUE_STYLE_DATA less wasteful, but things would still be correct if it had been computed" or "If StyleVisibility was already computed, some properties might have been computed incorrectly based on the old writing-mode value"?  (I guess it's the latter.)  Can you clarify in the assertion message?
Attachment #8736638 - Flags: review?(cam) → review+
Comment on attachment 8736636 [details]
MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43473/diff/3-4/
Attachment #8736638 - Attachment description: MozReview Request: Bug 1097499 part 4 - Compute style context of text frame for text-combine-upright. r?heycam → MozReview Request: Bug 1097499 part 4 - Adjust computed value of writing-mode on text frames when text-combine-upright is used. r=heycam
Attachment #8736645 - Attachment description: MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r?jfkthame → MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame
Attachment #8741628 - Attachment description: MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r?jfkthame → MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r=jfkthame
Attachment #8741630 - Attachment description: MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r?jfkthame → MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r=jfkthame
Attachment #8743136 - Flags: review?(cam)
Comment on attachment 8736637 [details]
MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43475/diff/3-4/
Comment on attachment 8743136 [details]
MozReview Request: Bug 1097499 part 3 - Add a separate anonbox for text nodes. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47597/diff/1-2/
Comment on attachment 8736638 [details]
MozReview Request: Bug 1097499 part 4 - Adjust computed value of writing-mode on text frames when text-combine-upright is used. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43477/diff/3-4/
Comment on attachment 8736639 [details]
MozReview Request: Bug 1097499 part 5 - Layout text combine upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43479/diff/3-4/
Comment on attachment 8736640 [details]
MozReview Request: Bug 1097499 part 6 - Inherit move direction from parent for horizontal-in-vertical text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43481/diff/3-4/
Comment on attachment 8736641 [details]
MozReview Request: Bug 1097499 part 7 - Add reverse function of GetFullWidth. r=emk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43483/diff/3-4/
Comment on attachment 8736642 [details]
MozReview Request: Bug 1097499 part 8 - Move CountGraphemeClusters to mozilla::unicode. r=emk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43485/diff/3-4/
Comment on attachment 8736643 [details]
MozReview Request: Bug 1097499 part 9 - Transform full-width characters to non-full-width correspondents for combined text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43487/diff/4-5/
Comment on attachment 8736644 [details]
MozReview Request: Bug 1097499 part 10 - Add fwid/hwid/twid/qwid font feature support to gfx. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43489/diff/4-5/
Comment on attachment 8736645 [details]
MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43491/diff/4-5/
Comment on attachment 8736646 [details]
MozReview Request: Bug 1097499 part 12 - Handle spacing sensibly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43493/diff/4-5/
Comment on attachment 8741628 [details]
MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46643/diff/3-4/
Comment on attachment 8741629 [details]
MozReview Request: Bug 1097499 part 14 - Draw emphasis marks properly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46645/diff/3-4/
Comment on attachment 8741630 [details]
MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46647/diff/3-4/
Comment on attachment 8741631 [details]
MozReview Request: Bug 1097499 part 16 - Enable text-combine-upright by default. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46649/diff/3-4/
Comment on attachment 8743136 [details]
MozReview Request: Bug 1097499 part 3 - Add a separate anonbox for text nodes. r=heycam

https://reviewboard.mozilla.org/r/47597/#review44765

Looks good, thanks.

::: layout/style/nsStyleSet.h:171
(Diff revisions 1 - 2)
> -  // Perhaps this should go away and we shouldn't even create style
> -  // contexts for such content nodes.  However, not doing any rule
> -  // matching for them is a first step.
> +  // Perhaps mozOtherNonElement should go away and we shouldn't even
> +  // create style contexts for such content nodes.  However, not doing
> +  // any rule matching for them is a first step.

I would mention the possibility of not resolving style contexts for text frames when not in the presence of text-combine-upright.  (ResolveStyleContextForNonElement is by far mostly called for text frames, so it's that case that we'd want to optimize if possible.)
Attachment #8743136 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #105)
> Comment on attachment 8743136 [details]
> MozReview Request: Bug 1097499 part 3 - Add a separate anonbox for text
> nodes. r?heycam
> 
> https://reviewboard.mozilla.org/r/47597/#review44723
> 
> ::: layout/generic/nsFlexContainerFrame.cpp:998
> (Diff revision 1)
> >      nsIAtom* pseudoTag = aFrame->StyleContext()->GetPseudo();
> >  
> >      // If aFrame isn't an anonymous container, then it'll do.
> >      if (!pseudoTag ||                                 // No pseudotag.
> >          !nsCSSAnonBoxes::IsAnonBox(pseudoTag) ||      // Pseudotag isn't anon.
> > -        pseudoTag == nsCSSAnonBoxes::mozNonElement) { // Text, not a container.
> > +        nsCSSAnonBoxes::IsNonElement(pseudoTag)) {    // Text, not a container.
> 
> I wonder if we can get in here for an nsFirstLetterFrame?  If so, it might
> be an underyling bug, but I'm not sure how flexbox and ::first-letter
> interact here.  Ask dholbert?

dholbert, do you think we can get in here for an nsFirstLetterFrame after ::first-letter? pseudoTag could be non-element only for placeholder, text, and nsFirstLetterFrame continuation after ::first-letter. Do you think there would be any issue when interacting with flexbox here?
Flags: needinfo?(dholbert)
Comment on attachment 8736636 [details]
MozReview Request: Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43473/diff/4-5/
Attachment #8743136 - Attachment description: MozReview Request: Bug 1097499 part 3 - Add a separate anonbox for text nodes. r?heycam → MozReview Request: Bug 1097499 part 3 - Add a separate anonbox for text nodes. r=heycam
Comment on attachment 8736637 [details]
MozReview Request: Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43475/diff/4-5/
Comment on attachment 8743136 [details]
MozReview Request: Bug 1097499 part 3 - Add a separate anonbox for text nodes. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47597/diff/2-3/
Comment on attachment 8736638 [details]
MozReview Request: Bug 1097499 part 4 - Adjust computed value of writing-mode on text frames when text-combine-upright is used. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43477/diff/4-5/
Comment on attachment 8736639 [details]
MozReview Request: Bug 1097499 part 5 - Layout text combine upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43479/diff/4-5/
Comment on attachment 8736640 [details]
MozReview Request: Bug 1097499 part 6 - Inherit move direction from parent for horizontal-in-vertical text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43481/diff/4-5/
Comment on attachment 8736641 [details]
MozReview Request: Bug 1097499 part 7 - Add reverse function of GetFullWidth. r=emk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43483/diff/4-5/
Comment on attachment 8736642 [details]
MozReview Request: Bug 1097499 part 8 - Move CountGraphemeClusters to mozilla::unicode. r=emk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43485/diff/4-5/
Comment on attachment 8736643 [details]
MozReview Request: Bug 1097499 part 9 - Transform full-width characters to non-full-width correspondents for combined text. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43487/diff/5-6/
Comment on attachment 8736644 [details]
MozReview Request: Bug 1097499 part 10 - Add fwid/hwid/twid/qwid font feature support to gfx. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43489/diff/5-6/
Comment on attachment 8736645 [details]
MozReview Request: Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43491/diff/5-6/
Comment on attachment 8736646 [details]
MozReview Request: Bug 1097499 part 12 - Handle spacing sensibly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43493/diff/5-6/
Comment on attachment 8741628 [details]
MozReview Request: Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46643/diff/4-5/
Comment on attachment 8741629 [details]
MozReview Request: Bug 1097499 part 14 - Draw emphasis marks properly for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46645/diff/4-5/
Comment on attachment 8741630 [details]
MozReview Request: Bug 1097499 part 15 - Add reftests for text-combine-upright. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46647/diff/4-5/
Comment on attachment 8741631 [details]
MozReview Request: Bug 1097499 part 16 - Enable text-combine-upright by default. r=jfkthame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46649/diff/4-5/
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #124)
> dholbert, do you think we can get in here for an nsFirstLetterFrame after
> ::first-letter? pseudoTag could be non-element only for placeholder, text,
> and nsFirstLetterFrame continuation after ::first-letter. Do you think there
> would be any issue when interacting with flexbox here?

(This is in GetFirstNonAnonBoxDescendant.)

No, we should never hit that comparison for a nsFirstLetterFrame.

My reasoning:
 (1) ::first-letter has no effect on a flex container (which it looks like I still need to add tests for, bug 828651).
 (2) Any nsFirstLetterFrames that live inside of a nsFirstLetterFrame must necessarily be children of a non-anonymous box (because there must be a ::first-letter style rule that's applied to some non-anonymous thing).
 (3) This method (GetFirstNonAnonBoxDescendant) would stop when it hits that non-anonymous thing (the parent of the nsFirstLetterFrame).
Flags: needinfo?(dholbert)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5816f7da8d5b801a0e15cf9c12f4cb118fc14875
Bug 1097499 part 1 - Control support of 'text-combine-upright: digits' via a separate pref. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5bf61c4ab96104dc1ce7465331093b08a9fdc2
Bug 1097499 part 2 - Add a macro to simplify usage of nsStyleContext::GetUniqueStyleData. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/de6c6c719ef5d93ba032e09db0c2c85f10f7d723
Bug 1097499 part 3 - Add a separate anonbox for text nodes. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/400f5b753ae00255f18dd2d3f3ea88ca211302d9
Bug 1097499 part 4 - Adjust computed value of writing-mode on text frames when text-combine-upright is used. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/6373a28c2d743636969afe25a215ad66b1e1751c
Bug 1097499 part 5 - Layout text combine upright. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/c582f2934be9efa80e5e0ec8b7ff482277fdf97d
Bug 1097499 part 6 - Inherit move direction from parent for horizontal-in-vertical text. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/555af8b37aace3843df75a4f04cfa75f2e34e14c
Bug 1097499 part 7 - Add reverse function of GetFullWidth. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/02e33a3ddffd74907e356c33eabd7e82b0191e62
Bug 1097499 part 8 - Move CountGraphemeClusters to mozilla::unicode. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4eea6eb300398971025a95baed98c068fe146b
Bug 1097499 part 9 - Transform full-width characters to non-full-width correspondents for combined text. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/de351d24e7d74b1d76a9bcec89296b487e81f273
Bug 1097499 part 10 - Add fwid/hwid/twid/qwid font feature support to gfx. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/f851031ac21e42334c1e63cc20b3b4639ad98658
Bug 1097499 part 11 - Set width variant for text-combined frame. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/d238e12a7f717b577ecceefa4e8e93951ee9437c
Bug 1097499 part 12 - Handle spacing sensibly for text-combine-upright. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/45acf5ee3c4222765919340d3f7abace373bab56
Bug 1097499 part 13 - Draw decoration line properly for text-combine-upright. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/660f1684ab37d18791bd03f6121813f6f3b2a449
Bug 1097499 part 14 - Draw emphasis marks properly for text-combine-upright. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/23604d09e7a08796fc6cf23a1b0d4a33f5b45077
Bug 1097499 part 15 - Add reftests for text-combine-upright. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/b797895bca880a5607fc772a9e00da59029af823
Bug 1097499 part 16 - Enable text-combine-upright by default. r=jfkthame
Filed bug 1266645 for that issue. Cancel the ni? here.
Flags: needinfo?(jfkthame)
Xidorn,

"
If a box has a different writing-mode value than its containing block:

    If the box has a specified display of inline, its display computes to inline-block. [CSS21]
"
3.1. Block Flow Direction: the writing-mode property
https://drafts.csswg.org/css-writing-modes-3/#block-flow

So, do you really need to set display to inline-block for your span with class "fake-tcy" ?

- - - - - - 

These other declarations in .fake-tcy

 width: 1em;
 height: 1em;
 text-align: center;
 line-height: 1em;

also seem - to me - to be unneeded or unnecessary. line-height is inherited by definition.

- - - - - -

The class you named fake-tcy I would name it "reference" or "comparison". 

The class you named tcy I would name it "test".

And the div with the class test I would not use any class on it.
(In reply to Gérard Talbot from comment #144)
> Xidorn,
> 
> "
> If a box has a different writing-mode value than its containing block:
> 
>     If the box has a specified display of inline, its display computes to
> inline-block. [CSS21]
> "
> 3.1. Block Flow Direction: the writing-mode property
> https://drafts.csswg.org/css-writing-modes-3/#block-flow
> 
> So, do you really need to set display to inline-block for your span with
> class "fake-tcy" ?

Yeah, that's probably not necessary.

> These other declarations in .fake-tcy
> 
>  width: 1em;
>  height: 1em;
>  text-align: center;
>  line-height: 1em;
> 
> also seem - to me - to be unneeded or unnecessary. line-height is inherited
> by definition.

None of the four rules seems redundant to me. Note the default line-height is not 1em, but a implementation-dependent value.

Removing any of the four lines may cause a test failure I suppose. You can probably try.

> The class you named fake-tcy I would name it "reference" or "comparison". 
> 
> The class you named tcy I would name it "test".

"test" and "reference" are vague name not referring to what the usage of the classes. They are for making a span "tcy" and a simulated "tcy", so the names make sense.

If you call them "test" and "reference", you may not be able to reuse them in any other test due to potential name conflicts. But with the current names, I can use them to rewrite some of the existing test to make them work properly, e.g.
> <p><span class="fake-tcy">6</span>月<span class="fake-tcy">19</span>日</p>
in some of the full-width* tests.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #145)

> > These other declarations in .fake-tcy
> > 
> >  width: 1em;
> >  height: 1em;
> >  text-align: center;
> >  line-height: 1em;
> > 
> > also seem - to me - to be unneeded or unnecessary. line-height is inherited
> > by definition.
> 
> None of the four rules seems redundant to me. Note the default line-height
> is not 1em, but a implementation-dependent value.

The computed value of line-height (set on parent) is inherited from div.test (specifying 72px/1) into the span.fake-tcy . The developer inspector tool shows that too.

> Removing any of the four lines may cause a test failure I suppose. You can
> probably try.

I haven't tried yet with today's nightly built but I tried it with Chrome 51.

> > The class you named fake-tcy I would name it "reference" or "comparison". 
> > 
> > The class you named tcy I would name it "test".
> 
> "test" and "reference" are vague name not referring to what the usage of the
> classes. They are for making a span "tcy" and a simulated "tcy", so the
> names make sense.

They do make sense. And "test" and "reference" are generic.

Thank you for fixing this bug!

Gérard
Depends on: 1268347
Added to Fx48 Aurora release notes
Let's link the property in the release note to https://developer.mozilla.org/en-US/docs/Web/CSS/text-combine-upright.

Sebastian
Flags: needinfo?(rkothari)
(In reply to Sebastian Zartner [:sebo] from comment #149)
> Let's link the property in the release note to
> https://developer.mozilla.org/en-US/docs/Web/CSS/text-combine-upright.
> 
> Sebastian

Done!
Flags: needinfo?(rkothari)
(In reply to Ritu Kothari (:ritu) from comment #150)
> (In reply to Sebastian Zartner [:sebo] from comment #149)
> > Let's link the property in the release note to
> > https://developer.mozilla.org/en-US/docs/Web/CSS/text-combine-upright.
> > 
> > Sebastian
> 
> Done!

Thanks! And I have updated the documentation accordingly.

Sebastian
When I imported the tests to the CSSWG repository, I got:

> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-001-ref.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-001.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-002-ref.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-002.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-003-ref.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-003.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-004-ref.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-004.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-005-ref.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-005.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-005a.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-006-ref.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-006.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-006a.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-007-ref.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). No author specified.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-007.html status changed to 'Needs Work' due to error:
> remote: Author link missing name (title attribute). Assert meta missing content attribute. No author specified.

check-for-references.sh also said:
Missing link for ./writing-modes-3/text-combine-upright-compression-007.html
(meaning it has no rel=match)
Flags: needinfo?(bugzilla)
Pushed the fix ^
Flags: needinfo?(bugzilla)
Still missed part of it:

> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-001-ref.html status changed to 'Resubmitted for Review'
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-001.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-002-ref.html status changed to 'Resubmitted for Review'
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-002.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-003-ref.html status changed to 'Resubmitted for Review'
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-003.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-004-ref.html status changed to 'Resubmitted for Review'
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-004.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-005-ref.html status changed to 'Resubmitted for Review'
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-005.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-005a.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-006-ref.html status changed to 'Resubmitted for Review'
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-006.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-006a.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-007-ref.html status changed to 'Resubmitted for Review'
> remote: vendor-imports/mozilla/mozilla-central-reftests/writing-modes-3/text-combine-upright-compression-007.html status remains as 'Needs Work' due to error:
> remote: Assert meta missing content attribute.
Flags: needinfo?(bugzilla)
Hopefully that's all :)
Flags: needinfo?(bugzilla)
Depends on: 1494380
You need to log in before you can comment on or make changes to this bug.