1878294 - SVG is blurred during CSS transform

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Max]

Attachment: test_blurred_svg.jpg

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:122.0) Gecko/20100101 Firefox/122.0

Steps to reproduce:

1. Go to: https://www.spreadshirt.de/selbst-gestalten?productType=127&view=1&draft=jnJMaepMkaI8BvezXrrW&affiliateId=1246955&orgn=CYO&netw=OT
2. Move the design of the dog around

Actual results:

The image and text are blurred while moving.

Expected results:

Just like in Safari or Chrome, image and text should not be blurred.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5ac6a69a01f47ca050d90704a9791b8224d30f14&tochange=9d71902c65c606063dd97cfc66f42f821f9193e9

Suspect:Bug 1761770

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1761770

:nical, since you are the author of the regressor, bug 1761770, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Timothy Nikkel (:tnikkel)]

Looks like the site changes the translate only svg transform attribute on an element. This triggers a call into the ActiveLayerTracker

https://searchfox.org/mozilla-central/rev/5eedf36dc46f1683af7478c7adaf308ceb42911b/layout/painting/ActiveLayerTracker.cpp#226

it's not a regular css transform so we don't detect it and think it's been removed and increment the activity count. We should check if we detected a previous scale before incrementing.

Comment 4

[Robert Longson [:longsonr]]

So we just need to change that code to call nsIFrame::IsTransformed() instead.

Comment 5

[Robert Longson [:longsonr]]

FWIW I did try that: https://treeherder.mozilla.org/jobs?repo=try&revision=1a5e85f3148c75d1223465c2ead62ea6b90e106e&selectedTaskRun=OS4Q1m4CQ4K6Jpt9Kx3Bkg.0 We may need to make one reftest fuzzier.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1878294. Teach the scale tracking in ActiveLayerTracker to understand svg transforms. r?longsonr,#layout-reviewers

The bug comes about because the svg transform is changed, so IncrementScaleRestyleCountIfNeeded gets called. It determines that there is no transform because it doesn't check svg transforms, so it incorrectly concludes that there was a transform but now it was removed, and so marks the scale as having been changed. Then in ChooseScale in StackingContextHelper, because we think the scale is being animated, we clamp the allowed scale factor based on the ratio of the viewport and the size of the svg. So avoiding marking this as incorrectly having an animating scale fixes the bug.

A simpler fix for this bug would be to check if the mPreviousTransformScale was Nothing() and only marked as mutated if it was Some.

This fix is a bit more complicated but it allows us to detect svg transform scales changing.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Robert Longson [:longsonr] from comment #5)

FWIW I did try that: https://treeherder.mozilla.org/jobs?repo=try&revision=1a5e85f3148c75d1223465c2ead62ea6b90e106e&selectedTaskRun=OS4Q1m4CQ4K6Jpt9Kx3Bkg.0 We may need to make one reftest fuzzier.

Yeah, I ran into the same reftest with my first fix (checking if the previous scale was Nothing before marking as mutated, slightly different idea from yours but more or less the same end result). I think the fuzziness is actually probably okay (ideally we'd fix it, but I don't think it's the fault of this patch), but I uploaded a better fix that, mostly by luck, doesn't run into that issue: the arrows that get fuzzier in that reftest actually have their scale changing slightly (because of the skew transform I guess?) and so if we detect svg transform scales then we again go back to marking these elements as active.

Comment 8

[Bob Hood [:bhood]]

Tim, can you apply Priority/Severity settings to this report, please?

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1761770

Comment 10

[Glenn Watson [:gw]]

Tim, is the attached patch something that we can land soon, or is more work required on this one?

Comment 11

[Timothy Nikkel (:tnikkel)]

It needs a test. I actually wrote one yesterday and am I just working with try server to fine tune it into a usable final result.

There is a long history of ChooseScale fixes like this one, and it can be hard to keep all the requirements for the function in your head, so in order to keep the space reasonable to work in I feel automated tests are very important.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28daa2e42bd0
Teach the scale tracking in ActiveLayerTracker to understand svg transforms. r=longsonr

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/28daa2e42bd0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this worth a Beta uplift request?

Comment 16

[Timothy Nikkel (:tnikkel)]

I think it's fine to ride the trains.