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

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Rik Cabanier]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is there a reason that nsStyleDisplay::CalcDifference doesn't return a repaint hint for mix-blend-mode changes?

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

And it should also stop returning the nsChangeHint_NeutralChange hint.

Comment 3

[Rik Cabanier]

(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?

Comment 4

[Rik Cabanier]

We're also seeing something similar with background blending on Windows. Could it be related?

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(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.)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 7

[Rik Cabanier]

Attachment: Added repaint hint when mix-blend-mode is changed

Comment 8

[Rik Cabanier]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b1fd98c6360

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b1fd98c6360

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?

Comment 12

[Rik Cabanier]

Sure!
Can you point me to a page that describes how I can start that process?

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Set the appropriate tracking flags (See "Tracking flags:") on this bug to "?", and answer the questions that are autofilled into the comments.

Comment 14

[Rik Cabanier]

[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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Actually, you can just request approval flags directly on the patch itself even if the bug is not being tracked.

Comment 16

[Rik Cabanier]

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]:

Comment 17

[Rik Cabanier]

(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.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(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.

Comment 20

[Rik Cabanier]

(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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 22

[Rik Cabanier]

(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.