userSpaceOnUse patterns are not stroked for empty object bounding box elements

• Component: SVG
• Status: RESOLVED FIXED

[Florin Malita]

Created attachment 188199 [details]
Should display a green rect - fails on ToT.

The spec is somewhat confusing when it comes to patternUnits == 'objectBoundingBox' vs. elements with an empty object bounding box (see https://bugs.webkit.org/show_bug.cgi?id=109665).

But for patternUnits == 'userSpaceOnUse', the whole empty object bounding box logic is not applicable.

We have a bug where elements with an empty OBB are not pattern-stroked on the first paint:

    if (m_attributes.patternUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX && objectBoundingBox.isEmpty())
        return false;

Unfortunately, we are making this decision before updating patternUnits from the element, so we're always seeing SVG_UNIT_TYPE_OBJECTBOUNDINGBOX on the first pass! Later, if a second element gets painted, the pattern attributes get collected and patternUnits is updated to the specified SVG_UNIT_TYPE_USERSPACEONUSE. Then on subsequent repaints the empty bounding box logic no longer kicks in because attributes are up to date.

[Florin Malita]

Created attachment 188202 [details]
Patch

[Philip Rogers]

Comment on attachment 188202 [details]
Patch

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

We will also need to make this same change in RenderSVGResourceGradient, and possible elsewhere as well.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:78
> +    FloatRect objectBoundingBox = object->objectBoundingBox();

Is this local variable needed?

[Florin Malita]

(In reply to comment #2)
> (From update of attachment 188202 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188202&action=review
> 
> We will also need to make this same change in RenderSVGResourceGradient, and possible elsewhere as well.

Gradients are OK and probably all other resource renderers. This was introduced recently, most likely by a change by yours truly :)

> 
> > Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:78
> > +    FloatRect objectBoundingBox = object->objectBoundingBox();
> 
> Is this local variable needed?

Nope, we can fold.

[Florin Malita]

Created attachment 188396 [details]
Patch

Updated per comments.

[Brent Fulgham]

Comment on attachment 188396 [details]
Patch

This patch no longer applies cleanly. For that reason, r-.

[Said Abou-Hallawa]

The attached test case is confusing. It does not display on any browser. The test also includes patternUnits='userSpaceOnUse' while the bug is about patternUnits == 'objectBoundingBox'. It also include mask which is not related to this bug.

[Said Abou-Hallawa]

Created attachment 274697 [details]
test case

[Said Abou-Hallawa]

The attaches "test case" does not display correctly in WebKit but it does display correctly in FireFox and Chrome.

[Said Abou-Hallawa]

Created attachment 274705 [details]
Patch

[Said Abou-Hallawa]

I prefer to call collectPatternAttributes() from RenderSVGResourcePattern::applyResource() instead of leaving it in RenderSVGResourcePattern::buildPattern() because I think it is hard to track this side effect of buildPattern(). The function name does not say it is also ensuring the pattern attributes are collected.

applyResource() is the only caller to buildPattern(). Also m_attributes is only used by applyResource() and buildPattern().

[Build Bot]

Comment on attachment 274705 [details]
Patch

Attachment 274705 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1023244

New failing tests:
svg/custom/pattern-units-fill-stroke.svg

[Build Bot]

Created attachment 274715 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 274716 [details]
Patch

[Brent Fulgham]

Comment on attachment 274716 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 274716 [details]
Patch

Clearing flags on attachment: 274716

Committed r198574: <http://trac.webkit.org/changeset/198574>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.