Bug 95474

Summary: Introduce new CSS property for clip-path
Product: WebKit Reporter: Dirk Schulze <krit>
Component: CSSAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dino, eric, macpherson, menard, mkwst, ossy, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://dvcs.w3.org/hg/FXTF/raw-file/tip/masking/index.html#the-clip-path
Bug Depends on: 95639, 95653    
Bug Blocks: 95389    
Attachments:
Description Flags
Patch
none
Patch
none
Patch thorton: review+

Dirk Schulze
Reported 2012-08-30 10:20:30 PDT
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.
Attachments
Patch (28.04 KB, patch)
2012-08-30 13:38 PDT, Dirk Schulze
no flags
Patch (28.10 KB, patch)
2012-08-30 16:48 PDT, Dirk Schulze
no flags
Patch (28.98 KB, patch)
2012-08-31 14:13 PDT, Dirk Schulze
thorton: review+
Dirk Schulze
Comment 1 2012-08-30 13:38:50 PDT
Alexandru Chiculita
Comment 2 2012-08-30 16:10:53 PDT
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 3 2012-08-30 16:44:34 PDT
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
Comment 4 2012-08-30 16:48:33 PDT
Dirk Schulze
Comment 5 2012-08-31 14:13:19 PDT
Dirk Schulze
Comment 6 2012-08-31 14:14:41 PDT
Worked together with Alex to clean up the code more.
Tim Horton
Comment 7 2012-08-31 15:08:06 PDT
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
Comment 8 2012-08-31 15:29:28 PDT
Csaba Osztrogonác
Comment 9 2012-08-31 22:19:08 PDT
(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
Comment 10 2012-09-01 00:25:44 PDT
/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
Comment 11 2012-09-01 00:43:58 PDT
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
Comment 12 2012-09-02 08:59:17 PDT
Re-opened since this is blocked by 95653
Mike West
Comment 13 2012-09-02 09:01:02 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.