Closed Bug 901375 Opened 7 years ago Closed 7 years ago

Implement parsing of mix-blend-mode property

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: cabanier, Assigned: cabanier)

References

Details

Attachments

(1 file, 6 obsolete files)

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
Component: Untriaged → SVG
Product: Firefox → Core
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.
Attached patch First patch (obsolete) — Splinter Review
Checking for syntax problems. Need more comments.
Attachment #785600 - Flags: feedback?(cam)
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.
Attachment #785600 - Flags: feedback?(cam) → feedback+
Assignee: nobody → cabanier
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch rev3.patch (obsolete) — Splinter Review
Attachment #786114 - Flags: feedback?(cam)
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.
rev3.patch appears to be pretty much empty.
Attached patch rev7.patch (obsolete) — Splinter Review
Attachment #786456 - Flags: feedback?(cam)
Attachment #786114 - Flags: feedback?(cam)
Attachment #785600 - Attachment is obsolete: true
Attachment #786114 - Attachment is obsolete: true
Attachment #786456 - Flags: feedback?(cam)
Attachment #786456 - Attachment is obsolete: true
Attached patch rev8.patch (obsolete) — Splinter Review
Attachment #786618 - Flags: review?(cam)
Attachment #786618 - Flags: review?(cam)
Attached patch rev9.patch (obsolete) — Splinter Review
Reduced patch that just introduces storing and querying of the mix-blend-mode property + making it a runtime flag
Attachment #786659 - Flags: review?(cam)
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.
Attachment #786659 - Flags: review?(cam) → review+
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.
(Though you should check with someone more familiar with layers than me.)
Attached patch rev10.patch (obsolete) — Splinter Review
Attachment #786659 - Attachment is obsolete: true
Attachment #786618 - Attachment is obsolete: true
Keywords: checkin-needed
Attached patch rev11.patchSplinter Review
Fix bitrot and add patch headers
Attachment #786689 - Attachment is obsolete: true
Summary: Implement CSS blending for SVG → Implement support in CSS for mix-blend-mode
It's not immediately clear from the commit message, but did this only implement parsing?
(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.
Blocks: 902525
Component: SVG → DOM: CSS Object Model
https://hg.mozilla.org/mozilla-central/rev/62d14ef85b34
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 902642
Summary: Implement support in CSS for mix-blend-mode → Implement parsing of mix-blend-mode property
Depends on: 952051
No longer depends on: 952051
Blocks: 952643
You need to log in before you can comment on or make changes to this bug.