901375 - Implement parsing of mix-blend-mode property

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Rik Cabanier]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36

Comment 1

[Rik Cabanier]

The CSS blending specification defines a property for blending of elements: http://dev.w3.org/fxtf/compositing-1/#mix-blend-mode

this bug will implement parsing of this property and will also implement the feature.

Comment 2

[Rik Cabanier]

Attachment: First patch

Checking for syntax problems. Need more comments.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 785600 [details] [diff] [review]
First patch

Review of attachment 785600 [details] [diff] [review]:
-----------------------------------------------------------------

I can't really comment on the changes in the layers-related files, since I'm not that familiar with them.  The other stuff generally looks OK.  A few notes on the tests:

  * name the test files blend-color-01.html rather than svg-blend-color.html
  * name the reference files "-ref" rather than "-expected"
  * if the tests don't need any mixed HTML/SVG functionality, better to have them as pure SVG documents
  * please include a CC0 comment at the top of the files, if you're willing to (like say rect-01.svg has)

Check the other cases where we call PushGroup in layout/svg/, some of which are to handle opacity, to see if we need to handle mix-blend-mode there too.

A few other quick comments:

::: gfx/layers/basic/BasicLayersImpl.cpp
@@ +102,5 @@
>    aContext->Paint(aOpacity);
>  }
> +    
> +void
> +PaintWithMask(gfxContext* aContext, float aOpacity, Layer* aMaskLayer)

I think it'd be better to just have a single function, and don't bother about making the aMixBlendMode optional.  There aren't many callers of PaintWithMask.

::: layout/generic/nsFrame.cpp
@@ +1804,5 @@
>      inTransform = true;
>    }
>  
>    bool useOpacity = HasOpacity() && !nsSVGUtils::CanOptimizeOpacity(this);
> +  useOpacity = useOpacity || disp->mMixBlendMode;

If you're ORing this in here, "useOpacity" is no longer the right name for the variable.

@@ +2090,4 @@
>      return;
>  
>    // Child is composited if it's transformed, partially transparent, or has
>    // SVG effects.

Update the comment.

::: layout/style/nsCSSKeywordList.h
@@ +666,5 @@
> +CSS_KEY(exclusion, exclusion)
> +CSS_KEY(hue, hue)
> +CSS_KEY(saturation, saturation)
> +CSS_KEY(color, color)
> +CSS_KEY(luminosity, luminosity)

I'm not sure why the file splits the '-moz-appearance' property keywords out from the rest of the list, and why that second half of the file has accreted some other keywords, but best not make it worse and include your new CSS_KEY entries in the top half of the file, and inserted at their sorted positions.

::: layout/style/nsCSSPropList.h
@@ +2285,5 @@
> +    "layout.css.mix-blend-mode.enabled",
> +    VARIANT_HK,
> +    kBlendModeKTable,
> +    offsetof(nsStyleDisplay, mMixBlendMode),
> +    eStyleAnimType_None)

Are you planning to get CSS animation working on this property with this patch?  If not, then you can replace the "offsetof(...)" with CSS_PROP_NO_OFFSET.  Otherwise the animation type should be eStyleAnimType_EnumU8, I believe.

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +490,5 @@
>  
>    bool complexEffects = false;
>    /* Check if we need to do additional operations on this child's
>     * rendering, which necessitates rendering into another surface. */
> +  if (opacity != 1.0f || maskFrame || (clipPathFrame && !isTrivialClip) || mixBlendMode) {

Better to not rely on the fact that the "normal" mix-blend-mode converts to false.

::: layout/svg/nsSVGUtils.cpp
@@ +845,1 @@
>      complexEffects = true;

I think we'll need to handle the mMixBlendMode in here once we pop the group and paint it, like we do with the opacity value.  We sometimes paint SVG frames without creating layers, such as when we paint them inside a pattern, in a mask, etc.

::: modules/libpref/src/init/all.js
@@ +1776,5 @@
>  pref("layout.css.masking.enabled", true);
>  #endif
>  
> +// pref to turn on blending
> +pref("layout.css.mix-blend-mode.enabled", true);

This should start off false.

Comment 4

[Rik Cabanier]

Attachment: rev3.patch

Comment 5

[Rik Cabanier]

I think I addressed most of Cameron's comments.
There might be issues with patterns and complex clipping paths. I will experiment and file them as separate bugs if I find issues.

Comment 6

[Robert Longson [:longsonr]]

rev3.patch appears to be pretty much empty.

Comment 7

[Rik Cabanier]

Attachment: rev7.patch

Comment 8

[Rik Cabanier]

Attachment: rev8.patch

Comment 9

[Rik Cabanier]

Attachment: rev9.patch

Reduced patch that just introduces storing and querying of the mix-blend-mode property + making it a runtime flag

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 786659 [details] [diff] [review]
rev9.patch

Review of attachment 786659 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: layout/style/nsComputedDOMStyle.cpp
@@ +3624,5 @@
>  
>  CSSValue*
> +nsComputedDOMStyle::DoGetMixBlendMode()
> +{
> +    nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;

Two spaces of indent, and "*" next to the type.

::: layout/style/nsRuleNode.cpp
@@ +4882,5 @@
>    SetDiscrete(*aRuleData->ValueForDisplay(), display->mDisplay, canStoreInRuleTree,
>                SETDSC_ENUMERATED, parentDisplay->mDisplay,
>                NS_STYLE_DISPLAY_INLINE, 0, 0, 0, 0);
> +
> +  // display: enum, inherit, initial

mix-blend-mode

@@ +4883,5 @@
>                SETDSC_ENUMERATED, parentDisplay->mDisplay,
>                NS_STYLE_DISPLAY_INLINE, 0, 0, 0, 0);
> +
> +  // display: enum, inherit, initial
> +  SetDiscrete(*aRuleData->ValueForMixBlendMode(), display->mMixBlendMode, canStoreInRuleTree,

Wrap to 80 columns.

@@ +4885,5 @@
> +
> +  // display: enum, inherit, initial
> +  SetDiscrete(*aRuleData->ValueForMixBlendMode(), display->mMixBlendMode, canStoreInRuleTree,
> +              SETDSC_ENUMERATED, parentDisplay->mMixBlendMode,
> +               NS_STYLE_BLEND_NORMAL, 0, 0, 0, 0);

Indenting is off.

::: layout/style/test/property_database.js
@@ +2787,5 @@
>  			"calc(3*25px + 50%)",
>  		],
>  		invalid_values: [ "auto", "none", "5" ]
>  	},
> +

Don't think this blank line is warranted.

@@ +4642,5 @@
> +                       "hue",
> +                       "saturation",
> +                       "color",
> +                       "luminosity"
> +                       ],

Most other entries in this file keep their values on one line, so maybe do that here too.

(If you care, this file for some reason uses tabs for indentation.  Match it if you want.)

::: modules/libpref/src/init/all.js
@@ +1775,5 @@
>  #else
>  pref("layout.css.masking.enabled", true);
>  #endif
>  
> +// pref to turn on blending

The prefs around here for enabling features have comments of the form "Is support for blah enabled?", so I suggest following that style.

Comment 11

[Cameron McCormack (:heycam)]

One thing you will need to do in a separate patch is handle mMixBlendMode in nsStyleDisplay::CalcStyleDifference.  I think we will need a new change hint, nsChangeHint_UpdateMixBlendModeLayer.

Comment 12

[Cameron McCormack (:heycam)]

(Though you should check with someone more familiar with layers than me.)

Comment 13

[Rik Cabanier]

Attachment: rev10.patch

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: rev11.patch

Fix bitrot and add patch headers

Comment 15

[Rik Cabanier]

try build: https://tbpl.mozilla.org/?tree=Try&rev=11023c8d31fe

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/62d14ef85b34

Comment 17

[Tom S [:evilpie]]

It's not immediately clear from the commit message, but did this only implement parsing?

Comment 18

[Rik Cabanier]

(In reply to Tom Schuster [:evilpie] from comment #17)
> It's not immediately clear from the commit message, but did this only
> implement parsing?

yes. I will land the actual drawing implementation shortly.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62d14ef85b34