Bug 169273 – Implement stroke-width CSS property.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

Implement stroke-width, see https://drafts.fxtf.org/paint/.

[Per Arne Vollan]

Created attachment 303645 [details]
Patch

[WebKit Commit Bot]

Attachment 303645 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 303649 [details]
Patch

[Per Arne Vollan]

Created attachment 303652 [details]
Patch

[Per Arne Vollan]

Created attachment 303660 [details]
Patch

[Per Arne Vollan]

rdar://problem/30754819

[Brent Fulgham]

Comment on attachment 303660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303660&action=review

I think Simon should do the full review of this, but it seems reasonable.

I do think that "textStrokeWidht()" might be able to encapsulate the "explicitly set stroke width" case, at least from the examples in this patch. Perhaps there are bigger picture issues that make it better to handle the two cases separately, but it seems like a lot of duplicated checks in multiple places.

> Source/WebCore/rendering/InlineFlowBox.cpp:172
> +            if (childStyle->letterSpacing() < 0 || childStyle->textShadow() || childStyle->textEmphasisMark() != TextEmphasisMarkNone || childStyle->textStrokeWidth() || (childStyle->hasExplicitlySetStrokeWidth() && childStyle->strokeWidth().value()))

You and Simon have probably already hashed all of this out, but reading this code makes me wonder why "childStyle->textStrokeWidth()" couldn't handle the explicit stroke width case internally. I guess there might be cases where we want the textStrokeWidth even if we have an explicitly set stroke width?

> Source/WebCore/rendering/InlineFlowBox.cpp:908
> +    int strokeOverflow = static_cast<int>(ceilf(lineStyle.computedTextStrokeWidth(viewport) / 2.0f));

I think this should be "std::ceil" so it will shift type if we ever modify lineStyle's return types.

[Simon Fraser (smfr)]

Comment on attachment 303660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303660&action=review

>> Source/WebCore/rendering/InlineFlowBox.cpp:172
>> +            if (childStyle->letterSpacing() < 0 || childStyle->textShadow() || childStyle->textEmphasisMark() != TextEmphasisMarkNone || childStyle->textStrokeWidth() || (childStyle->hasExplicitlySetStrokeWidth() && childStyle->strokeWidth().value()))
> 
> You and Simon have probably already hashed all of this out, but reading this code makes me wonder why "childStyle->textStrokeWidth()" couldn't handle the explicit stroke width case internally. I guess there might be cases where we want the textStrokeWidth even if we have an explicitly set stroke width?

I don't know why we need hasExplicitlySetStrokeWidth() and the changelog is sorely lacking in explanation.

>> Source/WebCore/rendering/InlineFlowBox.cpp:908
>> +    int strokeOverflow = static_cast<int>(ceilf(lineStyle.computedTextStrokeWidth(viewport) / 2.0f));
> 
> I think this should be "std::ceil" so it will shift type if we ever modify lineStyle's return types.

Just adding half stroke width here seems naive, and doesn't take into account miters.

> Source/WebCore/rendering/style/RenderStyle.h:1920
> +        unsigned hasSetStrokeWidth : 1;

I don't think this should be here; it should be in StyleRareInheritedData.

[Per Arne Vollan]

Created attachment 303797 [details]
Patch

[Per Arne Vollan]

(In reply to comment #8)
> Comment on attachment 303660 [details]
> Patch
> 

Thanks for reviewing!

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303660&action=review
> 
> >> Source/WebCore/rendering/InlineFlowBox.cpp:172
> >> +            if (childStyle->letterSpacing() < 0 || childStyle->textShadow() || childStyle->textEmphasisMark() != TextEmphasisMarkNone || childStyle->textStrokeWidth() || (childStyle->hasExplicitlySetStrokeWidth() && childStyle->strokeWidth().value()))
> > 
> > You and Simon have probably already hashed all of this out, but reading this code makes me wonder why "childStyle->textStrokeWidth()" couldn't handle the explicit stroke width case internally. I guess there might be cases where we want the textStrokeWidth even if we have an explicitly set stroke width?
> 

Added a new member to simplify this in the last patch.

> I don't know why we need hasExplicitlySetStrokeWidth() and the changelog is
> sorely lacking in explanation.
> 

I added hasExplicitlySetStrokeWidth() to fall back to the -webkit-text-stroke-width value when stroke-width is not set, to make sure the -webkit-text-stroke-width property still works.

Currently, a float member is holding the value of the -webkit-text-stroke-width value, and a Length member is holding the value of the stroke-width property. Would a better solution be to let both properties use the same Length member?

> >> Source/WebCore/rendering/InlineFlowBox.cpp:908
> >> +    int strokeOverflow = static_cast<int>(ceilf(lineStyle.computedTextStrokeWidth(viewport) / 2.0f));
> > 
> > I think this should be "std::ceil" so it will shift type if we ever modify lineStyle's return types.
> 

Fixed.

> Just adding half stroke width here seems naive, and doesn't take into
> account miters.
> 

Do you think we should involve the stroke-miterlimit property here  (https://bugs.webkit.org/show_bug.cgi?id=169078)?

[Simon Fraser (smfr)]

Comment on attachment 303797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303797&action=review

r+ but please check SVG rendering.

> Source/WebCore/rendering/style/RenderStyle.cpp:2267
> +        ExceptionOr<float> result = length.value() * (viewportSize.width() + viewportSize.height()) / 200.0f;

Please add a comment here referencing the "Percentages: relative to the scaled viewport size, scaled viewport size is the geometric mean of the viewport width and height" part of the spec, otherwise this code is really confusing to see.

Does this code work when stroke-width is used inside SVG?

> Source/WebCore/rendering/style/StyleRareInheritedData.h:143
> +    Length strokeWidth;    
> +    unsigned hasSetStrokeWidth : 1;

You may get better packing by putting the bitfield before the Length

[Per Arne Vollan]

(In reply to comment #11)
> Comment on attachment 303797 [details]
> Patch
> 

Thanks for reviewing :)

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303797&action=review
> 
> r+ but please check SVG rendering.
> 

A quick test shows that rendering is correct when using stroke-width in SVG.


> > Source/WebCore/rendering/style/RenderStyle.cpp:2267
> > +        ExceptionOr<float> result = length.value() * (viewportSize.width() + viewportSize.height()) / 200.0f;
> 
> Please add a comment here referencing the "Percentages: relative to the
> scaled viewport size, scaled viewport size is the geometric mean of the
> viewport width and height" part of the spec, otherwise this code is really
> confusing to see.
> 
> Does this code work when stroke-width is used inside SVG?

I believe this code path is not used by SVG.

[Per Arne Vollan]

Committed <https://trac.webkit.org/changeset/213634>.

[Jon Lee]

Per, could you add tests that show expected fallback behavior between stroke-width and -webkit-text-stroke-width?

[Per Arne Vollan]

(In reply to comment #14)
> Per, could you add tests that show expected fallback behavior between
> stroke-width and -webkit-text-stroke-width?

Yes :)

[Myles C. Maxfield]

Comment on attachment 303797 [details]
Patch

I don't see anywhere where you inflate repaint rects to account of stroke-widths. Is this not a problem?

[Per Arne Vollan]

(In reply to Myles C. Maxfield from comment #16)
> Comment on attachment 303797 [details]
> Patch
> 
> I don't see anywhere where you inflate repaint rects to account of
> stroke-widths. Is this not a problem?

I am not aware of any problems related to that. I might be mistaken, but I think this is handled by the computation of the 'strokeOverflow' variable in the patch.