Bug 139294 – [SVG Masking] Enable the use of <mask> elements for mask-image

• Component: CSS
• Status: RESOLVED DUPLICATE

[Radu Stavila]

Because the patch in https://bugs.webkit.org/show_bug.cgi?id=129682 is quite large, I will break it into two smaller patches. This is the second one and it links together the existing code with the new masking functionality added in the first part (https://bugs.webkit.org/show_bug.cgi?id=139092).

[Radu Stavila]

Created attachment 242628 [details]
Patch

[Radu Stavila]

Created attachment 242632 [details]
Patch

[Radu Stavila]

Created attachment 242666 [details]
Patch

[Radu Stavila]

Created attachment 242843 [details]
Patch

[Radu Stavila]

Created https://bugs.webkit.org/show_bug.cgi?id=139440 and https://bugs.webkit.org/show_bug.cgi?id=139442 for further improvements.

[Simon Fraser (smfr)]

Comment on attachment 242843 [details]
Patch

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

> LayoutTests/css3/masking/mask-multiple-values-expected.html:26
> +            p
> +            {
> +                margin: 0px;
> +            }
> +            #back {
> +                width: 500px;
> +                height: 500px;
> +                background-color: green;
> +            }
> +            .front
> +            {
> +                position: absolute;
> +                width: 300px;
> +                height: 300px;
> +                background-color: black;
> +                border: 50px solid blue;
> +                padding: 50px;
> +                -webkit-mask-repeat: no-repeat;
> +                -webkit-mask-origin: content-box;
> +                -webkit-mask-clip: border-box;
> +            }
> +            #front1 {

Inconsistent brace style. We prefer the opening brace on the same line.

> Source/WebCore/css/StyleResolver.cpp:1189
> +        while (newMaskLayer && oldMaskLayer) {
> +            RefPtr<MaskImageOperation> newMaskImage = newMaskLayer->maskImage();
> +            RefPtr<MaskImageOperation> oldMaskImage = oldMaskLayer->maskImage();
> +            if (newMaskImage.get() && oldMaskImage.get() && (*oldMaskImage == *newMaskImage)) {
> +                newMaskLayer->setMaskImage(oldMaskImage);
> +                if (newMaskImage->isExternalDocument())
> +                    removedExternalResources.append(newMaskImage);
> +            } else
> +                break;
> +
> +            newMaskLayer = newMaskLayer->next();
> +            oldMaskLayer = oldMaskLayer->next();
> +        }
> +
> +        Vector<RefPtr<MaskImageOperation>>& pendingResources = m_state.maskImagesWithPendingSVGDocuments();
> +        for (int i = pendingResources.size() - 1; i >= 0; i--) {
> +            if (removedExternalResources.contains(pendingResources[i]))
> +                pendingResources.remove(i);
> +        }

This doesn't seem correct if a new mask was inserted early in the list. Seems like you should just consider sets of the old and new images.

> Source/WebCore/rendering/RenderLayerMaskImageInfo.cpp:124
> -    const FillLayer* maskLayer = m_layer.renderer().style().maskLayers();
> +    const FillLayer* maskLayer = oldStyle->maskLayers();

oldStyle should be a reference.

[Radu Stavila]

Created attachment 243118 [details]
Implemented Simon's suggestions

[Antti Koivisto]

Landed this in http://trac.webkit.org/changeset/177155

[Alexey Proskuryakov]

Rolled out in r177169, because the patch caused crashes:

https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK1%20(Tests)/r177159%20(1259)/results.html

[Radu Stavila]

Created attachment 243194 [details]
Moved WebKitCSSResourceClass out of the list values section

[Antti Koivisto]

r=me for that change

[WebKit Commit Bot]

Comment on attachment 243194 [details]
Moved WebKitCSSResourceClass out of the list values section

Clearing flags on attachment: 243194

Committed r177223: <http://trac.webkit.org/changeset/177223>

[Alexey Proskuryakov]

Test animations/cross-fade-webkit-mask-image.html fails on multiple bots:

+CONSOLE MESSAGE: line 238: Unexpected mismatch between CSS Image functions.
+CONSOLE MESSAGE: line 238: Unexpected mismatch between CSS Image functions.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 139589

[Alexey Proskuryakov]

Radu is going to mark the test as flaky for now, as this is likely an issue with the test itself.

[Radu Stavila]

Created https://bugs.webkit.org/show_bug.cgi?id=139590 for the flaky cross-fade-webkit-mask-image test.

[Alexey Proskuryakov]

One of the new tests flakily crashes:

https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r177237%20(9289)/css3/masking/mask-multiple-values-crash-log.txt

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010616b1fa WTFCrash + 42 (Assertions.cpp:321)
1   com.apple.WebCore             	0x000000010b6be019 WebCore::StyleResolver::loadPendingResources() + 153 (StyleResolver.cpp:3759)
2   com.apple.WebCore             	0x000000010b6b7e12 WebCore::StyleResolver::applyMatchedProperties(WebCore::StyleResolver::MatchResult const&, WebCore::Element const*, WebCore::StyleResolver::ShouldUseMatchedPropertiesCache) + 1906 (StyleResolver.cpp:1827)
3   com.apple.WebCore             	0x000000010b6b5a53 WebCore::StyleResolver::styleForElement(WebCore::Element*, WebCore::RenderStyle*, WebCore::StyleSharingBehavior, WebCore::RuleMatchingBehavior, WebCore::RenderRegion const*) + 1251 (StyleResolver.cpp:803)
4   com.apple.WebCore             	0x000000010b770ab6 WebCore::SVGElement::customStyleForRenderer(WebCore::RenderStyle&) + 150 (SVGElement.cpp:790)

[Radu Stavila]

Simon already filed a bug, I'll handle it asap: https://bugs.webkit.org/show_bug.cgi?id=139592

[Alexey Proskuryakov]

It's also crashing css3/masking/mask-svg-no-fragmentId.html in the same way when I run tests locally.

It's already four broken tests from this change, most of them flakily asserting. It doesn't seem OK to keep the patch in the tree, even if root causes are pre-existing - skipping all potentially affected tests is a non-trivial task. I'm going to roll out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 139631

[Radu Stavila]

Created attachment 243490 [details]
Rebased and prepared for reland

The pre-existing issue that caused the rollout has been fixed (https://bugs.webkit.org/show_bug.cgi?id=139644), relanding this.

[WebKit Commit Bot]

Comment on attachment 243490 [details]
Rebased and prepared for reland

Clearing flags on attachment: 243490

Committed r177494: <http://trac.webkit.org/changeset/177494>

[Simon Fraser (smfr)]

This broke data URIs in mask images: -webkit-mask-image: url(data:image/png;base64,...

[Simon Fraser (smfr)]

Filed bug 141857.

[Simon Fraser (smfr)]

This also caused bug 146509.

[Simon Fraser (smfr)]

Also caused bug 146561.

[Simon Fraser (smfr)]

I'm going to roll this out.

[Simon Fraser (smfr)]

Rolled out via bug 146653.

[Simon Fraser (smfr)]

When this gets worked on again, it needs to:
1. not have layering violations
2. not break mime types used to load content
3. not change cross-origin checking

[Dirk Schulze]

(In reply to comment #29)
> When this gets worked on again, it needs to:
> 1. not have layering violations
> 2. not break mime types used to load content
> 3. not change cross-origin checking

To sad that I didn't see the warning about rolling it out. The cross-origin checking is needed for mask element references. We didn't use to make it for normal images though. A message earlier said that point 2 was fixed. Where is the layering violation?

[Dirk Schulze]

(In reply to comment #28)
> Rolled out via bug 146653.

A really sad step backwards :(

[Simon Fraser (smfr)]

I warned on 7/2. Sorry that I had to roll this out, but the number of late-discovered regressions scared me.

The layering violation is platform/graphics/MaskImageOperation.cpp using CachedSVGDocument etc. It's also very weird that all mask image loading goes via CachedSVGDocument.

[Antti Koivisto]

This also cause security bug 145276. I would add

4. not use raw pointers

to Simon's list.

[Simon Fraser (smfr)]

This broke the image zoom on http://bicycleoutfitter.com/product/bontrager-trip-200-195280-1.htm

[Brent Fulgham]

@Sam: Is there more to do here? Or did we resolve this with future work?

[Sam Sneddon [:gsnedders]]

(In reply to Brent Fulgham from comment #35)
> @Sam: Is there more to do here? Or did we resolve this with future work?

I have no idea! Simon, Said?

[Radar WebKit Bug Importer]

<rdar://problem/114683360>

[Said Abou-Hallawa]

This patch was reverted twice but was eventually landed as 157671@main. But it was reverted again in 164756@main. 

Applying SVG masks in CSS was reimplemented in 268272@main. See bug 260732. So no more action is needed here.

*** This bug has been marked as a duplicate of bug 260732 ***