1040668 - Implement CSS3 text emphasis marks properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Sebastian Zartner [:sebo]]

The CSS Text Decoration Module Level 3 defines some properties for emphasis marks in East Asian documents.

These include:
- text-emphasis-style
- text-emphasis-color
- text-emphasis
- text-emphasis-position

This bug is for implementing these properties.

Sebastian

Comment 1

[John Daggett (:jtd)]

For rendering emphasis marks, Adobe has published an open source font containing all of the necessary marks:

https://github.com/adobe-fonts/kenten-generic

I think it would make sense to simply include this with Firefox and load it as needed.

Comment 2

[Jonathan Kew [:jfkthame]]

ISTM that we should prefer using glyphs from the same font as the text, where possible. In particular, the "sesame" glyphs U+FE45 '﹅' and U+FE46 '﹆' should have the same stroke style as the font being used.

I realize some fonts may not support the relevant characters, or may have bad glyphs, so some kind of check/fallback may be useful. Perhaps we'll want to include kenten-generic for those situations -- though I'd be a bit surprised if there wasn't a suitable fallback already present on major OS's.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

I'm going to work on this.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Emphasis font test

I made a simple page to test the emphasis marks of Japanese and Chinese in different fonts, alternative with "Kenten Generic" font.

It seems to me that Chinese's dot is generally fine on both OS X and Windows for major fonts. OS X's Japanese fonts are fine as well. However, it seems that the Japanese's sesame does not look very good on most fonts on Windows.

Do we want to do something to handle that?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Emphasis font test

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Hmm, the Chinese dot looks much better with Kenten Generic than any builtin font on Windows and OS X.

Comment 7

[Jonathan Kew [:jfkthame]]

I'm still inclined to think we should default to using whatever font is specified by the page's style, as per comment 2.

I agree that some of the major fonts -- in particular some of MS's Japanese fonts -- may not look good, but that's a design issue that should be addressed by the font vendors (or the OS distributors who choose what to bundle as part of their product).

Maybe we could propose a text-emphasis-font property that would allow authors to easily customize the font used for emphasis marks, separately from the font of the text?

Or just recommend that authors write things like

  font-family: "Kenten Generic", "Meiryo", sans-serif;

so that Kenten will be preferred for the characters it supports, and then the authors normal Japanese font for the rest of the text.

I think I'd be against the idea of bundling Kenten Generic in Firefox (at this point; maybe I can be convinced otherwise); if authors want to use it, they should specify it with @font-face.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Well, yes, I agree it is more a design issue. Also it seems all other browsers do not do anything special either. So ok, I'm fine with just using the font of the text.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - parse and compute text-emphasis properties

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - add helper function gfxTextRun::GetAdvanceForGlyph

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - add helper function InverseIfRTL in gfxFont.cpp

The helper functions added in patch 2 and patch 3 are used in patch 11.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 4 - add helper function for ensuring complex glyph

This helper function will be used in patch 9.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 5 - avoid necessary allocation inside the helper function

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 6 - add specifier for compiler optimization

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 7 - add method to unified preconditions of SetSimpleGlyph

This patch, along with patch 8, are mainly for avoiding losing glyph flags during text shaping, because that will happen in patch 9.

But there is an alternative method to implement the function in patch 9, which could remove the necessity of these two commits. I'm not sure whether that's a better way.

See the commit message of patch 9 for more details.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 8 - add simplified CompressedGlyph::SetComplex overload

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 9 - add glyph flag for characters should not have emphasis mark

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 10 - compute overflow from text-shadow after text decorations

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 11 - implement emphasis mark rendering

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 12 - move leading adjusting code to a separate function

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 13 - adjust line leadings for emphasis marks like ruby

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 14 - add text-emphasis related styles to ua sheet

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 15 - reftests

Sorry for these many small tests. I tried to follow the requirement of CSSWG so that we can submit these tests there directly.

Over half of the tests here are generated by two scripts, which have been marked out in reftest.list. You may want to review the scripts instead of the generated tests.

The script the script for text-emphasis-style generates both test pages and reference pages, while the script for text-emphasis-position only generates test pages.

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

I do not use MozReview because that produces too many messages when updating patches if the series is long.

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8687887 [details] [diff] [review]
patch 12 - move leading adjusting code to a separate function

Review of attachment 8687887 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 12; seems like just a straight refactoring (with one variable, 'leading', being replaced with an expression that has the same value). Looks good.

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8687889 [details] [diff] [review]
patch 13 - adjust line leadings for emphasis marks like ruby

Review of attachment 8687889 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with nits below:

::: layout/generic/nsLineLayout.cpp
@@ +1696,5 @@
> +    requiredEndLeading += endLeading;
> +  }
> +  if (aStyleText->HasTextEmphasis()) {
> +    LogicalSide side = aStyleText->TextEmphasisSide(mRootSpan->mWritingMode);
> +    nscoord halfHeight = aFontMetrics->MaxHeight() / 2;

Please add a comment here hinting at why we're using |halfHeight| in particular.  (I initially assumed that maybe we add half on top and half on bottom, or something like that -- but that's not the case.)

(As you say in IRC, this is per spec text about emphasis marks being "scaled down to 50%" -- please hint at that here.)

@@ +1700,5 @@
> +    nscoord halfHeight = aFontMetrics->MaxHeight() / 2;
> +    if (side == eLogicalSideBStart) {
> +      requiredStartLeading += halfHeight;
> +    } else {
> +      MOZ_ASSERT(side == eLogicalSideBEnd);

Consider adding a short message to the assertion statement here -- e.g:
      MOZ_ASSERT(side == eLogicalSideBEnd, "emphasis must be in block axis");

IMO, assertions with no text should be rare.

@@ +1709,5 @@
> +  nscoord leading = psd->mBStartLeading + psd->mBEndLeading;
> +  nscoord requiredLeading = requiredStartLeading + requiredEndLeading;
> +  nscoord deltaLeading = requiredLeading - leading;
> +  if (deltaLeading > 0) {
> +    // If the total leading is not wide enough for ruby annotations,

The "for ruby annotations" comment here is stale.  Needs to be updated to say "ruby annotations and/or text-emphasis marks", I think.

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 8687871 [details] [diff] [review]
patch 2 - add helper function gfxTextRun::GetAdvanceForGlyph

Review of attachment 8687871 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxTextRun.h
@@ +618,5 @@
>      void SetShapingState(ShapingState aShapingState) {
>          mShapingState = aShapingState;
>      }
>  
> +    int32_t GetAdvanceForGlyph(uint32_t aIndex)

This seems like a method that could usefully be defined on the abstract gfxShapedText class, in case we ever want to use it for gfxShapedWord as well as gfxTextRun.

That would require a virtual call to GetCharacterGlyphs(), though, so I guess we can leave it here for now, and reconsider if/when we see a need for it.

@@ +631,5 @@
> +        }
> +        const DetailedGlyph* details = GetDetailedGlyphs(aIndex);
> +        if (!details) {
> +            return 0;
> +        }

I don't think it's possible for GetDetailedGlyphs to return null, is it? The allocation of detailed-glyph records is infallible. So the null-check here is redundant (but include an assertion if you like).

(I see there was a null-check in the old code you're replacing here, but I think it was redundant there too...)

Comment 28

[Jonathan Kew [:jfkthame]]

Comment on attachment 8687876 [details] [diff] [review]
patch 5 - avoid necessary allocation inside the helper function

Review of attachment 8687876 [details] [diff] [review]:
-----------------------------------------------------------------

s/necessary/unnecessary/ in the commit message here :)

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 8687877 [details] [diff] [review]
patch 6 - add specifier for compiler optimization

Review of attachment 8687877 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxTextRun.h
@@ +475,5 @@
>      void ResetGlyphRuns() { mGlyphRuns.Clear(); }
>      void SortGlyphRuns();
>      void SanitizeGlyphRuns();
>  
> +    virtual CompressedGlyph* GetCharacterGlyphs() override final {

I think our current guidelines say that you don't need 'virtual' here, because 'override' implies it.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8687870 [details] [diff] [review]
patch 1 - parse and compute text-emphasis properties

Declaration.cpp:

Technically this isn't quite right, since you should produce a minimal
result, and this doesn't do so for a value like "green".  You should
probably check both properties against their defaults, and serialize
text-emphasis-style only if style is non-default *or* both are default
(since "none" is the most sensible serialization of "none currentColor").

nsCSSParser.cpp:

It seems like ParseTextEmphasisPosition() might as well be called
from ParseSingleValuePropertyByFunction (and thus use
CSS_PROPERTY_PARSE_VALUE | CSS_PROPERTY_VALUE_PARSER_FUNCTION instead
of CSS_PROPERTY_PARSE_FUNCTION).  It's slightly less code that
way (no AppendValue call

>+  const int32_t numProps = ArrayLength(kTextEmphasisIDs);
>+  nsCSSValue values[numProps];

I hope this doesn't invoke variable-length array code underneath.
Maybe clearer to make numProps static too, and use the macro
MOZ_ARRAY_LENGTH?

>+  if (ParseVariant(value, VARIANT_INHERIT, nullptr) != CSSParseResult::Ok) {

You should use ParseSingleTokenVariant here, which returns boolean.

>+    nsCSSValue left, right;

This is confusing given that left and right are values of the property.
Call them v1 and v2 or something like that.


Please fix ParseTextEmphasisStyle not to use 'left' and 'right' variables.

>+  }
>+
>+  if (left.GetUnit() != eCSSUnit_Enumerated) {
>+    return false;
>+  }

This could also just be an "else { return false " rather than a
separate if.

nsCSSPropList.h:

>+        CSS_PROPERTY_HASHLESS_COLOR_QUIRK |

New properties should not have the hashless color quirk.

> CSSValue*
>+nsComputedDOMStyle::DoGetTextEmphasis()
>+{
>+  nsDOMCSSValueList* valueList = GetROCSSValueList(false);
>+  valueList->AppendCSSValue(DoGetTextEmphasisStyle());
>+  if (!StyleText()->mTextEmphasisColorForeground) {
>+    valueList->AppendCSSValue(DoGetTextEmphasisColor());
>+  }
>+  return valueList;
>+}

You should probably remove this.  The norm is not to implement
computed style for new shorthand properties.

(But if you do want it for some reason, you should have the line
in nsComputedDOMStylePropertyList.h uncommented.  But if you do,
I'd like to understand why.)


I guess nsComputedDOMStyle::DoGetTextEmphasisPosition() is OK,
although there are similar properties where we've just used
AppendBitmaskCSSValue and SetString.

nsRuleNode.cpp:

>+static const TextEmphasisChars kTextEmphasisChars[] =

Please add static_asserts for the values of all the constants here.

>+  if (textEmphasisColorValue->GetUnit() == eCSSUnit_Null) {
>+    // We don't want to change anything in this case.

This is a bit odd.

Instead, you can drop this and change the final else to be:
  } else if (SetColor(...)) {
    text->mTextEmphasisColorForeground = false;
  }

Note that it's a little odd that you're implementing the new
definition of currentColor for one property when we haven't switched
any other properties, but that's ok.  (We should probably fix the
others, though!)

The text-emphasis-color computation should also treat
unset as inherit, not as initial.

Also, in the text-emphasis-color computation handling of currentColor,
could you set mTextEmphasisColor to mPresContext->DefaultColor() so
that you establish the invariant that if mTextEmphasisColorForeground
is true, then the color is always default color, and you don't need to
worry about dealing with fancy difference calculation in
CalcDifference or elsewhere.

In the text-emphasis-style computation, you should add an
assertion to the enumerated case that the resulting style is
nonzero.

Please add a comment to TruncateStringToSingleGrapheme that it helps
memory use that it not mutate the string in the common case since
then we share the buffer from the specified style into the computed
style.

nsStyleConsts.h:

>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_NONE           0
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_FILL_MASK      (1 << 3)
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_FILLED         (0 << 3)
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_OPEN           (1 << 3)
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_SHAPE_MASK     7
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_DOT            1
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_CIRCLE         2
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_DOUBLE_CIRCLE  3
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_TRIANGLE       4
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_SESAME         5
>+#define NS_STYLE_TEXT_EMPHASIS_STYLE_STRING         255

This seems to use the value '0' for both 'filled' and 'none'.  You
should explain with clear comments how it actually works; that in
specified style none is represented as eCSSUnit_None, and in computed
style filled has its shape computed, and the string is empty vs.
non-empty.

nsStyleContext.cpp:

You need to make the addition to CalcStyleDifference!

nsStyleStruct.cpp:

In nsStyleText::CalcDifference, you should make up your
mind about what hints you want.  You have two different returns
for the non-color hints!

nsStyleStruct.h:

>+  nscolor mTextEmphasisColor;           // [inherited]

Please add this after mTabSize so it's next to other 32-bit things.

property_database.js:

in the invalid_values line for text-emphasis-color and for text-emphasis,
please add an otherwise valid rgb() value with garbage instead of one
of the numbers.

In the other_values line for text-emphasis, please add "green none" and
"currentColor filled" and "currentColor open"

r=dbaron with that

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8687885 [details] [diff] [review]
patch 10 - compute overflow from text-shadow after text decorations

Seems like this fixes an existing bug, and could use a reftest.  (I would think that without this patch, a dynamic change that requires repaint (say, to color) wouldn't repaint enough area if the text has both a distant decoration (which can be done with the decoration on an ancestor and vertical-align on the child) and a text-shadow.

e.g., I see failure to paint the aqua underline with:
<!DOCTYPE html>
<style>
#one { text-decoration: underline }
#two { vertical-align: 1em; text-shadow: 0 10px aqua,10px 0 fuchsia }
</style>
<span id="one"><span id="two">x</span></span>

although it would probably be good to test with a dynamic change to the text-shadow's color on MozReftestInvalidate as well.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8687890 [details] [diff] [review]
patch 14 - add text-emphasis related styles to ua sheet

Neither of these selectors trigger any RuleHash optimizations, so they'll be explicitly tested against every single element.  That seems likely to be a measurable performance regression.  Could you get performance numbers on this change?

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8687892 [details] [diff] [review]
patch 15 - reftests

Could you put the generator in a support/ directory instead of a script/ directory?  (Fix the reftest.list too.)

Could you fix all the "No newline at end of file" (and the generator, if needed)?

And could you add a pair of link rel="author" lines for both Mozilla and yourself (to both tests and references)?


I didn't look at the tests in detail; let me know if you want me to.

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #30)
> Comment on attachment 8687870 [details] [diff] [review]
> patch 1 - parse and compute text-emphasis properties
> 
> Declaration.cpp:
> 
> Technically this isn't quite right, since you should produce a minimal
> result, and this doesn't do so for a value like "green".  You should
> probably check both properties against their defaults, and serialize
> text-emphasis-style only if style is non-default *or* both are default
> (since "none" is the most sensible serialization of "none currentColor").

Actually I don't quite agree with the current syntax. I don't think it makes much sense to allow 'text-emphasis' to omit the style part. Specifying a single color value still produces nothing. I raised it in www-style but do not get any reply: https://lists.w3.org/Archives/Public/www-style/2015Oct/0201.html

But before the spec actually gets changed, ok, I'll do what you want here.

Comment 35

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #29)
> Comment on attachment 8687877 [details] [diff] [review]
> patch 6 - add specifier for compiler optimization
> 
> Review of attachment 8687877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxTextRun.h
> @@ +475,5 @@
> >      void ResetGlyphRuns() { mGlyphRuns.Clear(); }
> >      void SortGlyphRuns();
> >      void SanitizeGlyphRuns();
> >  
> > +    virtual CompressedGlyph* GetCharacterGlyphs() override final {
> 
> I think our current guidelines say that you don't need 'virtual' here,
> because 'override' implies it.

Oh, I didn't know that. I did see some discussion in dev-platform, but I didn't see consensus there. Thanks for reminding.

Comment 36

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #30)
> Comment on attachment 8687870 [details] [diff] [review]
> patch 1 - parse and compute text-emphasis properties
> 
> >+  if (textEmphasisColorValue->GetUnit() == eCSSUnit_Null) {
> >+    // We don't want to change anything in this case.
> 
> This is a bit odd.
> 
> Instead, you can drop this and change the final else to be:
>   } else if (SetColor(...)) {
>     text->mTextEmphasisColorForeground = false;
>   }

One reason I did that is, it seems to me eCSSUnit_Null is the most common case, so moving it to the very beginning could eliminate all the checks when possible. Does it make sense?

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #36)
> One reason I did that is, it seems to me eCSSUnit_Null is the most common
> case, so moving it to the very beginning could eliminate all the checks when
> possible. Does it make sense?

Yeah, I guess that makes sense.  Maybe we should do it more widely in a separate bug.

Comment 38

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #30)
> Comment on attachment 8687870 [details] [diff] [review]
> patch 1 - parse and compute text-emphasis properties
> 
> nsRuleNode.cpp:
> 
> In the text-emphasis-style computation, you should add an
> assertion to the enumerated case that the resulting style is
> nonzero.

There is already an assertion that shape (which is style & NS_STYLE_TEXT_EMPHASIS_STYLE_SHAPE_MASK) is greater than 0. I don't think it is necessary to add another.

> Please add a comment to TruncateStringToSingleGrapheme that it helps
> memory use that it not mutate the string in the common case since
> then we share the buffer from the specified style into the computed
> style.

I'm not sure I understand what you wanted to say here. Do you mean, we call Truncate inside "if (!iter.AtEnd())" because we want to avoid mutating the string for common case? (which wasn't the reason I did that. I though if AtEnd() is true, iter itself may not contain a valid value, so we should always add a guard for it.)

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #38)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #30)
> > Comment on attachment 8687870 [details] [diff] [review]
> 
> There is already an assertion that shape (which is style &
> NS_STYLE_TEXT_EMPHASIS_STYLE_SHAPE_MASK) is greater than 0. I don't think it
> is necessary to add another.

Indeed; that's good enough.

> > Please add a comment to TruncateStringToSingleGrapheme that it helps
> > memory use that it not mutate the string in the common case since
> > then we share the buffer from the specified style into the computed
> > style.
> 
> I'm not sure I understand what you wanted to say here. Do you mean, we call
> Truncate inside "if (!iter.AtEnd())" because we want to avoid mutating the
> string for common case? (which wasn't the reason I did that. I though if
> AtEnd() is true, iter itself may not contain a valid value, so we should
> always add a guard for it.)

Pretty much.

It's just that it's worth a comment that avoiding mutation of the string in as many cases is possible is good, because then we share the same string buffer between specified and computed style the most.

Comment 40

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 v2 - parse and compute text-emphasis properties

Comment 41

[Xidorn Quan [:xidorn] UTC+11]

Sorry but it seems the interdiff is not quite useful here because there was some other changes touch those files. But I just addressed the review comments, and should not have touched anything else.

Comment 42

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8689935 [details] [diff] [review]
patch 1 v2 - parse and compute text-emphasis properties

Review of attachment 8689935 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsRuleNode.cpp
@@ +4269,5 @@
> +  unicode::ClusterIterator iter(aStr.Data(), aStr.Length());
> +  if (!iter.AtEnd()) {
> +    iter.Next();
> +    if (!iter.AtEnd()) {
> +      // Not mutating the string the string for common cases helps

The duplicate "the string" here has been fixed locally.

Comment 43

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 10 v2 - compute overflow from text-shadow after text decorations

Add a reftest you may want to review.

I did want to file an independent bug for this issue, but I didn't know how to trigger this bug, and thought this might not happen in general, so I just included this fix as a trivial patch here.

Comment 44

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd1662de6ba

Comment 45

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b979f9cdd3f

Comment 46

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #30)
> Comment on attachment 8687870 [details] [diff] [review]
> patch 1 - parse and compute text-emphasis properties
> 
> >+  const int32_t numProps = ArrayLength(kTextEmphasisIDs);
> >+  nsCSSValue values[numProps];
> 
> I hope this doesn't invoke variable-length array code underneath.
> Maybe clearer to make numProps static too, and use the macro
> MOZ_ARRAY_LENGTH?

In theory, ArrayLength should be fine, but it is not fine with VS2013 because that compiler doesn't support constexpr and thus reports a compile error.

Comment 47

[Jonathan Kew [:jfkthame]]

Comment on attachment 8687882 [details] [diff] [review]
patch 9 - add glyph flag for characters should not have emphasis mark

Review of attachment 8687882 [details] [diff] [review]:
-----------------------------------------------------------------

It seems a pity to do extra work in SetupClusterBoundaries for all text, given that emphasis marks will actually need to be drawn on only a minuscule proportion of the text we handle. Could we check (in BuildTextRunsScanner::BuildTextRunForFrames, I guess, where we already check for various other styles that require special handling) whether there is any text-emphasis usage, and skip the setting of this flag completely in the case where it's absent?

Comment 48

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #47)
> Comment on attachment 8687882 [details] [diff] [review]
> patch 9 - add glyph flag for characters should not have emphasis mark
> 
> Review of attachment 8687882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems a pity to do extra work in SetupClusterBoundaries for all text,
> given that emphasis marks will actually need to be drawn on only a minuscule
> proportion of the text we handle. Could we check (in
> BuildTextRunsScanner::BuildTextRunForFrames, I guess, where we already check
> for various other styles that require special handling) whether there is any
> text-emphasis usage, and skip the setting of this flag completely in the
> case where it's absent?

It looks like a good point. I'll try it tomorrow or next week. In the mean time, you can probably continue to review the next patch, which relies on the corresponding flag is set properly, but otherwise independent to this patch.

Comment 49

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #48)
> (In reply to Jonathan Kew (:jfkthame) from comment #47)
> > Comment on attachment 8687882 [details] [diff] [review]
> > patch 9 - add glyph flag for characters should not have emphasis mark
> > 
> > Review of attachment 8687882 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It seems a pity to do extra work in SetupClusterBoundaries for all text,
> > given that emphasis marks will actually need to be drawn on only a minuscule
> > proportion of the text we handle. Could we check (in
> > BuildTextRunsScanner::BuildTextRunForFrames, I guess, where we already check
> > for various other styles that require special handling) whether there is any
> > text-emphasis usage, and skip the setting of this flag completely in the
> > case where it's absent?
> 
> It looks like a good point. I'll try it tomorrow or next week. In the mean
> time, you can probably continue to review the next patch, which relies on
> the corresponding flag is set properly, but otherwise independent to this
> patch.

I mean, patch 11, not patch 10 :)

Comment 50

[Jonathan Kew [:jfkthame]]

Comment on attachment 8687881 [details] [diff] [review]
patch 8 - add simplified CompressedGlyph::SetComplex overload

Review of attachment 8687881 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxCoreTextShaper.cpp
@@ +525,2 @@
>              aShapedText->SetGlyphs(aOffset + baseCharIndex, textRunGlyph,
>                                     detailedGlyphs.Elements());

This is nice, but ISTM we should be able to do better. In particular, note that aShapedText->SetGlyphs will do a virtual function call to GetCharacterGlyphs(), and we're doing this on every character. (Similarly in the other shaper interfaces.)

Yet the caller here already has the charGlyphs pointer, so it's done that virtual call (once, for the run) already. So how about creating a SetGlyphs method that takes it as a parameter?

And as for the CompressedGlyph itself: we're copying that from charGlyphs, tweaking it, and then copying it back. Surely we could just modify it in-place?

Actually, if we do that, we don't need to pass it to SetGlyphs at all, nor do we need the charGlyphs array there. So we could end up with:

  uint32_t glyphCount = detailedGlyphs.Length();
  charGlyphs[baseCharIndex].SetComplex(glyphCount);
  aShapedText->StoreComplexGlyphs(aOffset + baseCharIndex, glyphCount,
                                  detailedGlyphs.Elements());

where StoreComplexGlyphs is defined inline in gfxShapedText:

  void StoreComplexGlyphs(uint32_t aIndex, uint32_t aCount,
                          const DetailedGlyph* aDetailedGlyphs)
  {
    if (aCount > 0) {
        DetailedGlyph *details = AllocateDetailedGlyphs(aIndex, aCount);
        memcpy(details, aDetailedGlyphs, sizeof(DetailedGlyph) * aCount);
    }
  }

(All untested, so I may have missed something... but AFAICS we could do something like this in each of the shaper backends. Maybe we can get rid of the current SetGlyphs altogether, with its "hidden" virtual call that we shouldn't be doing in inner-loop situations.)

Comment 51

[Xidorn Quan [:xidorn] UTC+11]

Well, actually with your newly proposed solution for what I wanted to do with patch 9, we no longer need patch 7 and 8. If you think we should have some fix there, we could probably have a new bug for that.

Comment 52

[Jonathan Kew [:jfkthame]]

Comment on attachment 8687886 [details] [diff] [review]
patch 11 - implement emphasis mark rendering

Review of attachment 8687886 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +5103,5 @@
> +{
> +  const nsString& emphasisString = aStyleText->mTextEmphasisStyleString;
> +  RefPtr<gfxContext> ctx = CreateReferenceThebesContext(aFrame);
> +  auto appUnitsPerDevUnit = aFrame->PresContext()->AppUnitsPerDevPixel();
> +  // The emphasis marks should always be rendered upright per spec.

I wonder if this is correct for the (recently-added) writing-mode:sideways-* modes, or for text-orientation:sideways?

The CSS Text Decorations draft doesn't seem to consider "sideways orientation" at all. It says

"Marks must remain upright in vertical writing modes: like CJK characters, they do not rotate to match the writing mode."

which covers the common case for vertical ideographic text, but fails to recognize that with text-orientation:sideways or writing-mode:sideways-*, even the CJK characters *do* rotate.

@@ +5106,5 @@
> +  auto appUnitsPerDevUnit = aFrame->PresContext()->AppUnitsPerDevPixel();
> +  // The emphasis marks should always be rendered upright per spec.
> +  uint32_t flags = aWM.IsVertical() ?
> +    gfxTextRunFactory::TEXT_ORIENT_VERTICAL_UPRIGHT :
> +    gfxTextRunFactory::TEXT_ORIENT_HORIZONTAL;

And therefore, I suspect you should be testing aWM.IsVertical() && !aWM.IsSideways() here. Maybe worth asking the WG for confirmation?

Comment 53

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #51)
> Well, actually with your newly proposed solution for what I wanted to do
> with patch 9, we no longer need patch 7 and 8. If you think we should have
> some fix there, we could probably have a new bug for that.

Yeah, sounds good; I think there's a small but worthwhile optimization we can have here. I'll be happy to r+ such a patch, if you can confirm it actually works. :)

Comment 54

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #52)
> Comment on attachment 8687886 [details] [diff] [review]
> patch 11 - implement emphasis mark rendering
> 
> And therefore, I suspect you should be testing aWM.IsVertical() &&
> !aWM.IsSideways() here. Maybe worth asking the WG for confirmation?

Sent here https://lists.w3.org/Archives/Public/www-style/2015Nov/0285.html

Comment 55

[Xidorn Quan [:xidorn] UTC+11]

Koji said he is not aware of any use cases for sideways text with emphasis marks, so he thinks it is fine to remain this undefined. [1]

As my current impl renders the emphasis marks as a whole after rendering the text (so that we do not add any cost to the normal path if there is no emphasis marks), handling sideways would need additional code to handle the canvas rotation, which adds complexity. So I think it's probably fine to just make it always upright for now, but remove the tests for this case from the reftests in patch 15. If the spec later does make it defined in a different way, we can change our impl to match that later.

Sounds good?


[1] https://lists.w3.org/Archives/Public/www-style/2015Nov/0291.html

Comment 56

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8689935 [details] [diff] [review]
patch 1 v2 - parse and compute text-emphasis properties

Review of attachment 8689935 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSPropList.h
@@ +3285,5 @@
> +    text-emphasis-position,
> +    text_emphasis_position,
> +    TextEmphasisPosition,
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_PARSE_FUNCTION,

This should be CSS_PROPERTY_VALUE_PARSER_FUNCTION instead, which has been fixed locally.

Comment 57

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 7 (new) - add NO_EMPHASIS_MARK flag

This is the first half of the old patch 9.

Comment 58

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 8 (new) - setup the emphasis mark flag

This implements the new approach from comment 47, which replaces the old patch 9.

Comment 59

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d205c3924678

Comment 60

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 13 v2 - adjust line leadings for emphasis marks like ruby

There was several reftest failures since this patch because of incorrectly filling negative leadings. THe new patch fixes this issue.

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8689935 [details] [diff] [review]
patch 1 v2 - parse and compute text-emphasis properties

Doesn't seem like this really needed another round of review.  (The only thing that wasn't completely straightforward based on the review comments and following discussion was the TEXT_EMPHASIS_CHARS_LIST() macro.)

Comment 62

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 14.1 - move GetLanguage() method to nsPresContext

Comment 63

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 14.2 - add helper function for simple language matching

Comment 64

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 14.3 - make text-emphasis-position language-aware

Comment 65

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #55)
> Koji said he is not aware of any use cases for sideways text with emphasis
> marks, so he thinks it is fine to remain this undefined. [1]

Even if this is left undefined in the spec, because of lack of known use cases, I think I'd really prefer to handle sideways-* the other way.

IMO, when using a "sideways" mode that is intended -- for all scripts, regardless of their "natural" way of writing -- to rotate the horizontal rendering, rather than to render in an upright-vertical mode, it seems clear that the most reasonable/expected result is that the emphasis marks, just like the base letters and any associated ruby, will also be rotated.

Basically, using 'writing-mode:sideways-rl' on a run of text is expected to result in essentially the same rendering as 'transform:rotate(90deg)' (ignoring issues of the origin for rotation), with the main difference being that transform doesn't affect layout (only rendering), whereas using writing-mode to rotate the run will also swap its height and width for layout purposes.

> 
> As my current impl renders the emphasis marks as a whole after rendering the
> text (so that we do not add any cost to the normal path if there is no
> emphasis marks), handling sideways would need additional code to handle the
> canvas rotation, which adds complexity. So I think it's probably fine to
> just make it always upright for now, but remove the tests for this case from
> the reftests in patch 15. If the spec later does make it defined in a
> different way, we can change our impl to match that later.

As long as the rendering of the emphasis marks is confined to DrawTextRunAndDecorations, and not handled by the simplified DrawTextRun method, there shouldn't be any significant impact on the majority of text (which goes through DrawTextRun). The canvas rotation for the emphasis-marks run will be handled by gfxFont::Draw, provided the run and font parameters are set up appropriately (vertical run, horizontal font), so I don't think it should add much complexity overall.

Could you please try implementing it this way, as I think it provides a much more logical and consistent result? If it truly turns out to be problematic, I suppose we could reconsider, but I don't think it will be so bad. Thanks.

Comment 66

[Jonathan Kew [:jfkthame]]

Comment on attachment 8690627 [details] [diff] [review]
patch 8 (new) - setup the emphasis mark flag

Review of attachment 8690627 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +2528,5 @@
>    }
>  }
>  
> +static bool
> +MayCharacterHaveEmphasisMark(char16_t aCh)

Hmm. This isn't quite right, I think, because it won't detect supplementary-plane characters that should be excluded. (There aren't many, but there are at least a handful of GC=Control characters up there, so we really should do the right thing.)

So we need to change this to take a uint32_t, and check for surrogate pairs in the code that calls it.

@@ +2556,5 @@
> +}
> +
> +template<typename T>
> +static void
> +SetupTextEmphasisForTextRun(gfxTextRun* aTextRun, const T* aText)

Because of the issue noted above, it's probably simplest to scrap the template<> here and have two separate implementations, a trivial 8-bit one and a 16-bit one that includes the surrogate check.

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8691226 [details] [diff] [review]
patch 14.3 - make text-emphasis-position language-aware

So two issues here:

 (1) This uses GetLanguageFromCharset, which the :lang() selector doesn't.  We don't want to tie any new language-specific features to GetLanguageFromCharset() because it gives legacy encodings an advantage over UTF-8, and we'd really prefer that everybody switch to UTF-8 and properly language-tag their content.  We use GetLanguageFromCharset() for font choices for legacy reasons, but we don't want to add new stuff.  I guess this is an issue with patch 1 as well -- we probably don't want to move that code to nsPresContext, and we probably should add a comment pointing out that it shouldn't be reused instead.

 (2) This doesn't actually match what the spec says.  First, the spec defines one default for everything other than Chinese and then another for Chinese -- so your labels of _JA and _ZH are a bit confusing.  Second -- and you could consider this a bug in the spec -- the spec doesn't define switching back to the non-Chinese default when the language is something other than Chinese, in a descendant of an element that is in Chinese.  This means your behavior doesn't match the spec's -- although it's probably better than the spec's.  Maybe worth raising as a spec issue.

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8691224 [details] [diff] [review]
patch 14.1 - move GetLanguage() method to nsPresContext

see (1) in previous comment

Comment 69

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #67)
> Comment on attachment 8691226 [details] [diff] [review]
> patch 14.3 - make text-emphasis-position language-aware
> 
> Second -- and
> you could consider this a bug in the spec -- the spec doesn't define
> switching back to the non-Chinese default when the language is something
> other than Chinese, in a descendant of an element that is in Chinese.  This
> means your behavior doesn't match the spec's -- although it's probably
> better than the spec's.  Maybe worth raising as a spec issue.

Yes, I have raised that in the mailing list when I was implementing this [1] and fantasai has replied and thought it is reasonable [2].


[1] https://lists.w3.org/Archives/Public/www-style/2015Nov/0311.html
[2] https://lists.w3.org/Archives/Public/www-style/2015Nov/0326.html

Comment 70

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #66)
> Comment on attachment 8690627 [details] [diff] [review]
> patch 8 (new) - setup the emphasis mark flag
> 
> Review of attachment 8690627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsTextFrame.cpp
> @@ +2528,5 @@
> >    }
> >  }
> >  
> > +static bool
> > +MayCharacterHaveEmphasisMark(char16_t aCh)
> 
> Hmm. This isn't quite right, I think, because it won't detect
> supplementary-plane characters that should be excluded. (There aren't many,
> but there are at least a handful of GC=Control characters up there, so we
> really should do the right thing.)

Ah, okay. I wasn't aware that there are control characters in supplementary-plane, but a grep in UnicodeData.txt shows there are some Cf characters in those planes.

Comment 71

[Daniel Holbert [:dholbert]]

Comment on attachment 8690656 [details] [diff] [review]
patch 13 v2 - adjust line leadings for emphasis marks like ruby

Review of attachment 8690656 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with the following addressed as you see fit:

::: layout/generic/nsLineLayout.cpp
@@ +1710,5 @@
> +    }
> +  }
> +
> +  nscoord requiredLeading = requiredStartLeading + requiredEndLeading;
> +  if (requiredLeading > 0) {

This >0 check seems to be the main change in this updated version. Two things:

(1) Per IRC, please add a code-comment to explain this check -- in particular, we don't want to adjust anything if requiredLeading is 0 (even if that's greater than |leading|).

(2) Consider changing this to a "!= 0" check, to make it clearer what we're actually checking for here.  Per IRC, this looks like we're checking for negative values, but really you're not intending to, since requiredLeading isn't expected to ever be negative.  (unless we have an absurdly-large font & have integer overflow, I'm guessing -- and in that case, visiting this clause won't produce anything better than not-visiting this clause, I expect)

Comment 72

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 8 (new) v2 - setup the emphasis mark flag

Comment 73

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 11 v2 - implement emphasis mark rendering

Comment 74

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 14.1 v2 - move part of nsStyleFont::GetLanguage to nsPresContext

Comment 75

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 14.3 v2 - make text-emphasis-position language-aware

About the newly added check against "mn" language, I've raised an issue in www-style: https://lists.w3.org/Archives/Public/www-style/2015Nov/0330.html

If the spec ends up not adding this, I would remove that condition.

Comment 76

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 15 v2 - reftests

Three main differences compare to the previous version:

1. all reference pages with ruby have added rule "rt { font-variant-east-asian: inherit; }" to ensure that the emphasis marks in reference pages are not rendered with ruby-specific font variant. The spec has some issue on this, see [1] and [2]. This also causes reftest failures on OS X with the previous version.

2. testcases which ensure emphasis marks are upright even with sideways text have been removed as it is an undefined behavior in the spec currently, and we are going to implement it in the other way than before.

3. a new set of tests (and the script to generate them, generate-text-emphasis-style-property-010-tests.sh) is added for having a more thoroughly check on characters which should not have emphasis marks.


[1] https://lists.w3.org/Archives/Public/www-style/2015Nov/0295.html
[2] https://lists.w3.org/Archives/Public/www-style/2015Nov/0336.html

Comment 77

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #65)
> Could you please try implementing it this way, as I think it provides a much
> more logical and consistent result? If it truly turns out to be problematic,
> I suppose we could reconsider, but I don't think it will be so bad. Thanks.

You are right. It isn't so bad. Actually it is not hard at all, because I call a high level method on the text run to draw it, which handles everything for me, so I don't even need to rotate the canvas. That could have penalty on performance, though.

Comment 78

[Jonathan Kew [:jfkthame]]

Comment on attachment 8687876 [details] [diff] [review]
patch 5 - avoid necessary allocation inside the helper function

Review of attachment 8687876 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.h
@@ +1001,5 @@
>          MOZ_ASSERT(GetCharacterGlyphs() + aIndex == &aGlyph);
>          if (aGlyph.IsSimpleGlyph()) {
> +            DetailedGlyph details = {
> +                aGlyph.GetSimpleGlyph(),
> +                aGlyph.GetSimpleAdvance(),

Beware: this needs a cast to int32_t to avoid a compile warning/error:

 0:52.79 ../../dist/include/gfxFont.h:1018:17: error: non-constant-expression cannot be narrowed from type 'uint32_t' (aka 'unsigned int') to 'int32_t' (aka 'int') in initializer list [-Wc++11-narrowing]
 0:52.79                 aGlyph.GetSimpleAdvance(),
 0:52.79                 ^~~~~~~~~~~~~~~~~~~~~~~~~
 0:52.79 ../../dist/include/gfxFont.h:1018:17: note: insert an explicit cast to silence this issue
 0:52.79                 aGlyph.GetSimpleAdvance(),
 0:52.79                 ^~~~~~~~~~~~~~~~~~~~~~~~~
 0:52.79                 static_cast<int32_t>(    )

Comment 79

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #78)
> Comment on attachment 8687876 [details] [diff] [review]
> patch 5 - avoid necessary allocation inside the helper function
> 
> Review of attachment 8687876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFont.h
> @@ +1001,5 @@
> >          MOZ_ASSERT(GetCharacterGlyphs() + aIndex == &aGlyph);
> >          if (aGlyph.IsSimpleGlyph()) {
> > +            DetailedGlyph details = {
> > +                aGlyph.GetSimpleGlyph(),
> > +                aGlyph.GetSimpleAdvance(),
> 
> Beware: this needs a cast to int32_t to avoid a compile warning/error:

Yeah, thanks for reminding. This has been fixed locally. The current patchset here is not the latest in my local. I've fixed bunch of issues detected in the try build, and this is actually one of them.

(Not having the latest patchset is a major disadvantage of submitting patches via attachment. But I'm really afraid of posting ridiculous number of useless messages from MozReview...)

Comment 80

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #79)
> (In reply to Jonathan Kew (:jfkthame) from comment #78)
> > Comment on attachment 8687876 [details] [diff] [review]
> > patch 5 - avoid necessary allocation inside the helper function
> > 
> > Review of attachment 8687876 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/thebes/gfxFont.h
> > @@ +1001,5 @@
> > >          MOZ_ASSERT(GetCharacterGlyphs() + aIndex == &aGlyph);
> > >          if (aGlyph.IsSimpleGlyph()) {
> > > +            DetailedGlyph details = {
> > > +                aGlyph.GetSimpleGlyph(),
> > > +                aGlyph.GetSimpleAdvance(),
> > 
> > Beware: this needs a cast to int32_t to avoid a compile warning/error:
> 
> Yeah, thanks for reminding. This has been fixed locally. The current
> patchset here is not the latest in my local. I've fixed bunch of issues
> detected in the try build, and this is actually one of them.

OK, great.

> (Not having the latest patchset is a major disadvantage of submitting
> patches via attachment. But I'm really afraid of posting ridiculous number
> of useless messages from MozReview...)

Yeah, attachments aren't ideal, especially because of the tendency to be stale; but TBH I find MozReview really annoying, so I still prefer this.

Comment 81

[Jonathan Kew [:jfkthame]]

Comment on attachment 8691879 [details] [diff] [review]
patch 11 v2 - implement emphasis mark rendering

Review of attachment 8691879 [details] [diff] [review]:
-----------------------------------------------------------------

I tried a build with this, and it mostly looks great, but one issue did show up: in writing-mode:sideways-lr, the emphasis marks appear to be misplaced (such that they're largely or entirely clipped out of the rendering). I think there are a couple of issues, one with inline-direction and one with positioning (baseline).... see below.

::: gfx/thebes/gfxTextRun.cpp
@@ +679,5 @@
> +    params.context = aContext;
> +    params.mark = aMark;
> +    params.advance = aMarkAdvance;
> +    params.isVertical = IsVertical();
> +    params.isRTL = IsRightToLeft();

I suspect that rather than IsRightToLeft here, you want to be looking at IsInlineReversed. (And so change the field name to match.) This should handle the reversal of inline direction that happens in the sideways-lr case.

::: layout/generic/nsTextFrame.cpp
@@ +5196,5 @@
> +  } else {
> +    MOZ_ASSERT(side == eLogicalSideBEnd);
> +    info->baselineOffset = absOffset;
> +    overflowRect.BStart(aWM) = frameSize.BSize(aWM);
> +  }

I don't think this works quite right for sideways-lr mode, but haven't tried to work through exactly where it should be fixed.

Comment 82

[Xidorn Quan [:xidorn] UTC+11]

I'll investigate that tomorrow, and see what's happening there. I probably need to add tests for that as well.

(TBH, I haven't tested much on sideways-* writing modes which I probably should, sorry...)

Comment 83

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8691882 [details] [diff] [review]
patch 14.3 v2 - make text-emphasis-position language-aware

>Bug 1040668 part 15 - Make the default value of text-emphasis-position aware of the language.

Would be nice to know if this is 14.3 or 15. :-)

>-  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Font)) {
>+  const nsAttrValue* langValue = aAttributes->GetAttr(nsGkAtoms::lang);
>+  bool hasLangValue = langValue && langValue->Type() == nsAttrValue::eString;
>+
>+  if (hasLangValue && (aData->mSIDs & NS_STYLE_INHERIT_BIT(Font))) {

Could you put all this code inside an if() that tests just the two NS_STYLE_INHERIT_BIT()s?

>+      } else if (nsStyleUtil::MatchesLanguagePrefix(lang, MOZ_UTF16("ja")) ||
>+                 nsStyleUtil::MatchesLanguagePrefix(lang, MOZ_UTF16("mn"))) {

Please add comments here pointing to https://lists.w3.org/Archives/Public/www-style/2015Nov/0330.html (and maybe also its parent message).

Maybe also similar comments with the changes in nsStyleStruct.cpp?

r=dbaron with that

Comment 84

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8691883 [details] [diff] [review]
patch 15 v2 - reftests

Was it intentional that text-emphasis-style-property-{006,007}* don't exist anymore?

text-emphasis-style-property-010Cn.html has an empty codepoints list, which seems bad.

r=dbaron with something done about 010Cn.

Comment 85

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #82)
> I'll investigate that tomorrow, and see what's happening there. I probably
> need to add tests for that as well.
> 
> (TBH, I haven't tested much on sideways-* writing modes which I probably
> should, sorry...)

I changed my mind. I decided not to finish the support of sideways-lr in this bug, because it seems to me that our ruby impl with sideways-lr is currently broken as well, which makes it hard to write tests for this feature. Also this isn't a quite high priority issue since there is no known usecase of this combination yet.

I'm going to file a new bug for implementing the sideways-lr support for text-emphasis and another bug for fixing ruby with sideways-lr as a blocker of the former.

Base on the this, could you continue reviewing patch 11?

Comment 86

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #84)
> Comment on attachment 8691883 [details] [diff] [review]
> patch 15 v2 - reftests
> 
> Was it intentional that text-emphasis-style-property-{006,007}* don't exist
> anymore?

Yes. 006* were for testing emphasis marks being upright even with sideways orientation. But jfkthame thought this is undesired, and this behavior is actually undefined in the spec. See the discussion in comment 55 and comment 65.

007* were the tests for characters without emphasis marks. I moved that to 010 and use a script to generate those tests.

> text-emphasis-style-property-010Cn.html has an empty codepoints list, which
> seems bad.

Hmmm... because there is no character in the unassigned category... OK, probably I could just remove it from the category list, and add a warning there to ensure we do not miss it in the future if Unicode actually adds some characters to this category.

Comment 87

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #83)
> Comment on attachment 8691882 [details] [diff] [review]
> patch 14.3 v2 - make text-emphasis-position language-aware
> 
> >Bug 1040668 part 15 - Make the default value of text-emphasis-position aware of the language.
> 
> Would be nice to know if this is 14.3 or 15. :-)

It is 15 in my local sequence but I guess it is better to keep the patch number in discussion consistent, so I use the initial number of the patch to reference them in the attachment name. Sorry if it confuses you.

Comment 88

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9145df073cda

Comment 89

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #85)
> I'm going to file a new bug for implementing the sideways-lr support for
> text-emphasis and another bug for fixing ruby with sideways-lr as a blocker
> of the former.
> 
> Base on the this, could you continue reviewing patch 11?

OK, we can leave that for the followups. I'll look at patch 11 again today.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #86)
> > text-emphasis-style-property-010Cn.html has an empty codepoints list, which
> > seems bad.
> 
> Hmmm... because there is no character in the unassigned category... OK,
> probably I could just remove it from the category list, and add a warning
> there to ensure we do not miss it in the future if Unicode actually adds
> some characters to this category.

There are no characters listed as having this category in UnicodeData.txt (and I assume there never will be), but this is the default value of the General Category field, so it applies to all character codes that *aren't* explicitly assigned any other.

Many of them may become assigned and get a new category in some future version, but there are certain characters that are guaranteed to remain permanently unassigned. Specifically, the range U+FDD0 to U+FDEF, and the last two characters of each plane (U+??FFFE and U+??FFFF where ?? is 00 to 10). So you could test those, or a selection of them.

Comment 90

[Jonathan Kew [:jfkthame]]

Comment on attachment 8691879 [details] [diff] [review]
patch 11 v2 - implement emphasis mark rendering

Review of attachment 8691879 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, aside from the sideways-lr issue that will be handled later.

I'd suggest you go ahead and make the change in gfxTextRun::DrawEmphasisMarks to use IsInlineReversed() rather than IsRightToLeft(), as that will be needed in the end; the result will be the same for the non-sideways-lr case anyway (see the definition of gfxShapedText::IsInlineReversed).

Comment 91

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #90)
> Comment on attachment 8691879 [details] [diff] [review]
> patch 11 v2 - implement emphasis mark rendering
> 
> Review of attachment 8691879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, aside from the sideways-lr issue that will be handled later.
> 
> I'd suggest you go ahead and make the change in
> gfxTextRun::DrawEmphasisMarks to use IsInlineReversed() rather than
> IsRightToLeft(), as that will be needed in the end; the result will be the
> same for the non-sideways-lr case anyway (see the definition of
> gfxShapedText::IsInlineReversed).

Hmmm, I guess this change actually makes patch 3 useless.

Comment 92

[Xidorn Quan [:xidorn] UTC+11]

Interestingly, text-emphasis is broken in a similiar (but not identical) way as ruby with sideways-lr...

Comment 93

[Jonathan Kew [:jfkthame]]

Comment on attachment 8691879 [details] [diff] [review]
patch 11 v2 - implement emphasis mark rendering

Review of attachment 8691879 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +5184,5 @@
> +                           frameSize.ISize(aWM) + info->advance,
> +                           fm->MaxAscent() + fm->MaxDescent());
> +  // When the writing mode is vertical-lr, and the text orientation is
> +  // sideways, the ascent and descent are swapped.
> +  bool baselineReversed = aWM.IsVerticalLR() && aWM.IsSideways();

Actually, rather than testing IsVerticalLR() here, does it work to test IsLineInverted()? That seems like perhaps the more logical condition.

Comment 94

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #93)
> Comment on attachment 8691879 [details] [diff] [review]
> patch 11 v2 - implement emphasis mark rendering
> 
> Review of attachment 8691879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsTextFrame.cpp
> @@ +5184,5 @@
> > +                           frameSize.ISize(aWM) + info->advance,
> > +                           fm->MaxAscent() + fm->MaxDescent());
> > +  // When the writing mode is vertical-lr, and the text orientation is
> > +  // sideways, the ascent and descent are swapped.
> > +  bool baselineReversed = aWM.IsVerticalLR() && aWM.IsSideways();
> 
> Actually, rather than testing IsVerticalLR() here, does it work to test
> IsLineInverted()? That seems like perhaps the more logical condition.

Yeah, okay. I'll include this as well as the direction fix in the final commit, and get rid of patch 3.

Comment 95

[Xidorn Quan [:xidorn] UTC+11]

Errrr, there is one problem observed in test_transition_events test: if you change the color property, transitionend event would be triggered for text-emphasis-color as well (because it is derived from color by default), even if text-emphasis is not enabled, and in which case, you are still not able to get the computed value of text-emphasis-color.

Comment 96

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1.1 - avoid queuing transition event for disabled properties

FWIW, I'm going to split this patch into two parts when landing. The change in nsTransitionManager will be in a patch before the current patch 1, and the test change will be merged into the current patch 1.

Comment 97

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e80db82eba4

Comment 98

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2fc03f6c174bbfa7c2ae5ea26d5ef1ad3f8940
Bug 1040668 part 1 - Avoid queuing transition event for disabled properties. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/d57862a8ffca78eb225bdf2c0348d9260a85cdc4
Bug 1040668 part 2 - Parse and compute text emphasis properties. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/5be14f3ca65c95678a9b4e3a6de592e11598f594
Bug 1040668 part 3 - Add helper function gfxTextRun::GetAdvanceForGlyph. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/038a7b0377a6ccf0fd3432d3b88a96529c128063
Bug 1040668 part 4 - Add helper function for ensuring a glyph is a complex glyph. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8112ce8e5370bc3d50376364432055a4e5af2b
Bug 1040668 part 5 - Avoid unnecessary allocation inside EnsureComplexGlyph helper function. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/363b1e4240438a6c3e95d9999afba80f3d437ede
Bug 1040668 part 6 - Add some specifier on gfxTextRun and gfxShapedWord so that compilers are able to reason out certain optimizations. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f1d0db3905a266571df36ae8a03866575b9169
Bug 1040668 part 7 - Add NO_EMPHASIS_MARK flag in CompressedGlyph. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/b347b258754fa571778f6243b4d95e730c336bb3
Bug 1040668 part 8 - Setup text emphasis for text run. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e6eb6a06f3a994ae08db295c93d54b76febf4d
Bug 1040668 part 9 - Compute overflow from text-shadow after text decorations. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/47f1a643b90abf0e3773e332d2416ae73195fa3c
Bug 1040668 part 10 - Implement emphasis mark rendering. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbbcf30b18338b5c6164d32fb37b8b57e3e2850
Bug 1040668 part 11 - Move line leadings adjusting code into a separate function in nsLineLayout. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e21e58e22b8887111a6fdaf6a88884bd32f6521
Bug 1040668 part 12 - Add line leadings for emphasis marks if necessary. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/9686b3471f5f34d7b40b494fac0ecee84589aa5d
Bug 1040668 part 13 - Move first part of nsStyleFont::GetLanguage to nsPresContext::GetContentLanguage. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/06856fda5ac15c16c14115295fdbdd1fd8c14003
Bug 1040668 part 14 - Add helper function nsStyleUtil::MatchesLanguagePrefix for doing simple language matching. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d967e504363d8b9962f578a23a3369ffb8c4090
Bug 1040668 part 15 - Make the default value of text-emphasis-position aware of the language. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2e3685fa13186a53a90c6f0cbb9bbaff6858dd
Bug 1040668 part 16 - Add reftests for text-emphasis. r=dbaron

Comment 99

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> ISTM that we should prefer using glyphs from the same font as the text,
> where possible. In particular, the "sesame" glyphs U+FE45 '﹅' and U+FE46 '﹆'
> should have the same stroke style as the font being used.
> 
> I realize some fonts may not support the relevant characters, or may have
> bad glyphs, so some kind of check/fallback may be useful. Perhaps we'll want
> to include kenten-generic for those situations -- though I'd be a bit
> surprised if there wasn't a suitable fallback already present on major OS's.

It seems Windows XP doesn't have any fallback for sesames (U+FE45 and U+FE46). See http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32-debug/1448672310/mozilla-inbound_xp-ix-debug_test-reftest-bm127-tests1-windows-build172.txt.gz&only_show_unexpected=1

Comment 100

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9eedc59a08a1b6cfa9fc5af68e5981c4396cedf
Bug 1040668 followup - Use monospace for text-emphasis reftests. rs=dbaron on a CLOSED TREE

Comment 101

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa5f996d9627d736bebdc22701aa8d6319646f1
Bug 1040668 followup 2 - Disable failing reftests of text-emphasis on Windows XP.

Comment 102

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c2fc03f6c17
https://hg.mozilla.org/mozilla-central/rev/d57862a8ffca
https://hg.mozilla.org/mozilla-central/rev/5be14f3ca65c
https://hg.mozilla.org/mozilla-central/rev/038a7b0377a6
https://hg.mozilla.org/mozilla-central/rev/2f8112ce8e53
https://hg.mozilla.org/mozilla-central/rev/363b1e424043
https://hg.mozilla.org/mozilla-central/rev/f2f1d0db3905
https://hg.mozilla.org/mozilla-central/rev/b347b258754f
https://hg.mozilla.org/mozilla-central/rev/b8e6eb6a06f3
https://hg.mozilla.org/mozilla-central/rev/47f1a643b90a
https://hg.mozilla.org/mozilla-central/rev/8fbbcf30b183
https://hg.mozilla.org/mozilla-central/rev/8e21e58e22b8
https://hg.mozilla.org/mozilla-central/rev/9686b3471f5f
https://hg.mozilla.org/mozilla-central/rev/06856fda5ac1
https://hg.mozilla.org/mozilla-central/rev/9d967e504363
https://hg.mozilla.org/mozilla-central/rev/2c2e3685fa13
https://hg.mozilla.org/mozilla-central/rev/e9eedc59a08a
https://hg.mozilla.org/mozilla-central/rev/0aa5f996d962

Comment 103

[yisibl]

Add MDN doc: 

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

@Xidorn Quan Please review it.

Comment 104

[Jean-Yves Perrier [:teoli]]

@Percyley Thank you very much for your work, it is very much helpful and appreciated.

I made a quick editorial review and mostly "shuffled" things around (a bit). Major changes that I did were:
* in text-emphasis, I added info about the values for text-emphasis-color
* I removed a note about text-emphasis-position in that compat table (as text-emphasis-position values are not included in the shorthand).
* I fixed the computed value for text-emphasis-position (it was the one for text-emphasis-color).

I updated the wording of https://developer.mozilla.org/en-US/Firefox/Releases/45
and added the properties in: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Text_Decoration

Note that we still need documentation for text-emphasis-color and text-emphasis-style.

Thanks a lot for your help!

Comment 105

[yisibl]

(In reply to Jean-Yves Perrier [:teoli] from comment #104)
> @Percyley Thank you very much for your work, it is very much helpful and
> appreciated.
> 
> I made a quick editorial review and mostly "shuffled" things around (a bit).
> Major changes that I did were:
> * in text-emphasis, I added info about the values for text-emphasis-color
> * I removed a note about text-emphasis-position in that compat table (as
> text-emphasis-position values are not included in the shorthand).
> * I fixed the computed value for text-emphasis-position (it was the one for
> text-emphasis-color).
> 
> I updated the wording of
> https://developer.mozilla.org/en-US/Firefox/Releases/45
> and added the properties in:
> https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Text_Decoration
> 
> Note that we still need documentation for text-emphasis-color and
> text-emphasis-style.
> 
> Thanks a lot for your help!

Thank you very much for your review, also thank my colleagues ([e-mail]) to help modify the grammar json file.

I think Firefox should have a list of CSS new features, and then we can add documents in advance.

Comment 106

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #83)
> >-  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Font)) {
> >+  const nsAttrValue* langValue = aAttributes->GetAttr(nsGkAtoms::lang);
> >+  bool hasLangValue = langValue && langValue->Type() == nsAttrValue::eString;
> >+
> >+  if (hasLangValue && (aData->mSIDs & NS_STYLE_INHERIT_BIT(Font))) {
> 
> Could you put all this code inside an if() that tests just the two
> NS_STYLE_INHERIT_BIT()s?

What happened to this review comment?

I really do prefer that the NS_STYLE_INHERIT_BIT() tests are outermost in these functions, since they are called for every style struct.  Having those tests be outer-most means that for most structs they should just be a rapid sequence of tests on the same value, with no other interruptions.

Comment 107

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #106)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #83)
> > >-  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Font)) {
> > >+  const nsAttrValue* langValue = aAttributes->GetAttr(nsGkAtoms::lang);
> > >+  bool hasLangValue = langValue && langValue->Type() == nsAttrValue::eString;
> > >+
> > >+  if (hasLangValue && (aData->mSIDs & NS_STYLE_INHERIT_BIT(Font))) {
> > 
> > Could you put all this code inside an if() that tests just the two
> > NS_STYLE_INHERIT_BIT()s?
> 
> What happened to this review comment?
> 
> I really do prefer that the NS_STYLE_INHERIT_BIT() tests are outermost in
> these functions, since they are called for every style struct.  Having those
> tests be outer-most means that for most structs they should just be a rapid
> sequence of tests on the same value, with no other interruptions.

Ah... I misunderstood your comment. I assumed that this method is called for each element rather than each style struct and thus that would be false only on relatively fewer cases, so I thought you meant I should move the language value check to the outmost so that we don't need to check twice.

I'll write a followup patch to fix this.

Comment 108

[Xidorn Quan [:xidorn] UTC+11]

Attachment: followup 3 - wrap lang attribute mapping in an SID test

Thanks for reminding.

Comment 109

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8695794 [details] [diff] [review]
followup 3 - wrap lang attribute mapping in an SID test

Probably declare it as "static inline void", since it's only used in one place.

r=dbaron

Comment 110

[Jean-Yves Perrier [:teoli]]

I've also added:
https://developer.mozilla.org/en-US/docs/Web/CSS/text-emphasis-color
and
https://developer.mozilla.org/en-US/docs/Web/CSS/text-emphasis-style

Comment 111

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d891ed3195225c3444f01b1b14324795443c656
Bug 1040668 followup 3 - Wrap lang attribute mapping code in NS_STYLE_INHERIT_BIT test. r=dbaron

Comment 112

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8d891ed31952