Closed Bug 1596979 Opened 5 years ago Closed 5 years ago

SVG Color Matrix filter on theblueprint.ru does not work

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Platform:
Desktop
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 376027

People

(Reporter: alaskanemily, Assigned: alaskanemily)

References

(
URL
)

Details

This is tracking the issue reported at https://github.com/webcompat/web-bugs/issues/43632

Having done some debugging on this, we aren't even getting to https://searchfox.org/mozilla-central/source/layout/svg/nsFilterInstance.cpp#580 for any filters that are anything except grayscale on that page. I need to do more debugging to find out why this is the case. I have been testing and I can only confirm this occurs without webrender enabled (I am unsure if this will work or not with webrender).

I have tested some and found that it's not the empty element which causes this. It also seems that color matrix filter works properly when using a saved version of the web page.

Using the inspector to disable the filter (and the webkit-branded variant) make the image at least re-appear.

mozregression says:
5:30.21 INFO: Last good revision: 9d6ffc7a08b6b47056eefe1e652710a3849adbf7 (2016-01-06)
5:30.21 INFO: First bad revision: 1424cdfc075d1b7e277be914488ac73e20d1c982 (2016-01-08)
5:30.21 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d6ffc7a08b6b47056eefe1e652710a3849adbf7&tochange=1424cdfc075d1b7e277be914488ac73e20d1c982

In that range, looking for the word "filter", this commit from me looks potentially related:

Bug 1236506: Add support for "-webkit-filter" as an alias for CSS property "filter". r=heycam

I'm not sure offhand how that would make a difference, because the site uses filter further down than the -webkit-filter alias, which means the filter line would win anyway:

 .css-filter-enabled .post:hover .image,
 .css-filter-enabled .post.hovered .image,
 .css-filter-enabled .post:hover .video,
 .css-filter-enabled .post.hovered .video,
 .css-filter-enabled .post:hover .fotorama__img,
 .css-filter-enabled .post.hovered .fotorama__img {
   -webkit-filter: url(#blueify);
   -moz-filter: url(#blueify);
   -ms-filter: url(#blueify);
   -o-filter: url(#blueify);
   filter: url(#blueify)
 }

...though it could be that they do some feature-detection and sniff for e.g. "-webkit-filter" support (perhaps combined with UA sniffing) and do something silly if they detect it.

The other thing I noticed is that this #blueify filter lives in a svg element at the bottom of the page, with ID filters, which is display:none. If I remove display:none, then it suddenly starts working correctly in current Firefox.

(I checked & noticed that the #filters element is display:none in old Firefox as well, too -- 2015-01-01 -- and yet things still work there. So it's not as simple as e.g. that element being undisplayed vs. displayed in new/old builds [e.g. via UA/feature-sniffing], as I was initially guessing it might be.)

The display:none on the svg is extremely interesting, I hadn't noticed that before. It could be that Firefox treats the application of a filter from an element with display:none differently now than it did before, couldn't it?

I will also keep the possibility of sniffing for webkit-filter in mind, too.

I would have said that this is expected, and a duplicate of bug 376027, except...

(In reply to Daniel Holbert [:dholbert] from comment #1)

mozregression says:
5:30.21 INFO: Last good revision: 9d6ffc7a08b6b47056eefe1e652710a3849adbf7 (2016-01-06)
5:30.21 INFO: First bad revision: 1424cdfc075d1b7e277be914488ac73e20d1c982 (2016-01-08)

That is worth checking/looking into. It's not clear to me why the filter would have applied back then.

I think really the fix here is to make filters work using just filter elements instead of requiring an nsIFrame.

FWIW, Emily, the entry point here is SVGObserverUtils::GetAndObserveFilters (in case you hadn't found that already).

Yeah, I've figured out the end cause where where GetAndObserveFilters returns SVGObserverUtils::eHasRefsSomeInvalid. That's because the filter element has mPrimaryFrame as NULL, which is because when we were in Element::BindToTree for the svg filter element the parent node had IsInUncomposedDoc() as true.

I took a cursory look at that range of changes, although I hadn't debugged this far yet when I did. I can look again to try to see what the problem would have been. Unfortunately, it's not possible to build 2016 gecko using a 2019 OS X toolchain, though.

(In reply to Jonathan Watt [:jwatt] from comment #4)

I would have said that this is expected, and a duplicate of bug 376027, except...

(In reply to Daniel Holbert [:dholbert] from comment #1)

mozregression says:
5:30.21 INFO: Last good revision: 9d6ffc7a08b6b47056eefe1e652710a3849adbf7 (2016-01-06)
5:30.21 INFO: First bad revision: 1424cdfc075d1b7e277be914488ac73e20d1c982 (2016-01-08)

That is worth checking/looking into. It's not clear to me why the filter would have applied back then.

Actually, I think the "blue" effect that I was observing in Firefox back before the regression-range was perhaps not a true filter after all. At least: I can delete the <svg id="filters"> in devtools, in a "good" pre-regression-range Firefox build, and everything still works fine (the blue hover effect is still there). So I'm guessing that's a partially-transparent overlay element with a blue background, or something like that. And perhaps the site is doing feature-detection on -webkit-filter support, and using the "real" filter styling instead of that overlay in browsers that it thinks supports real filters?

Also: if I run a build from just after the regression range (e.g. 2016-01-10) and I toggle the pref layout.css.prefixes.webkit = false[1] and reload the page, then the blue hover effect starts working. So that confirms that the "regression" was definitely from adding support for the -webkit-filter alias, and it lends further credence to there being some iffy webkit-prefixed feature-detection shenanigans here.

[1] Note that this pref doesn't exist (or do anything) in current Nightly, but it exists in older builds from when we were still adding new webkit-prefixed aliases.

Indeed -- in builds where we support -webkit-filter (i.e. after the regression range), the site adds css-filter-enabled to the <body class="..."> element.

If you remove that class in devtools, then the blue effect "starts working" (presumably using an overlay or something instead of a CSS filter).

So this is, in fact, not a regression - just the site using two different versions of the same effect, one with a filter and one without (and choosing between them by feature-detecting support for -webkit-filter). And the version with a filter is apparently not working due to bug 376027.

(In reply to Emily Anne McDonough [:AlaskanEmily] from comment #5)

Yeah, I've figured out the end cause where where GetAndObserveFilters returns SVGObserverUtils::eHasRefsSomeInvalid. That's because the filter element has mPrimaryFrame as NULL, which is because when we were in Element::BindToTree for the svg filter element the parent node had IsInUncomposedDoc() as true.

FWIW mPrimaryFrame wouldn't be set in any case until a fair bit after BindToTree is called. Since we don't create nsIFrames for display:none content it will not be set at the normal point when nsCSSFrameConstructor would create and set the frame.

(In reply to Daniel Holbert [:dholbert] from comment #8)
So this is, in fact, not a regression

Nice investigating. I guess you and Emily can discuss whether it's worth giving attention to this bug in that case, but if not, maybe punt it over to tech evangelism?

Correct, not a regression. (It feels like a regression from a user's perspective, but under the hood it's just a different behavior due to taking a different path in the UA-sniffing. Plus, the regression-range is far back enough that it's not like users have been using this site successfully in Firefox recently & expecting it to continue working, so there's no urgency there either.)

Emily: I'd suggest we close this as a duplicate of bug 376027, and add a note on https://github.com/webcompat/web-bugs/issues/43632 saying that:
(1) the site is simply running into that bug (the filter is failing to render because it's defined in a display:none subtree)
(2) if we like, we can do outreach to the site to suggest that they work around it by removing display:none from their <svg> element (and hiding it by another means, perhaps by changing the #filters rule to #filters { display block; height: 0} instead, which seems to make the svg element have no effect on the layout, which is good).

(Someone on the webcompat team might do some outreach, if e.g. they speak Russian and can find an appropriate contact email.)

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE

Yes, I can leave a comment on the webcompat github issue.

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