Closed Bug 1217905 Opened 4 years ago Closed 4 years ago

inset box-shadow is broken with a rotation

Categories

(Core :: Graphics, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- unaffected
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Inset box shadows with an applied rotation transform don't work.
Assignee: nobody → mchang
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
Attachment #8678239 - Flags: review?(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.
Added the auto save restore again. Thanks for catching that.
Attachment #8678239 - Attachment is obsolete: true
Attachment #8678239 - Flags: review?(mstange)
Attachment #8682168 - Flags: review?(mstange)
Attachment #8682168 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/689e3adb4e51
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Will ask for aurora uplift after bug 1216506 lands due to merge conflicts.
Flags: needinfo?(mchang)
Duplicate of this bug: 1221483
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
Flags: needinfo?(mchang)
Attachment #8682168 - Flags: approval-mozilla-aurora?
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!
Attachment #8682168 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(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!
Flags: needinfo?(rkothari)
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?
Flags: needinfo?(mchang)
Nevermind, just realized the regression was from 44.
Flags: needinfo?(mchang)
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.
Flags: needinfo?(rkothari)
Attachment #8682168 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.