User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36 Steps to reproduce: User agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 Operating system: Windows 10 64 bit Graphcis card Intel HD 520 (reproduced also with different GPU configs on different PCs) Open: https://codepen.io/tadeuszwojcik/pen/ZKJwRE which is simplified snipped of real world bug inside our web app. Drag image within black border - drags apply CSSmatrix to SVG <use> element that is used as part of clip mask as well as to 'ghost' group. Here's another example that shows the issue (random matrix changes cause visible artifacts): https://codepen.io/tadeuszwojcik/pen/oWexWW Actual results: Dragged image jumps around and sometimes outside <svg> bounds, and in general 'lags' in rendering creating pretty bad end user experience. Expected results: Dragged image should behave the same way as in FF v52 or in Chrome - no image 'leaking'/moving outside SVG, no jumps or visible lag This bug is most likely related to new graphics compositor enabled in v53 as in 52 and below it works fine. It also works fine when use disabled hardware acceleration in FF options. When trying Chrome it also behaves correctly (as in FF v52). Interestingly it only happens when applying 'translation', when we modify matrix to rotate/scale (not implemented in sample codepen) it works fine. We consider FF as first-class browser we support 100% and in general try to find workarounds in our code when finding bugs first, but this one seems to be really hard to fix on JS level.

I can reproduce the problem on Windows10 64 and Nightly55 32bit with/without e10s. Regression window with/without e10s: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0005d0bfadf72746ce36f4e8d09d9504b814557e&tochange=ba20391edbec448b56725ef01158176614706b4f Regressed by: Bug 1313898

Looks like the check at http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/painting/FrameLayerBuilder.cpp#3883-3888 is insufficient and we're reusing a mask layer with a size that's too small.

Briefly speaking, http://searchfox.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp#3908 aMaskItem->IsInvalid(dirtyRect) can not detect content change of nsSVGClipPathFrame or nsSVGMaskFrame.

Is the logic in nsDisplayMask::HasInvalidSVGMaskOrClipPath() correct? It is returning true if a mask or the clippath returns false from IsInvalid. This doesn't look right to me...

(In reply to Gerry Iles from comment #14) > Is the logic in nsDisplayMask::HasInvalidSVGMaskOrClipPath() correct? It is > returning true if a mask or the clippath returns false from IsInvalid. This > doesn't look right to me... Yes, that's wierd. Let me double check again

hmm... mask::isinvalid always return fasle, which means I eitehr can not use it or need to make it return true when it is. Or, I can go back to this version(https://reviewboard.mozilla.org/r/138734/diff/3/)

CJ, are you going to be able to fix this soon enough that we could uplift to 54, or should we consider this as a fix targeted at 55?

(In reply to Nathan Froyd [:froydnj] from comment #19) > CJ, are you going to be able to fix this soon enough that we could uplift to > 54, or should we consider this as a fix targeted at 55? I will try to make the fix land before 6/12

mstange is quite busy on other stuff. I will land the patch first since we do not have much time left for 55.

Comment on attachment 8867824 [details] Bug 1362000 - When the position of masked frame changed, always regenerate mask layer if any of maskUnits/maskContentUnits/clipPathUnits is userSpaceOnUse. https://reviewboard.mozilla.org/r/139350/#review146948

Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c4c92d37b9d When the position of masked frame changed, always regenerate mask layer if any of maskUnits/maskContentUnits/clipPathUnits is userSpaceOnUse. r=cjku

Comment on attachment 8867824 [details] Bug 1362000 - When the position of masked frame changed, always regenerate mask layer if any of maskUnits/maskContentUnits/clipPathUnits is userSpaceOnUse. https://reviewboard.mozilla.org/r/139350/#review147038 ::: layout/painting/FrameLayerBuilder.cpp:1655 (Diff revision 7) > + // layer, where the mask was drawn, stay in different coordinate space. > + // Since they are in different coordinate space, change the position of any > + // one of them makes mask layer not reusable, and modifying the position of > + // the masked frame does change the position of mask layer. > + // > + // CSS image mask's coordinate space is always objectBoudingBox, we do not typo: objectBoundingBox ::: layout/painting/FrameLayerBuilder.cpp:1656 (Diff revision 7) > + // Since they are in different coordinate space, change the position of any > + // one of them makes mask layer not reusable, and modifying the position of > + // the masked frame does change the position of mask layer. > + // > + // CSS image mask's coordinate space is always objectBoudingBox, we do not > + // have to worry about it. This seems rather magical and I'm not completely convinced it's true. Could we just always check the TopLeft() position, so that there would be no need for nsSVGEffects::HasUserSpaceOnUseUnitsMaskOrClipPath? Do you have a testcase where doing so would cause unnecessary repaints?

Comment on attachment 8867824 [details] Bug 1362000 - When the position of masked frame changed, always regenerate mask layer if any of maskUnits/maskContentUnits/clipPathUnits is userSpaceOnUse. https://reviewboard.mozilla.org/r/139350/#review147038 > This seems rather magical and I'm not completely convinced it's true. Could we just always check the TopLeft() position, so that there would be no need for nsSVGEffects::HasUserSpaceOnUseUnitsMaskOrClipPath? Do you have a testcase where doing so would cause unnecessary repaints? If we have a translation animation on masked element, we may end up keep repaint mask layer during animation becasue of the position chnage, that's the reason why I add one more check(HasUserSpaceOnUseUnitsMaskOrClipPath). But I think I might worry too much. When we apply an animation onto a mask element, we will put masked frame onto an indivisual layer, so the position of mask layer won't change. I will add one more patch to fix what you said since this one had been checkin.

Comment on attachment 8871865 [details] Bug 1362000 - (follow-up) Simplify match logic in CSSMaskLayerUserData::operator==. https://reviewboard.mozilla.org/r/143372/#review147078 Thanks!

Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/058239ada7a6 (follow-up) Simplify match logic in CSSMaskLayerUserData::operator==. r=mstange

It's getting late in the 54 cycle. Is this severe enough to warrant uplift consideration or should we let it ride the 55 train instead?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #36) > It's getting late in the 54 cycle. Is this severe enough to warrant uplift > consideration or should we let it ride the 55 train instead? I think we shoud uplift to 54 for two reasons: 1. Final patch is simple. 2. In SVG, by applying any transform animation onto a masked descendant, you might see this glitch. So I think we should get rid of it this regression in release version.

Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf76bce5f85b (followup) remove unused headers. r=me

Comment on attachment 8872024 [details] [diff] [review] beta.patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1313898 [User impact if declined]: Applying any transform animation onto a masked descendant in SVG, this glitch may be seen by users. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: this patch only [Is the change risky?]: no [Why is the change risky/not risky?]: small change [String changes made/needed]: N/A

Comment on attachment 8872024 [details] [diff] [review] beta.patch layout fix, beta54+, should be in 54.0b12

(In reply to C.J. Ku[:cjku](UTC+8) from comment #42) > [Is this code covered by automated tests?]: no > [Has the fix been verified in Nightly?]: yes > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on C.J. Ku's assessment on manual testing needs.