dynamic changes to mix-blend-mode don't cause repaints

RESOLVED FIXED in Firefox 33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cabanier, Assigned: cabanier)

Tracking

30 Branch
mozilla35
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox33+ fixed, firefox34 fixed, firefox35 fixed)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Applying a new mix-blend-mode style doesn't update the visual rendering until you force a repaint.
I *think* the solution is that nsDisplayMixBlendMode sets a dirty rect (since forcing that fixes the issue), but I don't know where to set that.
(Assignee)

Updated

4 years ago
(Assignee)

Updated

4 years ago
Assignee: nobody → cabanier
(Assignee)

Updated

4 years ago
Flags: needinfo?(roc)
Is there a reason that nsStyleDisplay::CalcDifference doesn't return a repaint hint for mix-blend-mode changes?
And it should also stop returning the nsChangeHint_NeutralChange hint.
(Assignee)

Comment 3

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #1)
> Is there a reason that nsStyleDisplay::CalcDifference doesn't return a
> repaint hint for mix-blend-mode changes?

No, that looks like an oversight. Should I treat it like opacity and call "NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);" ?
Also, should there be a test for CSS filters in that routine?
Flags: needinfo?(roc) → needinfo?(bzbarsky)
(Assignee)

Comment 4

4 years ago
We're also seeing something similar with background blending on Windows. Could it be related?
(In reply to Rik Cabanier from comment #3)
> (In reply to Boris Zbarsky [:bz] from comment #1)
> > Is there a reason that nsStyleDisplay::CalcDifference doesn't return a
> > repaint hint for mix-blend-mode changes?
> 
> No, that looks like an oversight. Should I treat it like opacity and call
> "NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);" ?

We do nsChangeHint_RepaintFrame or UpdateOpacityLayer for opacity, not ReconstructFrame.  Since mix-blend-mode isn't (I think) propagated to layers, you shouldn't need any hint for layer updating; RepaintFrame should be fine.  (I don't think it changes anything, like absolute positioning containing blocks, that RepaintFrame wouldn't handle.)

> Also, should there be a test for CSS filters in that routine?

nsStyleSVGReset::CalcDifference already handles filter changes.

(In reply to Rik Cabanier from comment #4)
> We're also seeing something similar with background blending on Windows.
> Could it be related?

It doesn't seem related.  nsStyleBackground::Layer::operator== does compare mBlendMode, and nsStyleBackground::CalcDifference compares the layers.  Or does changing background blending require more than a repaint?  (I don't see why it should.)
Flags: needinfo?(bzbarsky)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #5)
> We do nsChangeHint_RepaintFrame or UpdateOpacityLayer for opacity, not
> ReconstructFrame.  Since mix-blend-mode isn't (I think) propagated to
> layers, you shouldn't need any hint for layer updating; RepaintFrame should
> be fine.  (I don't think it changes anything, like absolute positioning
> containing blocks, that RepaintFrame wouldn't handle.)

RepaintFrame will update layers as necessary. UpdateOpacityLayer is a special optimization which we just don't need for mix-blend-mode changes. So the correct hint here should be RepaintFrame.
(Assignee)

Comment 7

4 years ago
Created attachment 8494624 [details] [diff] [review]
Added repaint hint when mix-blend-mode is changed
(Assignee)

Comment 8

4 years ago
Comment on attachment 8494624 [details] [diff] [review]
Added repaint hint when mix-blend-mode is changed

try build: https://tbpl.mozilla.org/?tree=Try&rev=dd87b8b76fd7
Attachment #8494624 - Flags: review?(dbaron)
Attachment #8494624 - Flags: review?(dbaron) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0b1fd98c6360
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
This is a very low-risk patch, and seems worth taking on aurora (maybe even beta).  (This is a bug in a feature that's new in 32.)  Do you mind requesting approval?
Flags: needinfo?(cabanier)
Summary: mix-blend-mode doesn't work when set in JS → dynamic changes to mix-blend-mode don't cause repaints
(Assignee)

Comment 12

4 years ago
Sure!
Can you point me to a page that describes how I can start that process?
Flags: needinfo?(cabanier)
Set the appropriate tracking flags (See "Tracking flags:") on this bug to "?", and answer the questions that are autofilled into the comments.
Flags: needinfo?(cabanier)
(Assignee)

Comment 14

4 years ago
[Tracking Requested - why for this release]: This is a low risk fix. We had reports from authors that they had to work around this by forcing a repaint.
tracking-firefox33: --- → ?
Flags: needinfo?(cabanier)
Actually, you can just request approval flags directly on the patch itself even if the bug is not being tracked.
(Assignee)

Comment 16

4 years ago
Comment on attachment 8494624 [details] [diff] [review]
Added repaint hint when mix-blend-mode is changed

Approval Request Comment
[Feature/regressing bug #]: Setting the CSS mix-blend-mode through JS
[User impact if declined]: CSS property doesn't take effect. User has to force a repaint
[Describe test coverage new/current, TBPL]: None
[Risks and why]: None
[String/UUID change made/needed]:
Attachment #8494624 - Flags: approval-mozilla-beta?
(Assignee)

Comment 17

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #15)
> Actually, you can just request approval flags directly on the patch itself
> even if the bug is not being tracked.

Does this look OK? They can always ask me for more specifics if neeed.
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → fixed
tracking-firefox33: ? → +
Comment on attachment 8494624 [details] [diff] [review]
Added repaint hint when mix-blend-mode is changed

Close ;) You have also to request aurora uplift.

I did it for you.

Accepting in beta too because it is low risk and a small change.
Attachment #8494624 - Flags: approval-mozilla-beta?
Attachment #8494624 - Flags: approval-mozilla-beta+
Attachment #8494624 - Flags: approval-mozilla-aurora+
(In reply to Boris Zbarsky [:bz] from comment #15)
> Actually, you can just request approval flags directly on the patch itself
> even if the bug is not being tracked.

Oops, sorry; not sure what I was thinking.
(Assignee)

Comment 20

4 years ago
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #19)
> (In reply to Boris Zbarsky [:bz] from comment #15)
> > Actually, you can just request approval flags directly on the patch itself
> > even if the bug is not being tracked.
> 
> Oops, sorry; not sure what I was thinking.

No worries! Thanks for pointing out that we can get it in earlier.
https://hg.mozilla.org/releases/mozilla-aurora/rev/293253d7df7a
https://hg.mozilla.org/releases/mozilla-beta/rev/badc5be25cc1

Can we get a test for this?
status-firefox33: affected → fixed
status-firefox34: affected → fixed
Flags: needinfo?(cabanier)
Flags: in-testsuite?
(Assignee)

Comment 22

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/293253d7df7a
> https://hg.mozilla.org/releases/mozilla-beta/rev/badc5be25cc1
> 
> Can we get a test for this?

I don't know how to test for test since it only fails in fullscreen mode.
Flags: needinfo?(cabanier)
You need to log in before you can comment on or make changes to this bug.