1217905 - inset box-shadow is broken with a rotation

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

Inset box shadows with an applied rotation transform don't work.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Don't pretransform dest rects if inset box shadow is rotated

We'd reset the destination matrix to the identity matrix because we pre-transformed all the dest rects. This destroyed the rotation. Now we'll do the same as outer box shadows[1], and not pretransform anything if we have a rotation.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?case=true&from=nsCSSRendering.cpp#5473

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8678239 [details] [diff] [review]
Don't pretransform dest rects if inset box shadow is rotated

Review of attachment 8678239 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +5572,5 @@
> +
> +  // XXX: we could probably handle negative scales but for now it's easier just to fallback
> +  if (!transform.HasNonAxisAlignedTransform() && transform._11 > 0.0 && transform._22 > 0.0) {
> +    // If we don't have a rotation, we're pre-transforming all the rects.
> +    aDestinationCtx->SetMatrix(gfxMatrix());

Is the original matrix ever restored? I can't find it.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Don't retransform dest rects if inset box shadow is rotated

Added the auto save restore again. Thanks for catching that.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=c458271930f2

Comment 5

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/689e3adb4e51

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/689e3adb4e51

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/689e3adb4e51

Comment 8

[Mason Chang [Inactive] [:mchang]]

Will ask for aurora uplift after bug 1216506 lands due to merge conflicts.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8682168 [details] [diff] [review]
Don't retransform dest rects if inset box shadow is rotated

Approval Request Comment
[Feature/regressing bug #]: Bug 1188075, speed up inset box shadows
[User impact if declined]: Inset box shadows with a rotation will not render correctly.
[Describe test coverage new/current, TreeHerder]: Manual testing, mochitests.
[Risks and why]: Low - We only go back through this path if the inset box shadow has a rotation applied to it.
[String/UUID change made/needed]: None

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8682168 [details] [diff] [review]
Don't retransform dest rects if inset box shadow is rotated

It's unclear how many end-users are impacted by this. Can we let this one just ride the trains? Thanks!

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: test showing wrong results

(In reply to Ritu Kothari (:ritu) from comment #11)
> Comment on attachment 8682168 [details] [diff] [review]
> Don't retransform dest rects if inset box shadow is rotated
> 
> It's unclear how many end-users are impacted by this. Can we let this one
> just ride the trains? Thanks!

I don't think this will be ok. Non rotated box shadows can create really jarring results for the users. Please load the attached test case in aurora vs nightly. Can we please reconsider uplift? Thanks!

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like we want this in order to be able to uplift bug 1211264 as well (which affects YouTube). Does this bug also affect 43?

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Nevermind, just realized the regression was from 44.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8682168 [details] [diff] [review]
Don't retransform dest rects if inset box shadow is rotated

Thanks Mason for providing the additional test case which helped me better understand issue. The box shadow rendering does seem badly messed up. Let's uplift this to Aurora44.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76459a3ecc84

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/76459a3ecc84