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.
Created attachment 208398 [details] Patch
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.
(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
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.
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'.
Created attachment 208597 [details] Patch
> > 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.
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.
Created attachment 208805 [details] Patch
Comment on attachment 208805 [details] Patch Attachment 208805 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1473147
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
Created attachment 208806 [details] Patch
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?
(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.
Comment on attachment 208806 [details] Patch r=me
Comment on attachment 208806 [details] Patch Clearing flags on attachment: 208806 Committed r154174: <http://trac.webkit.org/changeset/154174>
All reviewed patches have been landed. Closing bug.
How come the CSS masking code has no feature flag? Was it decided to make an exception and I missed the thread?
(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
(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.