Bug 139294 - [SVG Masking] Enable the use of <mask> elements for -webkit-mask-image
Summary: [SVG Masking] Enable the use of <mask> elements for -webkit-mask-image
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords: AdobeTracked
Depends on: 139590 139589 139592 139631 139644
Blocks: 129682 139440 139442
  Show dependency treegraph
 
Reported: 2014-12-05 03:52 PST by Radu Stavila
Modified: 2016-05-27 01:48 PDT (History)
9 users (show)

See Also:


Attachments
Patch (93.05 KB, patch)
2014-12-05 06:15 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Patch (93.87 KB, patch)
2014-12-05 08:53 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Patch (93.97 KB, patch)
2014-12-05 14:47 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Patch (98.19 KB, patch)
2014-12-08 13:15 PST, Radu Stavila
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Implemented Simon's suggestions (97.71 KB, patch)
2014-12-11 04:35 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Moved WebKitCSSResourceClass out of the list values section (98.28 KB, patch)
2014-12-12 04:48 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Rebased and prepared for reland (99.24 KB, patch)
2014-12-18 03:59 PST, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Radu Stavila 2014-12-05 03:52:05 PST
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).
Comment 1 Radu Stavila 2014-12-05 06:15:42 PST
Created attachment 242628 [details]
Patch
Comment 2 Radu Stavila 2014-12-05 08:53:48 PST
Created attachment 242632 [details]
Patch
Comment 3 Radu Stavila 2014-12-05 14:47:56 PST
Created attachment 242666 [details]
Patch
Comment 4 Radu Stavila 2014-12-08 13:15:25 PST
Created attachment 242843 [details]
Patch
Comment 5 Radu Stavila 2014-12-09 08:16:23 PST
Created https://bugs.webkit.org/show_bug.cgi?id=139440 and https://bugs.webkit.org/show_bug.cgi?id=139442 for further improvements.
Comment 6 Simon Fraser (smfr) 2014-12-10 15:09:22 PST
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.
Comment 7 Radu Stavila 2014-12-11 04:35:31 PST
Created attachment 243118 [details]
Implemented Simon's suggestions
Comment 8 Antti Koivisto 2014-12-11 09:59:08 PST
Landed this in http://trac.webkit.org/changeset/177155
Comment 9 Alexey Proskuryakov 2014-12-11 12:09:17 PST
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
Comment 10 Radu Stavila 2014-12-12 04:48:14 PST
Created attachment 243194 [details]
Moved WebKitCSSResourceClass out of the list values section
Comment 11 Antti Koivisto 2014-12-12 04:53:13 PST
r=me for that change
Comment 12 WebKit Commit Bot 2014-12-12 06:18:47 PST
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>
Comment 13 Alexey Proskuryakov 2014-12-12 09:57:40 PST
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.
Comment 14 WebKit Commit Bot 2014-12-12 10:06:42 PST
Re-opened since this is blocked by bug 139589
Comment 15 Alexey Proskuryakov 2014-12-12 10:15:51 PST
Radu is going to mark the test as flaky for now, as this is likely an issue with the test itself.
Comment 16 Radu Stavila 2014-12-12 10:30:54 PST
Created https://bugs.webkit.org/show_bug.cgi?id=139590 for the flaky cross-fade-webkit-mask-image test.
Comment 17 Alexey Proskuryakov 2014-12-12 15:59:33 PST
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)
Comment 18 Radu Stavila 2014-12-12 23:09:12 PST
Simon already filed a bug, I'll handle it asap: https://bugs.webkit.org/show_bug.cgi?id=139592
Comment 19 Alexey Proskuryakov 2014-12-14 17:52:35 PST
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.
Comment 20 WebKit Commit Bot 2014-12-14 17:56:40 PST
Re-opened since this is blocked by bug 139631
Comment 21 Radu Stavila 2014-12-18 03:59:30 PST
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.
Comment 22 WebKit Commit Bot 2014-12-18 05:42:05 PST
Comment on attachment 243490 [details]
Rebased and prepared for reland

Clearing flags on attachment: 243490

Committed r177494: <http://trac.webkit.org/changeset/177494>
Comment 23 Simon Fraser (smfr) 2015-02-20 15:38:19 PST
This broke data URIs in mask images: -webkit-mask-image: url(data:image/png;base64,...
Comment 24 Simon Fraser (smfr) 2015-02-20 16:19:03 PST
Filed bug 141857.
Comment 25 Simon Fraser (smfr) 2015-07-02 15:42:47 PDT
This also caused bug 146509.
Comment 26 Simon Fraser (smfr) 2015-07-02 15:44:25 PDT
Also caused bug 146561.
Comment 27 Simon Fraser (smfr) 2015-07-02 15:46:19 PDT
I'm going to roll this out.
Comment 28 Simon Fraser (smfr) 2015-07-06 18:18:23 PDT
Rolled out via bug 146653.
Comment 29 Simon Fraser (smfr) 2015-07-06 18:18:57 PDT
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
Comment 30 Dirk Schulze 2015-07-06 22:48:37 PDT
(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?
Comment 31 Dirk Schulze 2015-07-06 22:50:08 PDT
(In reply to comment #28)
> Rolled out via bug 146653.

A really sad step backwards :(
Comment 32 Simon Fraser (smfr) 2015-07-06 23:25:05 PDT
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.
Comment 33 Antti Koivisto 2015-07-07 01:18:04 PDT
This also cause security bug 145276. I would add

4. not use raw pointers

to Simon's list.
Comment 34 Simon Fraser (smfr) 2015-07-25 15:04:11 PDT
This broke the image zoom on http://bicycleoutfitter.com/product/bontrager-trip-200-195280-1.htm