1378710 - SVG: mask renders incorrectly when clipped group has animated contents

Status: RESOLVED FIXED

(Core :: SVG, defect, P1)

Description

[Paul LeBeau]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Related to bug #1378707. Also see: https://stackoverflow.com/questions/44935822/svg-text-clippath-in-firefox

If a mask is applies to a group that contains only animated elements, then it does not render correctly.




Actual results:

See: https://jsfiddle.net/q770saez/1/

<svg viewBox="0 0 600 150">

  <mask id="m">
    <rect width="100%" height="100%" fill="black"/>
    <polygon points="0,0, 600,75, 0,150" fill="white"/>
  </mask>

  <g mask="url(#m)">
    <rect width="100%" height="100%" class="anim-shape"/>
  </g> 

</svg>

.anim-shape {
  transform: translate(-600px, 0);
  animation: moving-panel 4s forwards ease-out;
}     

@keyframes moving-panel {
  100% {
    transform: translate(0, 0);
  }
}





Expected results:

But if you apply the same workaround as suggested in bug #1378707 (non-animated transparent rect in the group), the animation renders correctly.

https://jsfiddle.net/q770saez/3/

  <g mask="url(#m)">
    <rect x="0" y="0" width="100%" height="100%" fill="transparent" />
    <rect width="100%" height="100%" class="anim-shape"/>
  </g>

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0005d0bfadf72746ce36f4e8d09d9504b814557e&tochange=ba20391edbec448b56725ef01158176614706b4f

Regressed by: Bug 1313898

Comment 2

[u459114]

Attachment: anim-mask.svg

The mask we generate in ContainerState::SetupMaskLayerForCSSMask is not correct. Attach a simpler test case

Comment 3

[Gerry Chang [:gchang]]

Not a new regression in 54. Mark 54 won't fix and see if we can fix it in 55.

Comment 4

[u459114]

Attachment: Bug 1378710 - Correct the size of mask surface.

Review commit: https://reviewboard.mozilla.org/r/156062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156062/

Comment 5

[u459114]

Comment on attachment 8885179 [details]
Bug 1378710 - Correct the size of mask surface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156062/diff/1-2/

Comment 6

[u459114]

Attachment: Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

The condition is
1. The transform animation applies to a SVG element.
2. There is an SVG mask applying to a <svg::g> element, which is a acenstor of
the transform target element.

Here is an example:
.transform-anim {
    animation: ...;
}
<g mask="url(#mask)">
  <rect class="transform-anim" />
</g>

transform animation appplies onto <rect> and svg mask applied onto <g>

Review commit: https://reviewboard.mozilla.org/r/156428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156428/

Comment 7

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/1-2/

Comment 8

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/2-3/

Comment 9

[u459114]

Attachment: anim-mask.html

Comment 10

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/3-4/

Comment 11

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/4-5/

Comment 12

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/5-6/

Comment 14

[Robert Longson [:longsonr]]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

https://reviewboard.mozilla.org/r/156428/#review161632

::: layout/painting/nsDisplayList.cpp:7419
(Diff revision 6)
>          AnimationPerformanceWarning::Type::TransformFrameInactive));
>  
>      return NoPrerender;
>    }
>  
> +  // We should not allow prerender if any ancestor container element carries

...if any ancestor container element has mask/clip-path effects.

::: layout/painting/nsDisplayList.cpp:7423
(Diff revision 6)
>  
> +  // We should not allow prerender if any ancestor container element carries
> +  // mask/clip-path effects.
> +  //
> +  // With prerender and sync transform animation, we do not need to restyle
> +  // animated element to respect position change, since that transform is done

...need to restyle an animated element to respect position changes, since...

::: layout/painting/nsDisplayList.cpp:7425
(Diff revision 6)
> +  // mask/clip-path effects.
> +  //
> +  // With prerender and sync transform animation, we do not need to restyle
> +  // animated element to respect position change, since that transform is done
> +  // by layer animation. As a result, the container element is not aware of
> +  // position change of that containing element and lose chance to update

and loses the chance to...

::: layout/painting/nsDisplayList.cpp:7426
(Diff revision 6)
> +  //
> +  // With prerender and sync transform animation, we do not need to restyle
> +  // animated element to respect position change, since that transform is done
> +  // by layer animation. As a result, the container element is not aware of
> +  // position change of that containing element and lose chance to update
> +  // the content of mask/clip-path accordingly.

the content of the mask/clip-path

::: layout/painting/nsDisplayList.cpp:7428
(Diff revision 6)
> +  // animated element to respect position change, since that transform is done
> +  // by layer animation. As a result, the container element is not aware of
> +  // position change of that containing element and lose chance to update
> +  // the content of mask/clip-path accordingly.
> +  //
> +  // But why need updating mask? This is relative to how we generate a mask

Why do we need to update a mask? rather than But why need updating...

::: layout/painting/nsDisplayList.cpp:7432
(Diff revision 6)
> +  //
> +  // But why need updating mask? This is relative to how we generate a mask
> +  // layer in ContainerState::SetupMaskLayerForCSSMask. While creating a mask
> +  // layer, to reduce memory usage, we did not choose the size of the masked
> +  // element as mask size. Instead, we read the union of bounds of all children
> +  // display items by nsDisplayWrapList::GetBounds, which is smaller then or

smaller than

::: layout/painting/nsDisplayList.cpp:7433
(Diff revision 6)
> +  // But why need updating mask? This is relative to how we generate a mask
> +  // layer in ContainerState::SetupMaskLayerForCSSMask. While creating a mask
> +  // layer, to reduce memory usage, we did not choose the size of the masked
> +  // element as mask size. Instead, we read the union of bounds of all children
> +  // display items by nsDisplayWrapList::GetBounds, which is smaller then or
> +  // equal to the masked element's boundary, and use it as position/size of

as the position/size

::: layout/painting/nsDisplayList.cpp:7435
(Diff revision 6)
> +  // layer, to reduce memory usage, we did not choose the size of the masked
> +  // element as mask size. Instead, we read the union of bounds of all children
> +  // display items by nsDisplayWrapList::GetBounds, which is smaller then or
> +  // equal to the masked element's boundary, and use it as position/size of
> +  // the mask layer. That union bounds is actually effected by the geometry of
> +  // the animated element. To keep the content of mask up to date, forbid

forbidding of prerender is required

Comment 15

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/6-7/

Comment 16

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/7-8/

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

https://reviewboard.mozilla.org/r/156428/#review164930

I'm not very happy about this patch, but I don't have any better ideas. I guess this is not a regression compared to before we had layerized masks, so it's ok.

::: layout/painting/nsDisplayList.cpp:7434
(Diff revision 8)
> +  // mask layer in ContainerState::SetupMaskLayerForCSSMask. While creating a
> +  // mask layer, to reduce memory usage, we did not choose the size of the
> +  // masked element as mask size. Instead, we read the union of bounds of all
> +  // children display items by nsDisplayWrapList::GetBounds, which is smaller
> +  // than or equal to the masked element's boundary, and use it as the position
> +  ///size of the mask layer. That union bounds is actually effected by the

affected

::: layout/painting/nsDisplayList.cpp:7438
(Diff revision 8)
> +  // than or equal to the masked element's boundary, and use it as the position
> +  ///size of the mask layer. That union bounds is actually effected by the
> +  // geometry of the animated element. To keep the content of mask up to date,
> +  // forbidding of prerender is required.
> +  for (nsIFrame* container = aFrame->GetParent(); container;
> +       container = container->GetParent()) {

Should this be using nsLayoutUtils::GetCrossDocParentFrame? Otherwise I think you could have an OMTA animation inside a transparent iframe where the iframe has a mask.

Comment 18

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

https://reviewboard.mozilla.org/r/156428/#review164930

I don't like it either, since it's bad for rendering smoothness of animation. Base on my understanding of gecko, that what I can do so far, let me file a follow up to unblock this limitation later.

Comment 19

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/8-9/

Comment 20

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/9-10/

Comment 21

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156428/diff/10-11/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41c7ad2d0326
Disable OMTA if there is a mask applied on any ancestor of the animation target. r=mstange

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/41c7ad2d0326

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 25

[u459114]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1313898
[User impact if declined]: the shape of mask may not be correct during a transform animation of a masked descedant. 
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]:
1. verify the renering result of anim-mask.svg during the animation, and compare with chrome
2. virify the renering result of https://jsfiddle.net/q770saez/1/, and compare with chrome
[List of other uplifts needed for the feature/fix]: NA
[Is the change risky?]: no
[Why is the change risky/not risky?]: small change.
[String changes made/needed]: NA

I did not create a test case for this bug since reftest harness can not capture this glich. Glitch we saw on the screen is vanished while we do a snapshot in reftest.

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 8885577 [details]
Bug 1378710 - Disable OMTA if there is a mask applied on any ancestor of the animation target.

We shipped this in 54, we're almost at RC week, I'd prefer to punt on this for 55, unless somebody tells me this is really critical.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72873c109b1b
(followup) Correct typo in the comment. r=me DONTBUILD

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/72873c109b1b

Comment 29

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/72873c109b1b74ddd329d2e9f0a9506be3b05556
Bug 1378710 - (followup) Correct typo in the comment. r=me DONTBUILD