Bug 95474 – Introduce new CSS property for clip-path

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dirk Schulze]

This bug aims to introduce a new, prefixed property 'clip-path'. Even if we have 'clip-path' already, this property is experimental and will apply to HTML content as well.

[Dirk Schulze]

Created attachment 161545 [details]
Patch

[Alexandru Chiculita]

Comment on attachment 161545 [details]
Patch

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

Looks nice. Just small comments.

> Source/WebCore/css/CSSParser.cpp:2744
> +            if (RefPtr<CSSBasicShape> wrapShape = parseBasicShape()) {

I would rather pass the value to the parseBasicShape instead of relying on reading it from the m_valueList. That's mostly because it is not incrementing the m_valueList anymore.

> Source/WebCore/css/CSSParser.cpp:2749
> +            return false;

I suppose there's no need to return false. The parsing will fail anyway.

> Source/WebCore/css/CSSParser.cpp:4749
>      CSSParserValueList* args = value->function->args.get();

I know this is old code, but I think we might need to assert that the value is a function.

> Source/WebCore/rendering/style/RenderStyle.h:1469
> +        if (rareNonInheritedData->m_clipPath != shape)

Should clip-path overwrite the clip-rect property?

[Dirk Schulze]

Comment on attachment 161545 [details]
Patch

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

Thank you for the review Alex!

>> Source/WebCore/css/CSSParser.cpp:2744
>> +            if (RefPtr<CSSBasicShape> wrapShape = parseBasicShape()) {
> 
> I would rather pass the value to the parseBasicShape instead of relying on reading it from the m_valueList. That's mostly because it is not incrementing the m_valueList anymore.

Actually it is more common on the Parser code to let the helper function return a value. parseBasicShape was an exclusion so far. And the old code incremented as well, if the value is a CSSBasicShape. No difference to the old code beside the shapeInside that I can't pass for clip-path. It seemed more logical to follow the general pattern so.

>> Source/WebCore/css/CSSParser.cpp:2749
>> +            return false;
> 
> I suppose there's no need to return false. The parsing will fail anyway.

I am following the pattern of  CSSWebkitFilter and text-shadow, box-shadow and others. As long as we can be sure that validPrimitive and parsedValue is false, you are right.

>> Source/WebCore/css/CSSParser.cpp:4749
>>      CSSParserValueList* args = value->function->args.get();
> 
> I know this is old code, but I think we might need to assert that the value is a function.

Even if we just call the function if value is a function, it seems a good idea to make it safe against possible changes in the future. I'll add an assert.

>> Source/WebCore/rendering/style/RenderStyle.h:1469
>> +        if (rareNonInheritedData->m_clipPath != shape)
> 
> Should clip-path overwrite the clip-rect property?

No, it is different to 'clip', which just applies to absolute positioned elements. However, it is undecided if 'clip' shadows 'clip-path' in the future. I will refactor the code more to support FuncIRI later anyway.

[Dirk Schulze]

Created attachment 161588 [details]
Patch

[Dirk Schulze]

Created attachment 161773 [details]
Patch

[Dirk Schulze]

Worked together with Alex to clean up the code more.

[Tim Horton]

Comment on attachment 161773 [details]
Patch

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

Is there another bug about parsing the <clip-source> part too?

It might be nice to have comments among the file list in the changelog (and to filter the ugly meaningless lines like "(WebCore):").

> Source/WebCore/ChangeLog:9
> +        CSS Masking specification. In a first step the property just excepts

s/excepts/accepts/, I think.

[Dirk Schulze]

Committed r127327: <http://trac.webkit.org/changeset/127327>

[Csaba Osztrogonác]

(In reply to comment #8)
> Committed r127327: <http://trac.webkit.org/changeset/127327>
It broke the Qt minimal build. Could you fix it, please?

[Csaba Osztrogonác]

/ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp: In static member function 'static void WebCore::ApplyPropertyClipPath<getterFunction, setterFunction, initialFunction>::applyValue(WebCore::StyleResolver*, WebCore::CSSValue*)':
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:1718: error: there are no arguments to 'basicShapeForValue' that depend on a template parameter, so a declaration of 'basicShapeForValue' must be available
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:1718: note: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated)
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp: In static member function 'static void WebCore::ApplyPropertyClipPath<getterFunction, setterFunction, initialFunction>::applyValue(WebCore::StyleResolver*, WebCore::CSSValue*) [with WebCore::BasicShape* (WebCore::RenderStyle::* getterFunction)()const = &WebCore::RenderStyle::clipPath, void (WebCore::RenderStyle::* setterFunction)(WTF::PassRefPtr<WebCore::BasicShape>) = &WebCore::RenderStyle::setClipPath, WebCore::BasicShape* (* initialFunction)() = WebCore::RenderStyle::initialClipPath]':
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:1726:   instantiated from 'static WebCore::PropertyHandler WebCore::ApplyPropertyClipPath<getterFunction, setterFunction, initialFunction>::createHandler() [with WebCore::BasicShape* (WebCore::RenderStyle::* getterFunction)()const = &WebCore::RenderStyle::clipPath, void (WebCore::RenderStyle::* setterFunction)(WTF::PassRefPtr<WebCore::BasicShape>) = &WebCore::RenderStyle::setClipPath, WebCore::BasicShape* (* initialFunction)() = WebCore::RenderStyle::initialClipPath]'
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:2074:   instantiated from here
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/css/StyleBuilder.cpp:1718: error: 'basicShapeForValue' was not declared in this scope

[Csaba Osztrogonác]

I filed a new bug report for fixing the build - https://bugs.webkit.org/show_bug.cgi?id=95639

(Because reopening bugs is forbidden, and a closed 
bug is absolutely useless to track valid failures.)

[Mike West]

Re-opened since this is blocked by 95653

[Mike West]

(In reply to comment #12)
> Re-opened since this is blocked by 95653

Sorry, didn't mean to reopen the bug, just wanted to track the build fix.