Closed Bug 1470792 Opened 2 years ago Closed 2 years ago

we probably shouldn't be generating nsChangeHint_UpdateContainingBlock when mask-image changes

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: heycam, Assigned: hiro)

Details

Attachments

(2 files)

In bug 1343139 I reviewed a patch that makes us generate nsChangeHint_UpdateContainingBlock when nsStyleEffects::HasMask() changes, with a comment saying we do this to ensure we start or stop being a containing block for fixpos elements.  But there are no corresponding changes that check for HasMask() under nsStyleDisplay::HasFixedPosContainingBlockStyleInternal.

Also, I don't see anything in the https://drafts.fxtf.org/css-masking-1/ that says we should induce a fixpos containing block here.
Priority: -- → P3
Unfortunately we haven't yet landed the crash test case for bug 1343139, and the test case doesn't crash on current m-c with removing nsChangeHint_UpdateContainingBlock for mask property changes.  I also try to confirm the crash test actually did crash on the old revision before landing 1343139, but I can't build the revision locally since maybe compiler version mismatches or something.  So I gave up checking the crash test really crashes there.

Anyway, we can land the crash test and backout the change.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76cbacf1a7d6c23bb179d39db2aa210bad462503
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Unfortunately we haven't yet landed the crash test case for bug 1343139, and
> the test case doesn't crash on current m-c with removing
> nsChangeHint_UpdateContainingBlock for mask property changes.  I also try to
> confirm the crash test actually did crash on the old revision before landing
> 1343139, but I can't build the revision locally since maybe compiler version
> mismatches or something.  So I gave up checking the crash test really
> crashes there.

Sounds good, thanks.
Comment on attachment 8987433 [details]
Bug 1470792 - Backout the changeset that introduced UpdateContainingBlock change hint for mask changes.

https://reviewboard.mozilla.org/r/252674/#review259108
Attachment #8987433 - Flags: review?(cam) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/439d0fab0006
A crash test for bug 1343139. r=heycam
https://hg.mozilla.org/integration/autoland/rev/494ec6ef4c7c
Backout the changeset that introduced UpdateContainingBlock change hint for mask changes. r=heycam
https://hg.mozilla.org/mozilla-central/rev/439d0fab0006
https://hg.mozilla.org/mozilla-central/rev/494ec6ef4c7c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.