Bug 119614 – [CSS Masking] Add -webkit-mask-source-type property, with alpha and luminance values

• Component: CSS
• Status: RESOLVED FIXED

[Andrei Parvu]

The CSS masking specification contains the mask-type property, which can have the value of alpha or luminance, thus creating two separate types of masks.
This bug regards only the adding and parsing of the mask-type property - we do not yet apply different masks depending on the mask type.

[Andrei Parvu]

Created attachment 208398 [details]
Patch

[WebKit Commit Bot]

Attachment 208398 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/masking/parsing-mask-type-expected.txt', u'LayoutTests/fast/masking/parsing-mask-type.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSToStyleMap.cpp', u'Source/WebCore/css/CSSToStyleMap.h', u'Source/WebCore/css/DeprecatedStyleBuilder.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/style/FillLayer.cpp', u'Source/WebCore/rendering/style/FillLayer.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h']" exit_code: 1
Source/WebCore/rendering/style/RenderStyleConstants.h:167:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrei Parvu]

(In reply to comment #2)
> Attachment 208398 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/masking/parsing-mask-type-expected.txt', u'LayoutTests/fast/masking/parsing-mask-type.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSToStyleMap.cpp', u'Source/WebCore/css/CSSToStyleMap.h', u'Source/WebCore/css/DeprecatedStyleBuilder.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/style/FillLayer.cpp', u'Source/WebCore/rendering/style/FillLayer.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/SVGRenderStyleDefs.h']" exit_code: 1
> Source/WebCore/rendering/style/RenderStyleConstants.h:167:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Total errors found: 1 in 15 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

EMaskType was already defined in SVGRenderStyleDefs, I only moved it to RenderStyleConstants

[Dirk Schulze]

Comment on attachment 208398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208398&action=review

Great patch! Still some snippets.

> Source/WebCore/ChangeLog:4
> +        [CSS Masking]Add -webkit-mask-type property, with alpha and luminance values
> +        Added the -webkit-mask-type property, which can have a value of alpha or

First title, then in a new line the link to the bug report, a newline and then the description.

> Source/WebCore/ChangeLog:13
> +        * css/CSSComputedStyleDeclaration.cpp: Added case for CSSPropertyWebkitMaskType

Sentences please. You miss the period.

> Source/WebCore/ChangeLog:15
> +        * css/CSSParser.cpp: Parsed the values for CSSPropertyWebkitMaskType

Ditto, in the other sentences as well. Stop here.

> Source/WebCore/ChangeLog:39
> +        * rendering/style/RenderStyleConstants.h: Added EMaskType
> +        * rendering/style/SVGRenderStyleDefs.h: Removed EMaskType

s/Added EMaskType/Moved EMaskType from SVGRenderStyleDefs.h./

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1789
> +        case CSSPropertyWebkitMaskType: {
> +            const FillLayer* layers = style->maskLayers();
> +
> +            if (!layers)
> +                return cssValuePool().createIdentifierValue(CSSValueNone);
> +
> +            if (!layers->next()) {
> +                if (layers->maskType() == MT_ALPHA)
> +                    return cssValuePool().createIdentifierValue(CSSValueAlpha);
> +                return cssValuePool().createIdentifierValue(CSSValueLuminance);
> +            }
> +
> +            RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
> +            for (const FillLayer* currLayer = layers; currLayer; currLayer = currLayer->next()) {
> +                if (currLayer->maskType() == MT_ALPHA)
> +                    list->append(cssValuePool().createIdentifierValue(CSSValueAlpha));
> +                else
> +                    list->append(cssValuePool().createIdentifierValue(CSSValueLuminance));
> +            }
> +            return list.release();
> +        }

This needs testing. If you want to, you can add it to a new patch, since we do not compute the style for -webkit-mask yet. Or you need to add the code for testing as we do for the background property.

> Source/WebCore/css/CSSParser.cpp:4438
> +                case CSSPropertyWebkitMaskType: {
> +                    if (val->id == CSSValueAlpha || val->id == CSSValueLuminance) {
> +                        currValue = cssValuePool().createIdentifierValue(val->id);
> +                        m_valueList->next();
> +                    } else
> +                        currValue = 0;
> +                    break;
> +                }

Hm, so the -webkit-mask property does not support parsing of luminance and alpha yet. Can you open a separate bug report and state that in the ChangeLog explicitly please?

> Source/WebCore/css/CSSToStyleMap.cpp:313
> +    CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +    layer->setMaskType(*primitiveValue);

I think you want to set the enumeration and not pass the CSSPrimtiveValue.

> Source/WebCore/rendering/style/FillLayer.cpp:57
> +    , m_maskType(MT_ALPHA)

For consistency add a initialFillMaskType function that returns the enumeration.

> Source/WebCore/rendering/style/FillLayer.h:191
> +    static EMaskType initialMaskType(EFillLayerType) { return MT_ALPHA; }

So you already have the initial function here. Why didn't you use it?

> LayoutTests/ChangeLog:10
> +        * fast/masking/parsing-mask-type-expected.txt: Added.
> +        * fast/masking/parsing-mask-type.html: Added.

You can reuse the existing parsing-mask.html test. Just add your -webkit-mask-type tests there.

[Dirk Schulze]

Comment on attachment 208398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208398&action=review

>> Source/WebCore/css/CSSParser.cpp:4438
>> +                }
> 
> Hm, so the -webkit-mask property does not support parsing of luminance and alpha yet. Can you open a separate bug report and state that in the ChangeLog explicitly please?

Forget to say that it is the -webkit-mask-source-type property, not the -webkit-mask-type property. Also, -webkit-mask-source-type has an initial value of 'auto'.

[Andrei Parvu]

Created attachment 208597 [details]
Patch

[Andrei Parvu]

> > LayoutTests/ChangeLog:10
> > +        * fast/masking/parsing-mask-type-expected.txt: Added.
> > +        * fast/masking/parsing-mask-type.html: Added.
> 
> You can reuse the existing parsing-mask.html test. Just add your -webkit-mask-type tests there.

Added some tests to the parsing-mask.html file. Also, I didn't remove my test file, because I wanted to test mask-source-type with multiple fill layers.

[Dirk Schulze]

Comment on attachment 208597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208597&action=review

Great progress. Some more snippets.

> Source/WebCore/ChangeLog:6
> +        Added the -webkit-mask-type property, which can have a value of auto, alpha or

mask-source-type.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1436
> +    default:
> +        return cssValuePool().createValue(CSSValueAuto);
> +    }

Usually we have ASSERT_NOT_REACHED in the default. To make sure we do not support more than expected.

> Source/WebCore/css/CSSToStyleMap.cpp:323
> +    default:
> +        type = EMaskSourceType::MaskAuto;
> +    }

Ditto. And still set type of course (to initial value: auto).

> Source/WebCore/rendering/style/FillLayer.h:191
> +    static EMaskSourceType initialMaskSourceType(EFillLayerType) { return MaskAuto; }

The spec says that mask-image has alpha as default. So even if we need to support auto for SVG Masks, we already know that it will be alpha for FillLayers. No need to store auto or return auto. Will affect computed value as well.

> LayoutTests/fast/masking/parsing-mask-expected.txt:118
> +PASS innerStyle("-webkit-mask-source-type", "") is null

Test for existing but not supported keywords would be great as well.

> LayoutTests/fast/masking/parsing-mask.html:177
> +negativeTest("-webkit-mask-source-type", "");

Ditto.

[Andrei Parvu]

Created attachment 208805 [details]
Patch

[Early Warning System Bot]

Comment on attachment 208805 [details]
Patch

Attachment 208805 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1473147

[Early Warning System Bot]

Comment on attachment 208805 [details]
Patch

Attachment 208805 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1473148

[Andrei Parvu]

Created attachment 208806 [details]
Patch

[Dirk Schulze]

Comment on attachment 208806 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208806&action=review

Patch looks great. Just a question.

> Source/WebCore/css/CSSToStyleMap.cpp:322
> +    case CSSValueAuto:
> +        break;

Why don't you set type = EMaskSourceType::MaskAlpha; here? after all, the user explicitly specified auto?

[Andrei Parvu]

(In reply to comment #13)
> (From update of attachment 208806 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208806&action=review
> 
> Patch looks great. Just a question.
> 
> > Source/WebCore/css/CSSToStyleMap.cpp:322
> > +    case CSSValueAuto:
> > +        break;
> 
> Why don't you set type = EMaskSourceType::MaskAlpha; here? after all, the user explicitly specified auto?

Type is already initialized to the default value, which is MaskAlpha, and since auto sets the default value for FillLayer, there's no need to assign it again.

[Dirk Schulze]

Comment on attachment 208806 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 208806 [details]
Patch

Clearing flags on attachment: 208806

Committed r154174: <http://trac.webkit.org/changeset/154174>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

How come the CSS masking code has no feature flag?
Was it decided to make an exception and I missed the thread?

[Dirk Schulze]

(In reply to comment #18)
> How come the CSS masking code has no feature flag?
> Was it decided to make an exception and I missed the thread?

It was not developed behind a flag in the first place. I picked up the work and announced it on the webkit-dev mailing list: http://mac-os-forge.2317878.n4.nabble.com/CSS-Masking-in-WebKit-td191605.html

[Benjamin Poulain]

(In reply to comment #19)
> It was not developed behind a flag in the first place. I picked up the work and announced it on the webkit-dev mailing list: http://mac-os-forge.2317878.n4.nabble.com/CSS-Masking-in-WebKit-td191605.html

Fair enough :)
I did not remember that.