Style changes to perspective get their change hints ignored if the frame isn't also transformed.

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

Attachments

(2 attachments)

nsStyleDisplay::CalcDifference set 'hint |= nsChangeHint_NeutralChange;' if HasTransformStyle returns false (which is the case for just perspective), and so ignores any change hints we generated for a new/different perspective value.

Looks like this bug has existed since I first implemented perspective, the change hint checks were initially in an if (HasTransformStyle()) block.
Bug 1466954 has a crashtest that triggers this (and hits an RDL assertion).
See Also: → 1466954
FWIW, this is a crash report caused by the same reason. bp-16522335-6c5b-495b-928e-97fe40180613
Actually, the original description isn't quite right. The check for a changed value of HasPerspectiveStyle() sets the value of 'hint' and not 'transformHint', so it always applied.

The bug here is that we only generate nsChangeHint_UpdateContainingBlock in that case (since the check for changes to mChildPerspective *is* scoped to transformed frame) and not nsChangeHint_RepaintFrame.

The broken testcases are applying the new perspective to the document element (which is already a containing block for fixed elements), so it ends up being a no-op, and nothing invalidates for the newly added perspective.

We do have an existing testcase for the normal case of this (layout/reftests/w3c-css/submitted/transforms/perspective-containing-block-dynamic-1b.html) and that passes already.
Comment on attachment 8986079 [details]
Bug 1467688 - Make sure we invalidate for perspective changes even if the frame isn't otherwise transformed.

https://reviewboard.mozilla.org/r/251528/#review257800

::: layout/style/nsStyleStruct.cpp:3879
(Diff revision 1)
> +  if (HasPerspectiveStyle() != aNewData.HasPerspectiveStyle()) {
> +    // A change from/to being a containing block for position:fixed.
> +    hint |= nsChangeHint_UpdateContainingBlock |
> +            nsChangeHint_UpdateOverflow |
> +            nsChangeHint_RepaintFrame;
> +  } else if (mChildPerspective != aNewData.mChildPerspective) {
> +    hint |= nsChangeHint_UpdateOverflow |
> +            nsChangeHint_RepaintFrame;
> +  }
> +

I did now mostly same fix for this bug. :)

I think we can rewrite this if-else block like this;

```c++
  if (mChildPerspective != anewData.mChildPerspective) {
    hint |= nsChangeHint_UpdateOverflow |
            nsChangeHint_RepaintFrame;
    if (HasPerspectiveStyle()! = aNewData.HasPerspectiveStyle()) {
      hint |= nsChangeHint_UpdateContainingBlock;
    }
  }
```
Assignee: nobody → matt.woodrow
Blocks: RDLbugs
Comment on attachment 8986079 [details]
Bug 1467688 - Make sure we invalidate for perspective changes even if the frame isn't otherwise transformed.

https://reviewboard.mozilla.org/r/251528/#review258936

Seems like it might be good to add a reftest rather than just a crashtest?
Attachment #8986079 - Flags: review?(dbaron) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 09fe2c0b64bbf5f7856e3216d1746ffd971f62f1 -d 5a560404cf9e: rebasing 470889:09fe2c0b64bb "Bug 1467688 - Make sure we invalidate for perspective changes even if the frame isn't otherwise transformed. r=dbaron" (tip)
merging layout/base/crashtests/crashtests.list
merging layout/style/nsStyleStruct.cpp
warning: conflicts while merging layout/base/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b976829f826a
Make sure we invalidate for perspective changes even if the frame isn't otherwise transformed. r=dbaron
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3446bd708e60
Backed out 1 changesets for reftest failures crashtests/1467688.html CLOSED TREE
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08ad3e81d9af
Make sure we invalidate for perspective changes even if the frame isn't otherwise transformed. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/08ad3e81d9af
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8986079 [details]
Bug 1467688 - Make sure we invalidate for perspective changes even if the frame isn't otherwise transformed.

Approval Request Comment
[Feature/Bug causing the regression]: RDL
[User impact if declined]: Assertion crashes on Nightly/Dev Edition, potentially incorrect z-ordering on release (though usually temporary).
[Is this code covered by automated tests?]: Crashtest added.
[Has the fix been verified in Nightly?]: By me, yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just adds extra style change hints to make sure we invalidate. Should never be a problem to hint too many changes.
[String changes made/needed]: None.
Flags: needinfo?(matt.woodrow)
Attachment #8986079 - Flags: approval-mozilla-beta?
Comment on attachment 8986079 [details]
Bug 1467688 - Make sure we invalidate for perspective changes even if the frame isn't otherwise transformed.

Crash fix, adds new tests, let's uplift for beta 8.
Attachment #8986079 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.