bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Implement -webkit-text-stroke

RESOLVED FIXED in Firefox 48

Status

()

Core
Layout: Text
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: miketaylr, Assigned: jeremychen)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla48
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(URL)

Attachments

(4 attachments, 25 obsolete attachments)

7.36 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
34.03 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
22.40 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
11.71 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
Introduced in https://webkit.org/blog/85/introducing-text-stroke/, this is used by sites to do fancy typography effects (see Bug 1248644 for some examples). It seems if we only implement -webkit-background-clip:text and -webkit-text-fill-color but not this, we will be breaking sites in unintended ways.

Introduced in https://webkit.org/blog/85/introducing-text-stroke/:

-webkit-text-stroke-color – This property allows you to specify a stroke color for text. If it is not set, then the color property will be used to do the stroke.
-webkit-text-stroke-width – This property allows you to specify the width of the stroke. It defaults to 0, meaning that only a fill is performed. You can specify a length for this value, and in addition the values thin, medium and thick can be used (with ‘thin’ being most like the outline behavior of OS X).
-webkit-text-stroke – This property is a shorthand for the two stroke properties.

Here's a link to issue to spec this: https://github.com/whatwg/compat/issues/40
See Also: → bug 1247777

Updated

2 years ago
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Just pushed the initial spec definitions to https://compat.spec.whatwg.org/#text-fill-and-stroking (I've lumped -webkit-text-fill-color in with the stroke props). (Feedback welcome, I still need to add some examples and maybe diagrams, etc.)
I'm working on this bug from today.
After discussing with Jeremy offline, he might be interested in implementing this bug since this is somewhat related to bug 1247777.
Assignee: tlin → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jeremychen)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #3)
> After discussing with Jeremy offline, he might be interested in implementing
> this bug since this is somewhat related to bug 1247777.

Yeah, this bug is related to bug 1247777. I'll work on this once bug 1247777 is resolved.
Take this bug first.
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Flags: needinfo?(jeremychen)
I'm working on this bug from today.
Depends on: 1247777
Blocks: 1259345
Created attachment 8734283 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property.

Hi Xidorn, could you take a look and give me some feedback? Thank you.
Attachment #8734283 - Flags: feedback?(quanxunzhen)
Comment on attachment 8734283 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property.

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

::: layout/style/nsStyleStruct.h
@@ +1838,5 @@
>  };
>  
> +// Webkit-text-stroke-width is rounded to the nearest-below integer number of pixels,
> +// but values between zero and one device pixels are always rounded up to
> +// one device pixel.

Why do you want to do this? It doesn't seem to me the spec says anything about this.

@@ +1840,5 @@
> +// Webkit-text-stroke-width is rounded to the nearest-below integer number of pixels,
> +// but values between zero and one device pixels are always rounded up to
> +// one device pixel.
> +#define NS_ROUND_WIDTH_TO_PIXELS(l,tpp) \
> +  ((l) == 0) ? 0 : std::max((tpp), (l) / (tpp) * (tpp))

Even if we need this, it should probably be a function rather than a macro. Function is type-safe, and compilers are able to inline this kind of small functions automatically.

@@ +1973,5 @@
>    inline bool WordCanWrap(const nsIFrame* aContextFrame) const;
>  
>    mozilla::LogicalSide TextEmphasisSide(mozilla::WritingMode aWM) const;
> +private:
> +  nscoord mTwipsPerPixel;

I think the ratio between device pixel and app unit could change without a restyle, e.g. when the window is moved from a HiDPI screen to a LoDPI screen. That says, storing it here won't work as expected.
Attachment #8734283 - Flags: feedback?(quanxunzhen) → feedback-
OK, it seems I was wrong, and now I know why Jeremy wanted to do that. So it seems we round border-width and some other properties to device pixel (in bug 287624), and also store mTwipsPerPixel in style structs. And we do force recomputing style data when that value changes.

The reason we round border-width and others is that we do not do subpixel rendering for them, but we want to keep their width consistent no matter where they start.

However we may want to do subpixel rendering for text-stroke (and it seems we do so in SVG), so we probably shouldn't round the value here.
I agree I don't think we should be rounding to whole pixel values here.
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #8)
> OK, it seems I was wrong, and now I know why Jeremy wanted to do that. So it
> seems we round border-width and some other properties to device pixel (in
> bug 287624), and also store mTwipsPerPixel in style structs. And we do force
> recomputing style data when that value changes.
> 
> The reason we round border-width and others is that we do not do subpixel
> rendering for them, but we want to keep their width consistent no matter
> where they start.
> 
> However we may want to do subpixel rendering for text-stroke (and it seems
> we do so in SVG), so we probably shouldn't round the value here.

Xidorn, Cameron, thank you for the feedback. I'll remove the rounding part of the patch.
Comment hidden (obsolete)
Attachment #8734283 - Attachment is obsolete: true
Attachment #8734694 - Flags: feedback?(quanxunzhen)
Created attachment 8734732 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v2)

Address xidorn and heycam's comments.

Use nsStyleCoord instead of nscoord to store computed value, so the computing in
nsRuleNode could be slightly simplfied.
Attachment #8734732 - Flags: feedback?(quanxunzhen)
Attachment #8734694 - Attachment is obsolete: true
Comment on attachment 8734732 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v2)

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

Looks fine to me.

For your commit message, you should use "r?heycam" instead of "r=". "r=" could be confusing as it usually indicates that this patch has been reviewed.

::: layout/style/nsCSSParser.cpp
@@ +10727,5 @@
> +    eCSSProperty__webkit_text_stroke_width,
> +    eCSSProperty__webkit_text_stroke_color
> +  };
> +
> +  const size_t numProps = MOZ_ARRAY_LENGTH(kWebkitTextStrokeIDs);

Probably use ArrayLength function instead of MOZ_ARRAY_LENGTH for C++ code.

::: layout/style/test/test_transitions_events.html
@@ +212,5 @@
> +          ok(!got_one_target_webkittextstrokecolor,
> +             "transitionend on one on target (-webkit-text-stroke-color)");
> +          got_one_target_webkittextstrokecolor = true;
> +          event.stopPropagation();
> +          break;

Indent here should be 2 spaces fewer.

@@ +245,5 @@
>  if (SpecialPowers.getBoolPref("layout.css.prefixes.webkit")) {
>    started_test(); // -webkit-text-fill-color on #one
>  }
> +if (SpecialPowers.getBoolPref("layout.css.prefixes.webkit")) {
> +  started_test(); // -webkit-text-stroke-color on #one

This can be merged to the if-statement above.
Attachment #8734732 - Flags: feedback?(quanxunzhen) → feedback+
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #13)
> Comment on attachment 8734732 [details] [diff] [review]
> Part1: parse and compute -webkit-text-stroke property. (v2)
> 
> Review of attachment 8734732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me.
> 
> For your commit message, you should use "r?heycam" instead of "r=". "r="
> could be confusing as it usually indicates that this patch has been reviewed.
> 
> ::: layout/style/nsCSSParser.cpp
> @@ +10727,5 @@
> > +    eCSSProperty__webkit_text_stroke_width,
> > +    eCSSProperty__webkit_text_stroke_color
> > +  };
> > +
> > +  const size_t numProps = MOZ_ARRAY_LENGTH(kWebkitTextStrokeIDs);
> 
> Probably use ArrayLength function instead of MOZ_ARRAY_LENGTH for C++ code.
> 
> ::: layout/style/test/test_transitions_events.html
> @@ +212,5 @@
> > +          ok(!got_one_target_webkittextstrokecolor,
> > +             "transitionend on one on target (-webkit-text-stroke-color)");
> > +          got_one_target_webkittextstrokecolor = true;
> > +          event.stopPropagation();
> > +          break;
> 
> Indent here should be 2 spaces fewer.
> 
> @@ +245,5 @@
> >  if (SpecialPowers.getBoolPref("layout.css.prefixes.webkit")) {
> >    started_test(); // -webkit-text-fill-color on #one
> >  }
> > +if (SpecialPowers.getBoolPref("layout.css.prefixes.webkit")) {
> > +  started_test(); // -webkit-text-stroke-color on #one
> 
> This can be merged to the if-statement above.

Thank you. I'll address your comments and move forward to rendering part.
Created attachment 8738510 [details] [diff] [review]
Part2: render -webkit-text-stroke property.

xidorn, could you take a look and give me some feedback?
Thank you.
Attachment #8738510 - Flags: feedback?(quanxunzhen)
Created attachment 8738511 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v3,carry f+:xidorn)

Address xidorn's comment.
Rebase.
Attachment #8738511 - Flags: feedback+
Attachment #8734732 - Attachment is obsolete: true
Comment on attachment 8738510 [details] [diff] [review]
Part2: render -webkit-text-stroke property.

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

::: gfx/thebes/DrawMode.h
@@ +17,5 @@
>    //  stroke underneath the fill.
> +  GLYPH_STROKE_UNDERNEATH = 3,
> +  // Appends glyphs to the current path. Can NOT be used with
> +  //  GLYPH_FILL or GLYPH_STROKE.
> +  GLYPH_PATH = 4

I don't understand this change. Isn't it supposed to be a bit flag?

::: layout/generic/nsTextFrame.cpp
@@ +6609,5 @@
> +    nsStyleCoord coord = textStyle->mWebkitTextStrokeWidth;
> +    float strokeWidth =
> +      (coord.GetUnit() == eStyleUnit_Coord)
> +      ? nsPresContext::AppUnitsToFloatCSSPixels(coord.GetCoordValue()) : 0.0f;
> +    nscolor strokeColor = aStyleContext->GetTextStrokeColor();

I wonder whether it is better to get them here or put them into DrawTextRunParams...

@@ +6612,5 @@
> +      ? nsPresContext::AppUnitsToFloatCSSPixels(coord.GetCoordValue()) : 0.0f;
> +    nscolor strokeColor = aStyleContext->GetTextStrokeColor();
> +    if ((strokeWidth != 0.0f) && (NS_GET_A(strokeColor) != 0)) {
> +      params.drawMode = (params.drawMode == DrawMode::GLYPH_FILL)
> +                        ? DrawMode::GLYPH_STROKE_UNDERNEATH : DrawMode::GLYPH_STROKE;                        

Trailing spaces should be removed.

And the logic here doesn't looks right to me. When the text is not transparent, and there is some stroke, shouldn't the drawMode be GLYPH_STROKE | GLYPH_FILL?

Looking at the comment above GLYPH_STROKE_UNDERNEATH, I believe that flag is for drawing stroke underneath the fill **when** both GLYPH_STROKE and GLYPH_FILL are set, because otherwise stroke should be drawn on top of fill. It is not supposed to be set independently. Though I have no idea how that flag could be useful.
Attachment #8738510 - Flags: feedback?(quanxunzhen) → feedback-
Comment on attachment 8738511 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v3,carry f+:xidorn)

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

::: layout/style/nsStyleContext.cpp
@@ +1115,5 @@
> +          thisVisText->mWebkitTextFillColor != otherVisText->mWebkitTextFillColor ||
> +          thisVisText->mWebkitTextStrokeColorForeground !=
> +          otherVisText->mWebkitTextStrokeColorForeground ||
> +          thisVisText->mWebkitTextStrokeColor != otherVisText->mWebkitTextStrokeColor ||
> +          thisVisText->mWebkitTextStrokeWidth != otherVisText->mWebkitTextStrokeWidth) {

I don't think text-stroke-width is needed here. Color properties are added here because they could be affected by "color", which can be changed for :visited. But text-stroke-width should not be changable for :visited.

::: layout/style/nsStyleStruct.cpp
@@ +3786,5 @@
>        mWebkitTextFillColorForeground != aOther.mWebkitTextFillColorForeground ||
> +      mWebkitTextFillColor != aOther.mWebkitTextFillColor ||
> +      mWebkitTextStrokeColorForeground != aOther.mWebkitTextStrokeColorForeground ||
> +      mWebkitTextStrokeColor != aOther.mWebkitTextStrokeColor ||
> +      mWebkitTextStrokeWidth != aOther.mWebkitTextStrokeWidth) {

I guess change on text-stroke-width may need recompute overflow area.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> Comment on attachment 8738510 [details] [diff] [review]
> Part2: render -webkit-text-stroke property.
> 
> Review of attachment 8738510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/DrawMode.h
> @@ +17,5 @@
> >    //  stroke underneath the fill.
> > +  GLYPH_STROKE_UNDERNEATH = 3,
> > +  // Appends glyphs to the current path. Can NOT be used with
> > +  //  GLYPH_FILL or GLYPH_STROKE.
> > +  GLYPH_PATH = 4
> 
> I don't understand this change. Isn't it supposed to be a bit flag?

At first, I thought we will need a new slot for setting both GLYPH_FILL and GLYPH_STROKE.
But now, I kind of get your point. I'll fix this in next version.

> ::: layout/generic/nsTextFrame.cpp
> @@ +6609,5 @@
> > +    nsStyleCoord coord = textStyle->mWebkitTextStrokeWidth;
> > +    float strokeWidth =
> > +      (coord.GetUnit() == eStyleUnit_Coord)
> > +      ? nsPresContext::AppUnitsToFloatCSSPixels(coord.GetCoordValue()) : 0.0f;
> > +    nscolor strokeColor = aStyleContext->GetTextStrokeColor();
> 
> I wonder whether it is better to get them here or put them into
> DrawTextRunParams...

Yeah, either way can do the implementation. Do you prefer to put them into DrawTextRunParams?

> @@ +6612,5 @@
> > +      ? nsPresContext::AppUnitsToFloatCSSPixels(coord.GetCoordValue()) : 0.0f;
> > +    nscolor strokeColor = aStyleContext->GetTextStrokeColor();
> > +    if ((strokeWidth != 0.0f) && (NS_GET_A(strokeColor) != 0)) {
> > +      params.drawMode = (params.drawMode == DrawMode::GLYPH_FILL)
> > +                        ? DrawMode::GLYPH_STROKE_UNDERNEATH : DrawMode::GLYPH_STROKE;                        
> 
> Trailing spaces should be removed.

okay.

> 
> And the logic here doesn't looks right to me. When the text is not
> transparent, and there is some stroke, shouldn't the drawMode be
> GLYPH_STROKE | GLYPH_FILL?
> 
> Looking at the comment above GLYPH_STROKE_UNDERNEATH, I believe that flag is
> for drawing stroke underneath the fill **when** both GLYPH_STROKE and
> GLYPH_FILL are set, because otherwise stroke should be drawn on top of fill.
> It is not supposed to be set independently. Though I have no idea how that
> flag could be useful.

Alright, I guess I've been confused by the comment above GLYPH_STROKE_UNDERNEATH and misunderstanding that GLYPH_STROKE_UNDERNEATH stands for setting both GLYPH_FILL and GLYPH_STROKE. I'll fix this in next version as well.
Flags: needinfo?(quanxunzhen)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #18)
> Comment on attachment 8738511 [details] [diff] [review]
> Part1: parse and compute -webkit-text-stroke property. (v3,carry f+:xidorn)
> 
> Review of attachment 8738511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsStyleContext.cpp
> @@ +1115,5 @@
> > +          thisVisText->mWebkitTextFillColor != otherVisText->mWebkitTextFillColor ||
> > +          thisVisText->mWebkitTextStrokeColorForeground !=
> > +          otherVisText->mWebkitTextStrokeColorForeground ||
> > +          thisVisText->mWebkitTextStrokeColor != otherVisText->mWebkitTextStrokeColor ||
> > +          thisVisText->mWebkitTextStrokeWidth != otherVisText->mWebkitTextStrokeWidth) {
> 
> I don't think text-stroke-width is needed here. Color properties are added
> here because they could be affected by "color", which can be changed for
> :visited. But text-stroke-width should not be changable for :visited.

Okay, I'll remove text-stroke-width part.

> ::: layout/style/nsStyleStruct.cpp
> @@ +3786,5 @@
> >        mWebkitTextFillColorForeground != aOther.mWebkitTextFillColorForeground ||
> > +      mWebkitTextFillColor != aOther.mWebkitTextFillColor ||
> > +      mWebkitTextStrokeColorForeground != aOther.mWebkitTextStrokeColorForeground ||
> > +      mWebkitTextStrokeColor != aOther.mWebkitTextStrokeColor ||
> > +      mWebkitTextStrokeWidth != aOther.mWebkitTextStrokeWidth) {
> 
> I guess change on text-stroke-width may need recompute overflow area.

So, are you suggesting that I keep it in this way?
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #19)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> > Comment on attachment 8738510 [details] [diff] [review]
> > Part2: render -webkit-text-stroke property.
> > 
> > Review of attachment 8738510 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: layout/generic/nsTextFrame.cpp
> > @@ +6609,5 @@
> > > +    nsStyleCoord coord = textStyle->mWebkitTextStrokeWidth;
> > > +    float strokeWidth =
> > > +      (coord.GetUnit() == eStyleUnit_Coord)
> > > +      ? nsPresContext::AppUnitsToFloatCSSPixels(coord.GetCoordValue()) : 0.0f;
> > > +    nscolor strokeColor = aStyleContext->GetTextStrokeColor();
> > 
> > I wonder whether it is better to get them here or put them into
> > DrawTextRunParams...
> 
> Yeah, either way can do the implementation. Do you prefer to put them into
> DrawTextRunParams?

I guess I prefer that here, yes. It seems to save computation. So in your current setting, you would need an addition StyleText() call for the text run and hyphen if present. However, in every possible path you we create DrawTextRunParams we already have StyleText() called. So you have 1~2 extra StyleText() calls here in a hot path. It isn't a big deal, but it is avoidable anyway.

(I actually feels that we should move the GetTextStrokeColor() / GetTextFillColor() into nsStyleText and make it take an nsStyleContext* param rather than having it in nsStyleContext. That could save a call to StyleText() in various places.)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #20)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #18)
> > Comment on attachment 8738511 [details] [diff] [review]
> > Part1: parse and compute -webkit-text-stroke property. (v3,carry f+:xidorn)
> > 
> > Review of attachment 8738511 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: layout/style/nsStyleStruct.cpp
> > @@ +3786,5 @@
> > >        mWebkitTextFillColorForeground != aOther.mWebkitTextFillColorForeground ||
> > > +      mWebkitTextFillColor != aOther.mWebkitTextFillColor ||
> > > +      mWebkitTextStrokeColorForeground != aOther.mWebkitTextStrokeColorForeground ||
> > > +      mWebkitTextStrokeColor != aOther.mWebkitTextStrokeColor ||
> > > +      mWebkitTextStrokeWidth != aOther.mWebkitTextStrokeWidth) {
> > 
> > I guess change on text-stroke-width may need recompute overflow area.
> 
> So, are you suggesting that I keep it in this way?

It seems to me your current rendering code doesn't handle overflow rect at all. You may want to try some very heavy stroke width to see whether it would be an issue.

If it is, you should take text-stroke-width into account in nsTextFrame::UpdateOverflow() (probably actually the code should go to nsTextFrame::RecomputeOverflow()). And in that case, you need to update CalcDifference() here to make it return nsChangeHint_UpdateOverflow in addition to paint-related flags for text-stroke-width change.
Flags: needinfo?(quanxunzhen)
Created attachment 8741367 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v4,carry f+:xidorn)

Address xidorn's comment.
Return overflow nsChangeHint for text-stroke-width change.

Overflow handling for rendering part would be updated in next version part2 patch.
Attachment #8738511 - Attachment is obsolete: true
Created attachment 8741696 [details] [diff] [review]
Part2: render -webkit-text-stroke property. (v2)

Address xidorn's comments.
Handle overflow rect for text stroke.

As to Comment 21, could you enlighten me a bit more?
I'm not sure about the pros/cons between having GetTextStrokeColor()/GetTextFillColor()
in nsStyleText and having them in nsStyleContext.
Attachment #8738510 - Attachment is obsolete: true
Attachment #8741696 - Flags: feedback?(bugzilla)
Comment on attachment 8741696 [details] [diff] [review]
Part2: render -webkit-text-stroke property. (v2)

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

::: layout/generic/nsTextFrame.cpp
@@ +5372,5 @@
> +  // text-stroke overflows
> +  nsRect strokeRect = *aVisualOverflowRect;
> +  nsStyleCoord coord = StyleText()->mWebkitTextStrokeWidth;
> +  float textStrokeWidth = (coord.GetUnit() == eStyleUnit_Coord)
> +                          ? coord.GetCoordValue() : 0.0f;

Can the unit be something else? If it can, does giving it 0.0f make sense here? If it cannot, you should just ignore the unit and let GetCoordValue asserts in that case.

@@ +5379,5 @@
> +    strokeRect.y -= textStrokeWidth;
> +    strokeRect.width += textStrokeWidth;
> +    strokeRect.height += textStrokeWidth;
> +  }
> +  aVisualOverflowRect->UnionRect(*aVisualOverflowRect, strokeRect);

This line, as well as the nsRect declaration, should be inside the if-statement.

@@ +6588,5 @@
> +    }
> +
> +    if ((NS_GET_A(aParams.textStrokeColor) != 0) &&
> +        (aParams.textStrokeWidth != 0.0f)) {
> +      params.drawMode = (params.drawMode == DrawMode::GLYPH_FILL)

The extra parentheses in the three lines above are not necessary. The precedence between the four operators should be clear enough.

@@ +6589,5 @@
> +
> +    if ((NS_GET_A(aParams.textStrokeColor) != 0) &&
> +        (aParams.textStrokeWidth != 0.0f)) {
> +      params.drawMode = (params.drawMode == DrawMode::GLYPH_FILL)
> +                        ? DrawMode::GLYPH_FILL_AND_STROKE : DrawMode::GLYPH_STROKE;

It seems complicated... I think it should be "or"ed together directly. Probably the "enum class" is stopping you from doing so. We probably should use mfbt/TypedEnumBits.h for DrawMode.

Could you probably add a preparation patch to do so?

@@ +6610,5 @@
> +  const nsStyleText* textStyle = StyleText();
> +  nsStyleCoord coord = textStyle->mWebkitTextStrokeWidth;
> +  float strokeWidth =
> +    (coord.GetUnit() == eStyleUnit_Coord)
> +    ? nsPresContext::AppUnitsToFloatCSSPixels(coord.GetCoordValue()) : 0.0f;

Same as above about the unit.

@@ +6615,5 @@
> +  nscolor strokeColor = StyleContext()->GetTextStrokeColor();
> +  if ((strokeWidth != 0.0f) && (NS_GET_A(strokeColor) != 0)) {
> +    params.textStrokeWidth = strokeWidth;
> +    params.textStrokeColor = strokeColor;
> +  }

Also, why is this code here, rather than in nsTextFrame::PaintText for example? What would it look like in shadow and selection?

::: layout/style/nsStyleStruct.h
@@ +2034,5 @@
>      return !mTextEmphasisStyleString.IsEmpty();
>    }
>  
> +  bool HasWebkitTextStroke() const {
> +    return (mWebkitTextStrokeWidth.GetCoordValue() > 0.0f);

The parentheses do not look necessary either.
Attachment #8741696 - Flags: feedback?(bugzilla) → feedback-
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> Comment on attachment 8741696 [details] [diff] [review]
> Part2: render -webkit-text-stroke property. (v2)
> 
> Review of attachment 8741696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsTextFrame.cpp
> @@ +5372,5 @@
> > +  // text-stroke overflows
> > +  nsRect strokeRect = *aVisualOverflowRect;
> > +  nsStyleCoord coord = StyleText()->mWebkitTextStrokeWidth;
> > +  float textStrokeWidth = (coord.GetUnit() == eStyleUnit_Coord)
> > +                          ? coord.GetCoordValue() : 0.0f;
> 
> Can the unit be something else? If it can, does giving it 0.0f make sense
> here? If it cannot, you should just ignore the unit and let GetCoordValue
> asserts in that case.

Okay.

> @@ +5379,5 @@
> > +    strokeRect.y -= textStrokeWidth;
> > +    strokeRect.width += textStrokeWidth;
> > +    strokeRect.height += textStrokeWidth;
> > +  }
> > +  aVisualOverflowRect->UnionRect(*aVisualOverflowRect, strokeRect);
> 
> This line, as well as the nsRect declaration, should be inside the
> if-statement.

Okay.

> @@ +6588,5 @@
> > +    }
> > +
> > +    if ((NS_GET_A(aParams.textStrokeColor) != 0) &&
> > +        (aParams.textStrokeWidth != 0.0f)) {
> > +      params.drawMode = (params.drawMode == DrawMode::GLYPH_FILL)
> 
> The extra parentheses in the three lines above are not necessary. The
> precedence between the four operators should be clear enough.

Okay.

> @@ +6589,5 @@
> > +
> > +    if ((NS_GET_A(aParams.textStrokeColor) != 0) &&
> > +        (aParams.textStrokeWidth != 0.0f)) {
> > +      params.drawMode = (params.drawMode == DrawMode::GLYPH_FILL)
> > +                        ? DrawMode::GLYPH_FILL_AND_STROKE : DrawMode::GLYPH_STROKE;
> 
> It seems complicated... I think it should be "or"ed together directly.
> Probably the "enum class" is stopping you from doing so. We probably should
> use mfbt/TypedEnumBits.h for DrawMode.
> 
> Could you probably add a preparation patch to do so?

You mean create a separated patch for using mfbt/TypedEnumBits.h for DrawMode?

> @@ +6610,5 @@
> > +  const nsStyleText* textStyle = StyleText();
> > +  nsStyleCoord coord = textStyle->mWebkitTextStrokeWidth;
> > +  float strokeWidth =
> > +    (coord.GetUnit() == eStyleUnit_Coord)
> > +    ? nsPresContext::AppUnitsToFloatCSSPixels(coord.GetCoordValue()) : 0.0f;
> 
> Same as above about the unit.

Okay.

> @@ +6615,5 @@
> > +  nscolor strokeColor = StyleContext()->GetTextStrokeColor();
> > +  if ((strokeWidth != 0.0f) && (NS_GET_A(strokeColor) != 0)) {
> > +    params.textStrokeWidth = strokeWidth;
> > +    params.textStrokeColor = strokeColor;
> > +  }
> 
> Also, why is this code here, rather than in nsTextFrame::PaintText for
> example? What would it look like in shadow and selection?

I'll do a brief verification. If nothing changes while setting strokes in shadow and selection, we shall be fine to only take care of text stroke in nsTextFrame::PaintText, right?

> ::: layout/style/nsStyleStruct.h
> @@ +2034,5 @@
> >      return !mTextEmphasisStyleString.IsEmpty();
> >    }
> >  
> > +  bool HasWebkitTextStroke() const {
> > +    return (mWebkitTextStrokeWidth.GetCoordValue() > 0.0f);
> 
> The parentheses do not look necessary either.

Okay.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #26)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> > Comment on attachment 8741696 [details] [diff] [review]
> > Part2: render -webkit-text-stroke property. (v2)
> > 
> > Review of attachment 8741696 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +6589,5 @@
> > > +
> > > +    if ((NS_GET_A(aParams.textStrokeColor) != 0) &&
> > > +        (aParams.textStrokeWidth != 0.0f)) {
> > > +      params.drawMode = (params.drawMode == DrawMode::GLYPH_FILL)
> > > +                        ? DrawMode::GLYPH_FILL_AND_STROKE : DrawMode::GLYPH_STROKE;
> > 
> > It seems complicated... I think it should be "or"ed together directly.
> > Probably the "enum class" is stopping you from doing so. We probably should
> > use mfbt/TypedEnumBits.h for DrawMode.
> > 
> > Could you probably add a preparation patch to do so?
> 
> You mean create a separated patch for using mfbt/TypedEnumBits.h for
> DrawMode?

Yes.

> > @@ +6615,5 @@
> > > +  nscolor strokeColor = StyleContext()->GetTextStrokeColor();
> > > +  if ((strokeWidth != 0.0f) && (NS_GET_A(strokeColor) != 0)) {
> > > +    params.textStrokeWidth = strokeWidth;
> > > +    params.textStrokeColor = strokeColor;
> > > +  }
> > 
> > Also, why is this code here, rather than in nsTextFrame::PaintText for
> > example? What would it look like in shadow and selection?
> 
> I'll do a brief verification. If nothing changes while setting strokes in
> shadow and selection, we shall be fine to only take care of text stroke in
> nsTextFrame::PaintText, right?

I don't think it should be set here anyway... And doing a unconditional copy at this place could affect performance I suppose.
Comment hidden (obsolete)
Attachment #8741696 - Attachment is obsolete: true
Comment hidden (obsolete)
Attachment #8742032 - Flags: feedback?(bugzilla)
Attachment #8742044 - Flags: feedback?(bugzilla)
Comment hidden (obsolete)
Comment hidden (obsolete)
Created attachment 8742107 [details] [diff] [review]
Part2.1: use mfbt/TypedEnumBits.h for DrawMode.
Attachment #8742044 - Attachment is obsolete: true
Created attachment 8742108 [details] [diff] [review]
Part2.2: render -webkit-text-stroke property. (v3)

Address xidorn's comment.
Set stroke property in nsTextFrame::PaintText and nsTextFrame::PaintTextWithSelectionColors.
Attachment #8742032 - Attachment is obsolete: true
Attachment #8742107 - Flags: feedback?(bugzilla)
Attachment #8742108 - Flags: feedback?(bugzilla)
Comment on attachment 8742107 [details] [diff] [review]
Part2.1: use mfbt/TypedEnumBits.h for DrawMode.

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

::: gfx/thebes/gfxTextRun.cpp
@@ +573,5 @@
>      NS_ASSERTION(aParams.drawMode == DrawMode::GLYPH_PATH || !aParams.callbacks,
>                   "callback must not be specified unless using GLYPH_PATH");
>  
>      bool skipDrawing = mSkipDrawing;
> +    if (aParams.drawMode & DrawMode::GLYPH_FILL) {

This change doesn't seem to belong to this changeset.

@@ +603,5 @@
>      BufferAlphaColor syntheticBoldBuffer(aParams.context);
>      Color currentColor;
>      bool needToRestore = false;
>  
> +    if (aParams.drawMode & DrawMode::GLYPH_FILL &&

And this.

@@ +643,5 @@
>          uint32_t end = iter.GetStringEnd();
>          Range ligatureRange(start, end);
>          ShrinkToLigatureBoundaries(&ligatureRange);
>  
> +        bool drawPartial = (aParams.drawMode & DrawMode::GLYPH_FILL) ||

And this.
Attachment #8742107 - Flags: feedback?(bugzilla) → feedback+
Comment on attachment 8742108 [details] [diff] [review]
Part2.2: render -webkit-text-stroke property. (v3)

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

::: gfx/thebes/gfxFont.cpp
@@ +1627,1 @@
>              FlushStroke(buf, state);

I guess you probably want to restore the original state.color after calling FlushStroke here.

::: layout/generic/nsTextFrame.cpp
@@ +5384,5 @@
>                                     UpdateTextEmphasis(wm, aProvider));
>    }
>  
> +  // text-stroke overflows
> +  nsRect strokeRect = *aVisualOverflowRect;

It seems this can be moved into the if-block as well.

@@ +6549,5 @@
>      gfxColor.a *= aOpacity;
>      foregroundColor = gfxColor.ToABGR();
>    }
>  
> +  nscoord textStrokeColor = textPaintStyle.GetWebkitTextStrokeColor();

Should be nscolor I suppose?
Attachment #8742108 - Flags: feedback?(bugzilla) → feedback+
Comment hidden (obsolete)
Attachment #8742107 - Attachment is obsolete: true
Comment hidden (obsolete)
Attachment #8742108 - Attachment is obsolete: true
Created attachment 8742182 [details] [diff] [review]
Part2.1: use mfbt/TypedEnumBits.h for DrawMode. (v2,carry f+:xidorn)
Attachment #8742176 - Attachment is obsolete: true
Created attachment 8742183 [details] [diff] [review]
Part2.2: render -webkit-text-stroke property. (v4,carry f+:xidorn)

Address xidorn's comment.
Restore color property if calling FlushStroke() before filling text.
Attachment #8742177 - Attachment is obsolete: true
Attachment #8741367 - Flags: review?(cam)
Attachment #8742182 - Flags: review?(jfkthame)
Attachment #8742183 - Flags: review?(jfkthame)
Attachment #8742182 - Flags: review?(jfkthame) → review+
Comment on attachment 8742183 [details] [diff] [review]
Part2.2: render -webkit-text-stroke property. (v4,carry f+:xidorn)

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

::: gfx/thebes/gfxFont.cpp
@@ +1719,5 @@
>              mFontParams.scaledFont->GetPathForGlyphs(aBuf, mRunParams.dt);
> +        mRunParams.dt->Stroke(path, ColorPattern(aState.color), aState.strokeOptions);
> +
> +        // XXX: Do we still need this? Not sure if this is saved for SVG. Since
> +        //      SVG use its own callback, I suppose that we could purge these codes.

Could you look into this a bit more, and figure out the answer... or ask someone who knows about SVG and how it renders stuff, maybe :jwatt or longsonr? If this code is now dead, we should get rid of it. (I suspect that's the case.)

Right now, it looks like if we get here and |mFontParams.contextPaint| is non-null, and GetStrokePattern() returns a pattern, we'd end up stroking with the color and then also with pattern, which doesn't seem right.

::: gfx/thebes/gfxFont.h
@@ +2173,5 @@
>      mozilla::gfx::Color      fontSmoothingBGColor;
>      gfxFloat                 direction;
>      double                   devPerApp;
> +    float                    textStrokeWidth;
> +    uint32_t                 textStrokeColor;

It seems ugly to use a uint32_t for textStrokeColor; can't we use nscolor here?

::: gfx/thebes/gfxTextRun.h
@@ +235,5 @@
>      struct DrawParams
>      {
>          gfxContext* context;
>          DrawMode drawMode = DrawMode::GLYPH_FILL;
> +        uint32_t textStrokeColor = 0;

same here -- nscolor?

::: layout/generic/nsTextFrame.cpp
@@ +287,5 @@
>    }
>  
>    nscolor GetTextColor();
> +
> +  // SVG text has its own painting process, so we shall never get its stroke

s/shall/should/

@@ +294,5 @@
> +    NS_ASSERTION(!mFrame->IsSVGText(),
> +                 "don't get WebkitTextStroke property for SVG text");
> +    return mFrame->StyleContext()->GetTextStrokeColor();
> +  }
> +  float GetWebkitTextStrokeWidth () {

there's a stray space before the ()

@@ +5385,5 @@
>    }
>  
> +  // text-stroke overflows
> +  float textStrokeWidth = StyleText()->mWebkitTextStrokeWidth.GetCoordValue();
> +  if (textStrokeWidth > 0.0f) {

shouldn't |textStrokeWidth| here be an |nscoord| (integer) value?

::: layout/style/nsStyleStruct.h
@@ +2034,5 @@
>      return !mTextEmphasisStyleString.IsEmpty();
>    }
>  
> +  bool HasWebkitTextStroke() const {
> +    return mWebkitTextStrokeWidth.GetCoordValue() > 0.0f;

GetCoordValue() returns an nscoord, which is an integer -- so you don't want to force the comparison to be floating-point...
(In reply to Jonathan Kew (:jfkthame) from comment #40)
> Comment on attachment 8742183 [details] [diff] [review]
> Part2.2: render -webkit-text-stroke property. (v4,carry f+:xidorn)
> 
> Review of attachment 8742183 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFont.cpp
> @@ +1719,5 @@
> >              mFontParams.scaledFont->GetPathForGlyphs(aBuf, mRunParams.dt);
> > +        mRunParams.dt->Stroke(path, ColorPattern(aState.color), aState.strokeOptions);
> > +
> > +        // XXX: Do we still need this? Not sure if this is saved for SVG. Since
> > +        //      SVG use its own callback, I suppose that we could purge these codes.
> 
> Could you look into this a bit more, and figure out the answer... or ask
> someone who knows about SVG and how it renders stuff, maybe :jwatt or
> longsonr? If this code is now dead, we should get rid of it. (I suspect
> that's the case.)
> 
> Right now, it looks like if we get here and |mFontParams.contextPaint| is
> non-null, and GetStrokePattern() returns a pattern, we'd end up stroking
> with the color and then also with pattern, which doesn't seem right.

According to the code path, FlushStroke() seems been dead for long time, because DrawMode::GLYPH_STROKE is never been used before). I'll ni people you suggested to confirm this.

> ::: gfx/thebes/gfxFont.h
> @@ +2173,5 @@
> >      mozilla::gfx::Color      fontSmoothingBGColor;
> >      gfxFloat                 direction;
> >      double                   devPerApp;
> > +    float                    textStrokeWidth;
> > +    uint32_t                 textStrokeColor;
> 
> It seems ugly to use a uint32_t for textStrokeColor; can't we use nscolor
> here?
> 
> ::: gfx/thebes/gfxTextRun.h
> @@ +235,5 @@
> >      struct DrawParams
> >      {
> >          gfxContext* context;
> >          DrawMode drawMode = DrawMode::GLYPH_FILL;
> > +        uint32_t textStrokeColor = 0;
> 
> same here -- nscolor?

Okay. To do so, an extra "nsColor.h" would be included in these two files. I'll update this in next version.

> ::: layout/generic/nsTextFrame.cpp
> @@ +287,5 @@
> >    }
> >  
> >    nscolor GetTextColor();
> > +
> > +  // SVG text has its own painting process, so we shall never get its stroke
> 
> s/shall/should/

Okay.

> @@ +294,5 @@
> > +    NS_ASSERTION(!mFrame->IsSVGText(),
> > +                 "don't get WebkitTextStroke property for SVG text");
> > +    return mFrame->StyleContext()->GetTextStrokeColor();
> > +  }
> > +  float GetWebkitTextStrokeWidth () {
> 
> there's a stray space before the ()

Okay.

> @@ +5385,5 @@
> >    }
> >  
> > +  // text-stroke overflows
> > +  float textStrokeWidth = StyleText()->mWebkitTextStrokeWidth.GetCoordValue();
> > +  if (textStrokeWidth > 0.0f) {
> 
> shouldn't |textStrokeWidth| here be an |nscoord| (integer) value?
> 
> ::: layout/style/nsStyleStruct.h
> @@ +2034,5 @@
> >      return !mTextEmphasisStyleString.IsEmpty();
> >    }
> >  
> > +  bool HasWebkitTextStroke() const {
> > +    return mWebkitTextStrokeWidth.GetCoordValue() > 0.0f;
> 
> GetCoordValue() returns an nscoord, which is an integer -- so you don't want
> to force the comparison to be floating-point...

You're right. Fix this in the next version.
Hi jwatt, according to the first paragraph of Comment 41, there is something we would like to hear from you. In [1], it seems that these codes were written for SVG painting at the beginning, however, the present SVG painting code flow would never reach here. Do you think it's a good idea to purge these codes?


[1] https://dxr.mozilla.org/mozilla-central/rev/1da1937a9e03154ae7c60089f2dcf5ad9ee20fa3/gfx/thebes/gfxFont.cpp#1714-1724
Flags: needinfo?(jwatt)
Comment on attachment 8741367 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v4,carry f+:xidorn)

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

Don't forget to add these new properties to property_database.js.  (Maybe we plan to do this in a later patch on this bug.)

::: layout/style/Declaration.cpp
@@ +1306,5 @@
> +        data->ValueFor(eCSSProperty__webkit_text_stroke_color);
> +      bool isDefaultColor = strokeColor->GetUnit() == eCSSUnit_EnumColor &&
> +        strokeColor->GetIntValue() == NS_COLOR_CURRENTCOLOR;
> +
> +      if ((strokeWidth->GetUnit() != eCSSUnit_Integer &&

Should the "&&" be an "||"?

::: layout/style/StyleAnimationValue.cpp
@@ +3332,5 @@
>  
> +        case eCSSProperty__webkit_text_stroke_color: {
> +          auto styleText = static_cast<const nsStyleText*>(styleStruct);
> +          nscolor color = styleText->mWebkitTextStrokeColorForeground ?
> +            aStyleContext->StyleColor()->mColor : styleText->mWebkitTextStrokeColor;

Please keep to 80 columns.

::: layout/style/nsCSSParser.cpp
@@ +10805,5 @@
> +    return false;
> +  }
> +
> +  if (!(found & 1)) { // Provide default -webkit-text-stroke-width
> +    values[0].SetInitialValue();

Rather than SetInitialValue(), the -webkit-text-stroke shorthand should set the actual initial value for -webkit-text-stroke-width, which is 0, when it is omitted.  Similarly for -webkit-text-stroke-color (currentcolor).

::: layout/style/nsCSSPropList.h
@@ +3440,5 @@
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER,
> +    "layout.css.prefixes.webkit",
> +    VARIANT_HKL,
> +    kWebkitTextStrokeWidthKTable,

We can just use kBorderWidthKTable here.

::: layout/style/nsCSSProps.h
@@ +835,5 @@
>    static const KTableEntry kUserSelectKTable[];
>    static const KTableEntry kVerticalAlignKTable[];
>    static const KTableEntry kVisibilityKTable[];
>    static const KTableEntry kVolumeKTable[];
> +  static const KTableEntry kWebkitTextStrokeWidthKTable[];

This can be dropped if we use kBorderWidthKTable.

::: layout/style/nsRuleNode.cpp
@@ +4718,5 @@
>    }
>  
> +  // -webkit-text-stroke-color: color, string, inherit, initial
> +  const nsCSSValue*
> +    webkitTextStrokeColorValue = aRuleData->ValueForWebkitTextStrokeColor();

Nit: in my opinion, it would be better to wrap this like:

  const nsCSSValue* webkitTextStrokeColorValue =
    aRuleData->ValueForWebkitTextStrokeColor();

@@ +4742,5 @@
> +  // -webkit-text-stroke-width: length, inherit, initial, enum
> +  const nsCSSValue*
> +    webkitTextStrokeWidthValue = aRuleData->ValueForWebkitTextStrokeWidth();
> +  if (webkitTextStrokeWidthValue->GetUnit() == eCSSUnit_Enumerated) {
> +    switch(webkitTextStrokeWidthValue->GetIntValue()) {

Space before "(".

@@ +4744,5 @@
> +    webkitTextStrokeWidthValue = aRuleData->ValueForWebkitTextStrokeWidth();
> +  if (webkitTextStrokeWidthValue->GetUnit() == eCSSUnit_Enumerated) {
> +    switch(webkitTextStrokeWidthValue->GetIntValue()) {
> +      case NS_STYLE_WEBKIT_TEXT_STROKE_WIDTH_THIN:
> +        text->mWebkitTextStrokeWidth.SetCoordValue(nsPresContext::CSSPixelsToAppUnits(1));

For consistency with how we deal with border-*-width, please call mPresContext->GetBorderWidthTable() and use it to map the thin/medium/thick values.

@@ +4759,5 @@
> +    }
> +  } else {
> +    SetCoord(*webkitTextStrokeWidthValue, text->mWebkitTextStrokeWidth,
> +             parentText->mWebkitTextStrokeWidth,
> +             SETCOORD_LH | SETCOORD_UNSET_INHERIT | SETCOORD_INITIAL_ZERO,

Is there a reason not to support calc() in -webkit-text-stroke-width?  Per the spec, this should be supported.

::: layout/style/nsStyleConsts.h
@@ +1120,5 @@
>  #define NS_STYLE_TEXT_RENDERING_OPTIMIZESPEED      1
>  #define NS_STYLE_TEXT_RENDERING_OPTIMIZELEGIBILITY 2
>  #define NS_STYLE_TEXT_RENDERING_GEOMETRICPRECISION 3
>  
> +// Possible enumerated specified values of -webkit-text-stroke-width

These can be dropped if we use the NS_STYLE_BORDER_WIDTH_* values.

::: layout/style/nsStyleStruct.cpp
@@ +3719,5 @@
>      return hint;
>    }
>  
> +  if (mWebkitTextStrokeWidth != aOther.mWebkitTextStrokeWidth) {
> +    NS_UpdateHint(hint, nsChangeHint_UpdateSubtreeOverflow);

-webkit-text-stroke seems similar to text-shadow, in terms of which frames paint it (the nsTextFrames) and how it affects overflow.

So I think we need nsChangeHint_SchedulePaint | nsChangeHint_RepaintFrame too.  It might be that our overflow doesn't change with the new stroke width value (maybe we already have a big overflow area due to text-decoration, for example, and the change in text stroke width doesn't make the overflow area any different), but we'll still need to invalidate and repaint.

If this is correct, then just merge this up into the previous if-statement.

::: layout/style/nsStyleStruct.h
@@ +1965,5 @@
>    bool mTextAlignTrue : 1;              // [inherited] see nsStyleConsts.h
>    bool mTextAlignLastTrue : 1;          // [inherited] see nsStyleConsts.h
>    bool mTextEmphasisColorForeground : 1;// [inherited] whether text-emphasis-color is currentColor
>    bool mWebkitTextFillColorForeground : 1;    // [inherited] whether -webkit-text-fill-color is currentColor
> +  bool mWebkitTextStrokeColorForeground : 1;    // [inherited] whether -webkit-text-stroke-color is currentColor

Nit: may as well keep the comment aligned with the previous line.
Attachment #8741367 - Flags: review?(cam)
Comment hidden (obsolete)
Attachment #8741367 - Attachment is obsolete: true
Attachment #8742737 - Flags: review?(cam)
Created attachment 8743141 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v5,carry f+:xidorn)

Address heycam's comment.
Use existing NS_STYLE_BORDER_WIDTH_* values.
Patch property_database.js
Take advantage of GetTextStrokeColor().
Attachment #8742737 - Attachment is obsolete: true
Attachment #8742737 - Flags: review?(cam)
Attachment #8743141 - Flags: review?(cam)
Created attachment 8743152 [details] [diff] [review]
Part3: add reftests.

Use SVG text as reference to make reftests.
Attachment #8743152 - Flags: review?(jfkthame)
Comment on attachment 8743141 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v5,carry f+:xidorn)

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

Looking good.  One question below.

::: layout/style/Declaration.cpp
@@ +1311,5 @@
> +      bool isDefaultColor = strokeColor->GetUnit() == eCSSUnit_EnumColor &&
> +        strokeColor->GetIntValue() == NS_COLOR_CURRENTCOLOR;
> +
> +      if ((strokeWidth->GetUnit() != eCSSUnit_Integer ||
> +           strokeWidth->GetIntValue() != 0) || isDefaultColor) {

Nit: remove the parentheses around the two strokeWidth tests.

::: layout/style/test/property_database.js
@@ +7041,5 @@
> +    prerequisites: { "color": "black" },
> +    subproperties: [ "-webkit-text-stroke-width", "-webkit-text-stroke-color" ],
> +    initial_values: [ "0 currentColor", "currentColor 0px", "0", "currentColor", "0px black" ],
> +    other_values: [ "thin black", "#f00 medium", "thick rgba(0,0,255,0.5)", "2px", "green 0", "currentColor 4em", "currentColor calc(5px - 1px)" ],
> +    invalid_values: [ "-3px black", "calc(2px - 5px) red", "calc(50%+ 2px) #000", "30% #f00" ]

Is "calc(2px - 5px) red" really an invalid value?  I think the calc() should end up clamping to 0.  Does layout/style/test/test_property_syntax_errors.html (which should check this) pass?
(In reply to Cameron McCormack (:heycam) from comment #47)
> Comment on attachment 8743141 [details] [diff] [review]
> Part1: parse and compute -webkit-text-stroke property. (v5,carry f+:xidorn)
> 
> Review of attachment 8743141 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good.  One question below.
> 
> ::: layout/style/Declaration.cpp
> @@ +1311,5 @@
> > +      bool isDefaultColor = strokeColor->GetUnit() == eCSSUnit_EnumColor &&
> > +        strokeColor->GetIntValue() == NS_COLOR_CURRENTCOLOR;
> > +
> > +      if ((strokeWidth->GetUnit() != eCSSUnit_Integer ||
> > +           strokeWidth->GetIntValue() != 0) || isDefaultColor) {
> 
> Nit: remove the parentheses around the two strokeWidth tests.

Will do.

> ::: layout/style/test/property_database.js
> @@ +7041,5 @@
> > +    prerequisites: { "color": "black" },
> > +    subproperties: [ "-webkit-text-stroke-width", "-webkit-text-stroke-color" ],
> > +    initial_values: [ "0 currentColor", "currentColor 0px", "0", "currentColor", "0px black" ],
> > +    other_values: [ "thin black", "#f00 medium", "thick rgba(0,0,255,0.5)", "2px", "green 0", "currentColor 4em", "currentColor calc(5px - 1px)" ],
> > +    invalid_values: [ "-3px black", "calc(2px - 5px) red", "calc(50%+ 2px) #000", "30% #f00" ]
> 
> Is "calc(2px - 5px) red" really an invalid value?  I think the calc() should
> end up clamping to 0.  Does
> layout/style/test/test_property_syntax_errors.html (which should check this)
> pass?

Yes, it passed on my local machine.
Are you suggesting that I should set SETCOORD_CALC_CLAMP_NONNEGATIVE for webkit-text-stroke-width as well?
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #48)
> Yes, it passed on my local machine.

So, without SETCOORD_CALC_CLAMP_NONNEGATIVE I would expect that value to be valid, and for test_property_syntax_errors.html to fail.  It's running with the webkit pref set?

I just tried applying the patch locally to test, but it looks like it's missing nsStyleContext::GetTextStrokeColor.

> Are you suggesting that I should set SETCOORD_CALC_CLAMP_NONNEGATIVE for
> webkit-text-stroke-width as well?

Yes.
(In reply to Cameron McCormack (:heycam) from comment #49)
> It's running with the webkit pref set?

I guess it should be, since it's set in profiles/prefs_general.js.
(In reply to Cameron McCormack (:heycam) from comment #49)
> I just tried applying the patch locally to test, but it looks like it's
> missing nsStyleContext::GetTextStrokeColor.

Oops! It looks like nsStyleContext::GetTextStrokeColor is patched in part2.2. :/
I shall move it to part1 since I'm planning to take advantage of it here.
May I do this in the next version?

> > Are you suggesting that I should set SETCOORD_CALC_CLAMP_NONNEGATIVE for
> > webkit-text-stroke-width as well?
> 
> Yes.

Per spec on [1], it is said that "a negative value is invalid". This is agree w/ the spec for border-width [2] as well. According to [2], it is said that "Negative <length> values are not allowed".

I think this is the reason why SETCOORD_CALC_CLAMP_NONNEGATIVE is not used for parsing border-width [3]. Thoughts?


[1] https://compat.spec.whatwg.org/#the-webkit-text-stroke-width
[2] https://drafts.csswg.org/css-backgrounds-3/#line-width
[3] https://dxr.mozilla.org/mozilla-central/rev/ae7413abfa4d3954a6a4ce7c1613a7100f367f9a/layout/style/nsRuleNode.cpp#7165
Comment on attachment 8742182 [details] [diff] [review]
Part2.1: use mfbt/TypedEnumBits.h for DrawMode. (v2,carry f+:xidorn)

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

Hmm, it looks like there's also code in layout/svg/SVGTextFrame.cpp that should be updated here; it currently casts DrawMode to/from integers in order to do bitwise twiddling.
Attachment #8742182 - Flags: review+

Updated

2 years ago
Depends on: 1266101
Comment on attachment 8742182 [details] [diff] [review]
Part2.1: use mfbt/TypedEnumBits.h for DrawMode. (v2,carry f+:xidorn)

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

In addition to addressing jfkthame's comment about SVGTextFrame, can you also address the following.

::: gfx/thebes/gfxFont.cpp
@@ +1619,5 @@
>          buf.mNumGlyphs = mNumGlyphs;
>  
>          gfxContext::AzureState state = mRunParams.context->CurrentState();
> +        if ((mRunParams.drawMode &
> +            (DrawMode::GLYPH_STROKE | DrawMode::GLYPH_STROKE_UNDERNEATH)) ==

While you're here, please indent the above line one extra space. Right now it looks like the opening parenthesis on both the above lines have equal nesting depth.

@@ +1698,5 @@
>                  buf, mRunParams.context->mPathBuilder,
>                  mRunParams.dt->GetBackendType(), &mat);
>          }
> +        if ((mRunParams.drawMode &
> +            (DrawMode::GLYPH_STROKE | DrawMode::GLYPH_STROKE_UNDERNEATH)) ==

Same thing here. One more space please.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #51)
> (In reply to Cameron McCormack (:heycam) from comment #49)
> > I just tried applying the patch locally to test, but it looks like it's
> > missing nsStyleContext::GetTextStrokeColor.
> 
> Oops! It looks like nsStyleContext::GetTextStrokeColor is patched in
> part2.2. :/
> I shall move it to part1 since I'm planning to take advantage of it here.
> May I do this in the next version?
> 
> > > Are you suggesting that I should set SETCOORD_CALC_CLAMP_NONNEGATIVE for
> > > webkit-text-stroke-width as well?
> > 
> > Yes.
> 
> Per spec on [1], it is said that "a negative value is invalid". This is
> agree w/ the spec for border-width [2] as well. According to [2], it is said
> that "Negative <length> values are not allowed".
> 
> I think this is the reason why SETCOORD_CALC_CLAMP_NONNEGATIVE is not used
> for parsing border-width [3]. Thoughts?
> 
> 
> [1] https://compat.spec.whatwg.org/#the-webkit-text-stroke-width
> [2] https://drafts.csswg.org/css-backgrounds-3/#line-width
> [3]
> https://dxr.mozilla.org/mozilla-central/rev/
> ae7413abfa4d3954a6a4ce7c1613a7100f367f9a/layout/style/nsRuleNode.cpp#7165

Looks like border-width did CLAMP_NONNEGATIVE thing just 2 lines below [3]. No idea why I miss it at the first time. Will add SETCOORD_CALC_CLAMP_NONNEGATIVE in the next version.

I've bumped into few errors while testing layout/style/test/test_transitions_per_property.html. Things do not go well during getPropertyValue/setProperty, still digging. :/
Created attachment 8743416 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v6)

Address heycam's comment.
Move nsStyleContext::GetTextStrokeColor from part2 to here.

Todo: fix fails in layout/style/test/test_transitions_per_property.html
Attachment #8743141 - Attachment is obsolete: true
Attachment #8743141 - Flags: review?(cam)
Cancelling the needinfo on me here since the question posed of me has spun off into bug 1266101.
Flags: needinfo?(jwatt)
Created attachment 8743432 [details] [diff] [review]
Part2.1: use mfbt/TypedEnumBits.h for DrawMode. (v3)

Patch layout/svg/SVGTextFrame.cpp as well.
Address jwatt's comment.
Attachment #8742182 - Attachment is obsolete: true
One question to consider here though is what needs to happen to make sure that the 'paint-order' property works with '-webikit-text-stroke'. 'paint-order' previously only applied to SVG text since it wasn't possible to stroke HTML text, but if we're adding '-webikit-text-stroke' then 'paint-order' should work with HTML text too. That may mean than some of the apparently dead code being discussed over in bug 1266101 is actually code that will now be useful. (But let's discuss that over in bug 1266101 so as not to clutter up this bug too much.)
Comment on attachment 8743432 [details] [diff] [review]
Part2.1: use mfbt/TypedEnumBits.h for DrawMode. (v3)

This seems like an independent change. I'd suggest you go ahead and land this to avoid bitrot, and add the 'leave-open' keyword to the Keywords field.
Attachment #8743432 - Flags: review+
Comment hidden (spam)
Attachment #8742183 - Attachment is obsolete: true
Attachment #8742183 - Flags: review?(jfkthame)
Comment on attachment 8743152 [details] [diff] [review]
Part3: add reftests.

try w/ test only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb5f2f22c751
try w/ test & patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65c022d61353

Looks like the patches can't pass basic functional reftests on Linux platform. I'm guessing that we might need to use fuzzy comparison on different platforms. Checking...
Attachment #8743152 - Flags: review?(jfkthame)
Created attachment 8743640 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v7,carry f+:xidorn)

Address heycam's comment.
Remove "calc(2px - 5px) red" from invalid value.
Set SETCOORD_CALC_CLAMP_NONNEGATIVE while computing.
Set VARIANT_CALC while parsing.

So far, this could pass following 2 tests on my local machine:
layout/style/test/test_property_syntax_errors.html
layout/style/test/test_transitions_per_property.html
Attachment #8743640 - Flags: review?(cam)
Attachment #8743416 - Attachment is obsolete: true
Comment on attachment 8743640 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v7,carry f+:xidorn)

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

r=me with the below, thanks.

::: layout/style/test/property_database.js
@@ +7040,5 @@
> +    type: CSS_TYPE_TRUE_SHORTHAND,
> +    prerequisites: { "color": "black" },
> +    subproperties: [ "-webkit-text-stroke-width", "-webkit-text-stroke-color" ],
> +    initial_values: [ "0 currentColor", "currentColor 0px", "0", "currentColor", "0px black" ],
> +    other_values: [ "thin black", "#f00 medium", "thick rgba(0,0,255,0.5)", "2px", "green 0", "currentColor 4em", "currentColor calc(5px - 1px)" ],

Include a calc() that gets clamped in here, e.g. "calc(4px - 8px) green".

@@ +7056,5 @@
> +  gCSSProperties["-webkit-text-stroke-width"] = {
> +    domProp: "webkitTextStrokeWidth",
> +    inherited: true,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "0", "0px", "0em", "0ex", "calc(0pt)" ],

Include a calc() that gets clamped in here, e.g. "calc(4px - 8px)".
Attachment #8743640 - Flags: review?(cam) → review+
Created attachment 8743681 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v8,carry f+:xidorn r+:heycam)

Add calc() that gets clamped into invalid value property tests.

heycam, thank you for the review. :)
Attachment #8743681 - Flags: review+
Attachment #8743640 - Attachment is obsolete: true
Created attachment 8743694 [details] [diff] [review]
Part3: add reftests. (v2)

The fuzzy condition is set based on try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d47e85f2596e&selectedJob=19765105
Attachment #8743694 - Flags: review?(jfkthame)
Attachment #8743152 - Attachment is obsolete: true
Created attachment 8743721 [details] [diff] [review]
Part3: add reftests. (v2)

fix nit in reftest.list
Attachment #8743721 - Flags: review?(jfkthame)
Attachment #8743694 - Attachment is obsolete: true
Attachment #8743694 - Flags: review?(jfkthame)
Created attachment 8743861 [details] [diff] [review]
Part2.2: render -webkit-text-stroke property. (v5,carry f+:xidorn)

Address jfkthame's comment.
Replace assertion w/ early return in textPaintStyle::GetTextStroke*().
Move nsStyleContext::GetTextStrokeColor to Part1 so we could take advantage of it there.
Move non-SVG text stroke draw code into if-else-statement.
Attachment #8743435 - Attachment is obsolete: true
Hi jfktham, I've moved non-SVG text stroke's draw call into if-else-statement, so the double-stroke is avoided. Per this change, I think we could make this bug be more independent from Bug 1266101. Do you think we could re-start the review process?
Flags: needinfo?(jfkthame)
Keywords: leave-open
BTW it's worth noting that the presence of 'contextPaint' does not mean that we're painting SVG text elements. See:

https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp?rev=77ba0dcb977a&mark=2050-2053#2050

where we set a contextPaint for any text that happens to be painting with a font that contains SVG glyphs.
Comment on attachment 8743861 [details] [diff] [review]
Part2.2: render -webkit-text-stroke property. (v5,carry f+:xidorn)

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

::: gfx/thebes/gfxFont.cpp
@@ +1712,2 @@
>          if (mFontParams.contextPaint) {
> +          // XXX: Do we still need this if-statement? Not sure if this is for SVG.

Which to be clear should help answer the question here.

@@ +1725,5 @@
> +                                    *strokePattern->GetPattern(mRunParams.dt),
> +                                    aState.strokeOptions);
> +          }
> +        } else {
> +          // non-SVG text stroke

And this comment would be misleading.
Cameron, are you able to help decide how best to take account of the part 2 patch in bug 1266101? You understand the text rendering code better than I do, and I don't have time to dig into this just now.
Flags: needinfo?(cam)
Sorry, I'm not really sure what you're asking here.  Can we worry about part 2 of bug 1266101 landing later?  What are the concerns in this bug?
Flags: needinfo?(cam)
jwatt clarified on IRC that this is about the patches here changing the code around GLYPH_STROKE etc. when those draw modes aren't used at all currently.  I just r+ed the part 2 patch in bug 1266101 so we don't have to worry about that here.
FWIW, with bug 1260543 lands, you'll need to update your part 1 on the following places:
* StyleAnimationValue.cpp: use SetCurrentOrActualColor like what is done for -webkit-text-fill-color
* test_transitions_events.html: remove all code you added here
* test_transitions_per_property.html: make -webkit-text-stroke-color only test test_color_transition
Comment on attachment 8743681 [details] [diff] [review]
Part1: parse and compute -webkit-text-stroke property. (v8,carry f+:xidorn r+:heycam)

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

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

The only fail so far is layout/style/test/test_value_computation.html w/ error msg:

INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_computation.html | should get initial value for '-webkit-text-stroke:currentColor' - got "1px ; rgb(0, 0, 0)", expected "0px ; rgb(0, 0, 0)"

Looks like values[0].SetIntValue(0, eCSSUnit_Integer) would confuse us when getProperty from shorthand. It seems "thin" keyword is computed instead of 0. After replacing this line by values[0].SetFloatValue(0, eCSSUnit_Pixel), it passed on my local machine. heycam, do you think this is the right fix?

::: layout/style/nsCSSParser.cpp
@@ +10812,5 @@
> +    return false;
> +  }
> +
> +  if (!(found & 1)) { // Provide default -webkit-text-stroke-width
> +    values[0].SetIntValue(0, eCSSUnit_Integer);

Should I replace this line by values[0].SetFloatValue(0, eCSSUnit_Pixel);?
Attachment #8743681 - Flags: feedback?(cam)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #75)
> FWIW, with bug 1260543 lands, you'll need to update your part 1 on the
> following places:
> * StyleAnimationValue.cpp: use SetCurrentOrActualColor like what is done for
> -webkit-text-fill-color
> * test_transitions_events.html: remove all code you added here
> * test_transitions_per_property.html: make -webkit-text-stroke-color only
> test test_color_transition

Thank you for telling me this. Will do this change in the next version.
(In reply to Jonathan Watt [:jwatt] from comment #71)
> Comment on attachment 8743861 [details] [diff] [review]
> Part2.2: render -webkit-text-stroke property. (v5,carry f+:xidorn)
> 
> Review of attachment 8743861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFont.cpp
> @@ +1712,2 @@
> >          if (mFontParams.contextPaint) {
> > +          // XXX: Do we still need this if-statement? Not sure if this is for SVG.
> 
> Which to be clear should help answer the question here.
> 
> @@ +1725,5 @@
> > +                                    *strokePattern->GetPattern(mRunParams.dt),
> > +                                    aState.strokeOptions);
> > +          }
> > +        } else {
> > +          // non-SVG text stroke
> 
> And this comment would be misleading.

Thank you for the comments. Per IRC w/ you, I'll wait for bug 1266101 to be resolved first.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #78)
> Thank you for the comments. Per IRC w/ you, I'll wait for bug 1266101 to be
> resolved first.

You don't need to wait. You just need to remove the changes you made to gfxFont.cpp in your patch, that's all. (Since that code is being removed by bug 1266101.)
Comment hidden (spam)
Attachment #8743861 - Attachment is obsolete: true
Attachment #8744292 - Flags: feedback?(jwatt)
Attachment #8744292 - Attachment is obsolete: true
Comment on attachment 8743721 [details] [diff] [review]
Part3: add reftests. (v2)

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

::: layout/reftests/text-stroke/reftest.list
@@ +3,5 @@
> +fuzzy(255,20) == webkit-text-stroke-property-001.html webkit-text-stroke-property-001-ref.html
> +fuzzy(255,20) == webkit-text-stroke-property-002.html webkit-text-stroke-property-002-ref.html
> +fuzzy(255,20) == webkit-text-stroke-property-003.html webkit-text-stroke-property-003-ref.html
> +fuzzy(255,20) == webkit-text-stroke-property-004.html webkit-text-stroke-property-004-ref.html
> +fuzzy(255,20) == webkit-text-stroke-property-005.html webkit-text-stroke-property-005-ref.html

From the try run, it looks like these passed cleanly on OS X, and with only a tiny bit of antialiasing fuzz on Windows. On Linux, OTOH, there's a real discrepancy in how the stroke is being rendered -- see the junction of the vertical stroke and the arch of the "r" glyph. It appears that the SVG stroke is respecting a mitre limit at the join, whereas the stroking of the HTML text doesn't have that feature and so the sharp angle becomes a long point.

Given all this, I'd like to be more specific about the annotations here; and in addition, I think we should consider a followup to improve the stroking of HTML text by implementing a mitre limit, similar to what the SVG code shows on Linux. (I'm curious whether this is actually consistent across platforms: it's hard to tell from this testcase because it ends up using different, platform-specific fonts.)

So let's annotate these with

  fuzzy-if(gtkWidget, ...) fuzzy-if(winWidget, ...)

(don't bother about the differences between Windows versions, though, just make it fuzzy enough for the worst version); and let's have non-fuzzy matches on OS X.

And please file a followup bug to investigate the question of the mitre limit at sharp joins when stroking glyphs. Is our behavior consistent across platforms for a given glyph shape? (Does it depend on the gfx backend being used, skia vs cairo?) And can we make the HTML glyph-stroking respect a mitre limit?
Created attachment 8744335 [details] [diff] [review]
Part2.2: render -webkit-text-stroke property. (v6, carry f+:xidorn)

Rebase from bug 1266101.
Attachment #8744335 - Flags: review?(jfkthame)
Comment on attachment 8744335 [details] [diff] [review]
Part2.2: render -webkit-text-stroke property. (v6, carry f+:xidorn)

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

Looks good, thanks.

Please file a follow-up bug about possibly extending this to support paint-order, but we don't need that for the initial landing.
Attachment #8744335 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #81)
> Comment on attachment 8743721 [details] [diff] [review]
> Part3: add reftests. (v2)
> 
> Review of attachment 8743721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/text-stroke/reftest.list
> @@ +3,5 @@
> > +fuzzy(255,20) == webkit-text-stroke-property-001.html webkit-text-stroke-property-001-ref.html
> > +fuzzy(255,20) == webkit-text-stroke-property-002.html webkit-text-stroke-property-002-ref.html
> > +fuzzy(255,20) == webkit-text-stroke-property-003.html webkit-text-stroke-property-003-ref.html
> > +fuzzy(255,20) == webkit-text-stroke-property-004.html webkit-text-stroke-property-004-ref.html
> > +fuzzy(255,20) == webkit-text-stroke-property-005.html webkit-text-stroke-property-005-ref.html
> 
> From the try run, it looks like these passed cleanly on OS X, and with only
> a tiny bit of antialiasing fuzz on Windows. On Linux, OTOH, there's a real
> discrepancy in how the stroke is being rendered -- see the junction of the
> vertical stroke and the arch of the "r" glyph. It appears that the SVG
> stroke is respecting a mitre limit at the join, whereas the stroking of the
> HTML text doesn't have that feature and so the sharp angle becomes a long
> point.
> 
> Given all this, I'd like to be more specific about the annotations here; and
> in addition, I think we should consider a followup to improve the stroking
> of HTML text by implementing a mitre limit, similar to what the SVG code
> shows on Linux. (I'm curious whether this is actually consistent across
> platforms: it's hard to tell from this testcase because it ends up using
> different, platform-specific fonts.)
> 
> So let's annotate these with
> 
>   fuzzy-if(gtkWidget, ...) fuzzy-if(winWidget, ...)
> 
> (don't bother about the differences between Windows versions, though, just
> make it fuzzy enough for the worst version); and let's have non-fuzzy
> matches on OS X.

Will do.

> And please file a followup bug to investigate the question of the mitre
> limit at sharp joins when stroking glyphs. Is our behavior consistent across
> platforms for a given glyph shape? (Does it depend on the gfx backend being
> used, skia vs cairo?) And can we make the HTML glyph-stroking respect a
> mitre limit?

Filed Bug 1266743 for this. Thank you for the review.
(In reply to Jonathan Kew (:jfkthame) from comment #84)
> Comment on attachment 8744335 [details] [diff] [review]
> Part2.2: render -webkit-text-stroke property. (v6, carry f+:xidorn)
> 
> Review of attachment 8744335 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks.
> 
> Please file a follow-up bug about possibly extending this to support
> paint-order, but we don't need that for the initial landing.

Filed Bug 1266752 for this. Thank you for the review. :)
Created attachment 8744350 [details] [diff] [review]
Part3: add reftests. (v3)

Address reviewer's comment.
Attachment #8744350 - Flags: review?(jfkthame)
Attachment #8743721 - Attachment is obsolete: true
Attachment #8743721 - Flags: review?(jfkthame)
(In reply to Jonathan Watt [:jwatt] from comment #79)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #78)
> > Thank you for the comments. Per IRC w/ you, I'll wait for bug 1266101 to be
> > resolved first.
> 
> You don't need to wait. You just need to remove the changes you made to
> gfxFont.cpp in your patch, that's all. (Since that code is being removed by
> bug 1266101.)

jwatt, very appreciate your help. Thank you.
Comment on attachment 8744350 [details] [diff] [review]
Part3: add reftests. (v3)

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

Thanks, let's hope they stay green! :)
Attachment #8744350 - Flags: review?(jfkthame) → review+
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #90)
> Comment on attachment 8744350 [details] [diff] [review]
> Part3: add reftests. (v3)
> 
> Review of attachment 8744350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, let's hope they stay green! :)

Thank you for the review. :)
Attachment #8743681 - Flags: feedback?(cam)
Keywords: leave-open

Comment 94

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28b1a8408b86
https://hg.mozilla.org/mozilla-central/rev/500cb83c3626
https://hg.mozilla.org/mozilla-central/rev/5f3ba1d0bbf4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Comment 95

2 years ago
This broke compilation with VS2013:

c:/t1/hg/comm-central/mozilla/layout/style/nsCSSParser.cpp(10816) : error C2057: expected constant expression
c:/t1/hg/comm-central/mozilla/layout/style/nsCSSParser.cpp(10816) : error C2466: cannot allocate an array of constant size 0
c:/t1/hg/comm-central/mozilla/layout/style/nsCSSParser.cpp(10816) : error C2133: 'values' : unknown size
Those errors all trace back to these two lines:
  const size_t numProps = ArrayLength(kWebkitTextStrokeIDs);
  nsCSSValue values[numProps];

I seem to recall that this might really have to use "MOZ_ARRAY_LENGTH" instead of "ArrayLength" for it to be usable at compile-time for the sizing of this array (in some compilers at least).

Philip, if you've got MSVC 2013 locally, could you see if that change fixes it?
(See also bug 1265154, where CJ hit the same issue recently in a static_assert expression.  Switching to MOZ_ARRAY_LENGTH fixed the problem there.)
Yeah, it's gotta fix it actually, because we have this same pattern elsewhere in this file, but using MOZ_ARRAY_LENGTH, and those other instances of the pattern aren't triggering this MSVC issue.

I'll push a trivial followup right now, so that this is buildable again soon for people with MSVC2013.

Updated

2 years ago
Blocks: 1267296

Updated

2 years ago
No longer blocks: 1267296
Duplicate of this bug: 1267296
Keywords: dev-doc-needed
(In reply to Daniel Holbert [:dholbert] from comment #98)
> Yeah, it's gotta fix it actually, because we have this same pattern
> elsewhere in this file, but using MOZ_ARRAY_LENGTH, and those other
> instances of the pattern aren't triggering this MSVC issue.
> 
> I'll push a trivial followup right now, so that this is buildable again soon
> for people with MSVC2013.

dholbert, thank you for taking care of this.
Depends on: 1275093
I've updated the docs according mentioned in comment 103 according to bug 1259345.

Sebastian
Depends on: 1426092
You need to log in before you can comment on or make changes to this bug.