Closed
Bug 1097499
Opened 10 years ago
Closed 9 years ago
Add layout support for "all" value of text-combine-upright (tate-chu-yoko)
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla48
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)
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
emk
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
emk
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
|
Details |
(deleted),
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.
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 1•9 years ago
|
||
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.
Comment 3•9 years ago
|
||
text-combine-upright tests:
http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/chapter-9.htm
Test results:
http://test.csswg.org/harness/results/css-writing-modes-3_dev/grouped/section/9.1/
Updated•9 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 4•9 years ago
|
||
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]
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
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:
--- → ?
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Summary: Add layout support for text-combine-upright (tate-chu-yoko) → Add layout support for "all" value of text-combine-upright (tate-chu-yoko)
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
(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.
Comment 12•9 years ago
|
||
(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 ...
Comment 13•9 years ago
|
||
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 == 6 or &x36; or U+0036: In basic latin range: ASCII Digits) versus "6" (6 == 6 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.
Assignee | ||
Comment 14•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43473/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43473/
Assignee | ||
Comment 15•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43475/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43475/
Assignee | ||
Comment 16•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43477/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43477/
Assignee | ||
Comment 17•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43479/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43479/
Assignee | ||
Comment 18•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43481/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43481/
Assignee | ||
Comment 19•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43483/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43483/
Assignee | ||
Comment 20•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43485/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43485/
Assignee | ||
Comment 21•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43487/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43487/
Assignee | ||
Comment 22•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43489/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43489/
Assignee | ||
Comment 23•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43491/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43491/
Assignee | ||
Comment 24•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43493/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43493/
Assignee | ||
Comment 25•9 years ago
|
||
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)
Assignee | ||
Comment 26•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bugzilla
Assignee | ||
Comment 27•9 years ago
|
||
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)
Assignee | ||
Comment 28•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46645/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46645/
Assignee | ||
Comment 29•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46647/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46647/
Assignee | ||
Comment 30•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46649/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46649/
Assignee | ||
Comment 31•9 years ago
|
||
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/
Assignee | ||
Comment 32•9 years ago
|
||
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/
Assignee | ||
Comment 33•9 years ago
|
||
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/
Assignee | ||
Comment 34•9 years ago
|
||
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/
Assignee | ||
Comment 35•9 years ago
|
||
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/
Assignee | ||
Comment 36•9 years ago
|
||
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/
Assignee | ||
Comment 37•9 years ago
|
||
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/
Assignee | ||
Comment 38•9 years ago
|
||
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/
Assignee | ||
Comment 39•9 years ago
|
||
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/
Assignee | ||
Comment 40•9 years ago
|
||
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/
Assignee | ||
Comment 41•9 years ago
|
||
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/
Assignee | ||
Comment 42•9 years ago
|
||
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/
Assignee | ||
Comment 43•9 years ago
|
||
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/
Assignee | ||
Comment 44•9 years ago
|
||
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/
Assignee | ||
Comment 45•9 years ago
|
||
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/
Assignee | ||
Comment 46•9 years ago
|
||
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/
Assignee | ||
Comment 47•9 years ago
|
||
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/
Assignee | ||
Comment 48•9 years ago
|
||
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/
Assignee | ||
Comment 49•9 years ago
|
||
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/
Assignee | ||
Comment 50•9 years ago
|
||
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 51•9 years ago
|
||
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)
Assignee | ||
Comment 52•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8736641 -
Flags: review?(VYV03354)
Comment 53•9 years ago
|
||
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.)
Assignee | ||
Comment 54•9 years ago
|
||
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?
Comment 55•9 years ago
|
||
(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.
Assignee | ||
Comment 56•9 years ago
|
||
(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?
Comment 57•9 years ago
|
||
> 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.
Comment 58•9 years ago
|
||
The function name should indicate that it is only meant to be used for text-combine-upright.
Assignee | ||
Comment 59•9 years ago
|
||
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.
Comment 60•9 years ago
|
||
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 61•9 years ago
|
||
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+
Assignee | ||
Comment 62•9 years ago
|
||
(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 63•9 years ago
|
||
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 64•9 years ago
|
||
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+
Assignee | ||
Comment 65•9 years ago
|
||
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.
Comment 66•9 years ago
|
||
(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 67•9 years ago
|
||
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 68•9 years ago
|
||
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 69•9 years ago
|
||
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 70•9 years ago
|
||
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 71•9 years ago
|
||
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 72•9 years ago
|
||
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 73•9 years ago
|
||
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 74•9 years ago
|
||
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 75•9 years ago
|
||
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 76•9 years ago
|
||
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 77•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8741631 -
Flags: review?(jfkthame) → review+
Comment 78•9 years ago
|
||
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!
Assignee | ||
Comment 79•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cam)
Assignee | ||
Comment 80•9 years ago
|
||
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)
Assignee | ||
Comment 81•9 years ago
|
||
(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)
Assignee | ||
Comment 82•9 years ago
|
||
try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6e94ead3b9f
Assignee | ||
Comment 83•9 years ago
|
||
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)
Assignee | ||
Comment 84•9 years ago
|
||
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/
Assignee | ||
Comment 85•9 years ago
|
||
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/
Assignee | ||
Comment 86•9 years ago
|
||
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/
Assignee | ||
Comment 87•9 years ago
|
||
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/
Assignee | ||
Comment 88•9 years ago
|
||
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/
Assignee | ||
Comment 89•9 years ago
|
||
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/
Assignee | ||
Comment 90•9 years ago
|
||
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/
Assignee | ||
Comment 91•9 years ago
|
||
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/
Assignee | ||
Comment 92•9 years ago
|
||
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/
Assignee | ||
Comment 93•9 years ago
|
||
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/
Assignee | ||
Comment 94•9 years ago
|
||
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/
Assignee | ||
Comment 95•9 years ago
|
||
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/
Assignee | ||
Comment 96•9 years ago
|
||
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/
Assignee | ||
Comment 97•9 years ago
|
||
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/
Assignee | ||
Comment 98•9 years ago
|
||
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/
Updated•9 years ago
|
Attachment #8736645 -
Flags: review?(jfkthame) → review+
Comment 99•9 years ago
|
||
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 100•9 years ago
|
||
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 101•9 years ago
|
||
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+
Comment 102•9 years ago
|
||
Hmm, looks like something is busted in nsCaseTransformTextRunFactory, according to try. But I'm sure you're looking into that.
Assignee | ||
Comment 103•9 years ago
|
||
(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.
Comment 104•9 years ago
|
||
Cool, I didn't think you'd overlook it! :) I guess I followed an older try link from mozreview, which looked a bit alarming.
Updated•9 years ago
|
Attachment #8743136 -
Flags: review?(cam)
Comment 105•9 years ago
|
||
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 106•9 years ago
|
||
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+
Assignee | ||
Comment 107•9 years ago
|
||
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)
Assignee | ||
Comment 108•9 years ago
|
||
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/
Assignee | ||
Comment 109•9 years ago
|
||
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/
Assignee | ||
Comment 110•9 years ago
|
||
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/
Assignee | ||
Comment 111•9 years ago
|
||
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/
Assignee | ||
Comment 112•9 years ago
|
||
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/
Assignee | ||
Comment 113•9 years ago
|
||
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/
Assignee | ||
Comment 114•9 years ago
|
||
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/
Assignee | ||
Comment 115•9 years ago
|
||
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/
Assignee | ||
Comment 116•9 years ago
|
||
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/
Assignee | ||
Comment 117•9 years ago
|
||
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/
Assignee | ||
Comment 118•9 years ago
|
||
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/
Assignee | ||
Comment 119•9 years ago
|
||
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/
Assignee | ||
Comment 120•9 years ago
|
||
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/
Assignee | ||
Comment 121•9 years ago
|
||
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/
Assignee | ||
Comment 122•9 years ago
|
||
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 123•9 years ago
|
||
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+
Assignee | ||
Comment 124•9 years ago
|
||
(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)
Assignee | ||
Comment 125•9 years ago
|
||
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
Assignee | ||
Comment 126•9 years ago
|
||
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/
Assignee | ||
Comment 127•9 years ago
|
||
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/
Assignee | ||
Comment 128•9 years ago
|
||
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/
Assignee | ||
Comment 129•9 years ago
|
||
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/
Assignee | ||
Comment 130•9 years ago
|
||
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/
Assignee | ||
Comment 131•9 years ago
|
||
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/
Assignee | ||
Comment 132•9 years ago
|
||
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/
Assignee | ||
Comment 133•9 years ago
|
||
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/
Assignee | ||
Comment 134•9 years ago
|
||
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/
Assignee | ||
Comment 135•9 years ago
|
||
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/
Assignee | ||
Comment 136•9 years ago
|
||
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/
Assignee | ||
Comment 137•9 years ago
|
||
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/
Assignee | ||
Comment 138•9 years ago
|
||
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/
Assignee | ||
Comment 139•9 years ago
|
||
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/
Assignee | ||
Comment 140•9 years ago
|
||
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/
Comment 141•9 years ago
|
||
(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)
Assignee | ||
Comment 142•9 years ago
|
||
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
Assignee | ||
Comment 143•9 years ago
|
||
Filed bug 1266645 for that issue. Cancel the ni? here.
Flags: needinfo?(jfkthame)
Comment 144•9 years ago
|
||
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.
Assignee | ||
Comment 145•9 years ago
|
||
(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.
Comment 146•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5816f7da8d5b
https://hg.mozilla.org/mozilla-central/rev/7c5bf61c4ab9
https://hg.mozilla.org/mozilla-central/rev/de6c6c719ef5
https://hg.mozilla.org/mozilla-central/rev/400f5b753ae0
https://hg.mozilla.org/mozilla-central/rev/6373a28c2d74
https://hg.mozilla.org/mozilla-central/rev/c582f2934be9
https://hg.mozilla.org/mozilla-central/rev/555af8b37aac
https://hg.mozilla.org/mozilla-central/rev/02e33a3ddffd
https://hg.mozilla.org/mozilla-central/rev/1d4eea6eb300
https://hg.mozilla.org/mozilla-central/rev/de351d24e7d7
https://hg.mozilla.org/mozilla-central/rev/f851031ac21e
https://hg.mozilla.org/mozilla-central/rev/d238e12a7f71
https://hg.mozilla.org/mozilla-central/rev/45acf5ee3c42
https://hg.mozilla.org/mozilla-central/rev/660f1684ab37
https://hg.mozilla.org/mozilla-central/rev/23604d09e7a0
https://hg.mozilla.org/mozilla-central/rev/b797895bca88
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 147•9 years ago
|
||
(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
Comment 149•9 years ago
|
||
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)
Comment 150•9 years ago
|
||
(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)
Comment 151•9 years ago
|
||
(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
Keywords: dev-doc-needed → dev-doc-complete
Comment 152•9 years ago
|
||
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)
Assignee | ||
Comment 153•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d85ea3a4665cef82d97443eaa751e93ae4fa8d
Bug 1097499 followup - Fix metadata of tests submitted to w3c. DONTBUILD
Comment 154•9 years ago
|
||
Comment 156•9 years ago
|
||
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)
Assignee | ||
Comment 157•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/66b4024afe1a4753aa3dc4a311eb8728f94b33b7
Bug 1097499 followup 2 - Fix metadata of tests submitted to w3c. DONTBUILD
Comment 158•9 years ago
|
||
Comment 160•9 years ago
|
||
Comment 161•9 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•