Bug 44650 – Support element() CSS image type

• Component: CSS
• Status: NEW

[Ojan Vafai]

See http://hacks.mozilla.org/2010/08/mozelement/ for more info. Looks awesome IMO.

[Anthony Ricaud]

FWIW, this has been added to a draft spec : http://dev.w3.org/csswg/css3-images/#element-reference

[Simon Fraser (smfr)]

<rdar://problem/10197315>

[Simon Fraser (smfr)]

Adam, can you think of any security issues in the CSS element() function? Could you add some notes here?

[Adam Barth]

> Adam, can you think of any security issues in the CSS element() function?

Not of the top of my head.  Did you have something in mind?  I can ask the security team to do a more thorough security review if you'd like.

[Ojan Vafai]

(In reply to comment #4)
> > Adam, can you think of any security issues in the CSS element() function?
> 
> Not of the top of my head.  Did you have something in mind?  I can ask the security team to do a more thorough security review if you'd like.

Lets do that. I'd really like to see us implement this and it would be good to catch and security issues early.

[Simon Fraser (smfr)]

That could be good. We're trying to look at the use cases of this vs. canvas.drawElement(); there's some overlap, so maybe this is preferable in some cases for having fewer security issues.

[David Mulder]

As I was looking into support in other browsers I came across this ticket and thought I would share a demo which I made and supports Simon Frasers suggestion: https://developer.mozilla.org/en-US/demos/detail/aero-windows

Secondly a more realistic use case which would not be possible with the canvas.drawElement is for example displaying a slide from a slideshow which contains images form different domains (quite common for online slideshow applications).

[Joseph Pearson]

I've been quietly riding this bug for a few months, so I thought I'd contribute why this would be huge for our project (the open-source ebook library, Monocle).

Here's a (Flipboard-inspired) example that works in Firefox 11:

   http://monocle.inventivelabs.com.au/static/scripts/src/test/experimental/moz-element-flipper/index.html


Our general use case for element(): we display "pages" as overlapping frames. Right now we load the content into multiple iframes — this is suboptimal for large/complex content, common in ebooks. element() would let us load the content just once into an "origin" iframe, and echo it for visual effects. An example of our current approach: http://monocle.inventivelabs.com.au/static/scripts/src/test/showcase/01-velveteen/index.html

[Pete Boere]

Another use case is to create watermark backgrounds (view dabblet in firefox):

http://dabblet.com/gist/1729950

[bop]

In my humble opinion, it's more important that mask-image accepts the element() image type than background-image. I'd love to use text as a mask for an image (=text with a pattern).

[Anish]

Created attachment 162898 [details]
This sets the foundation for -webkit-element by adding it to the grammar, parser, and values

first step for bug (parsing and grammar), not complete solution

[Tony Chang]

Comment on attachment 162898 [details]
This sets the foundation for -webkit-element by adding it to the grammar, parser, and values

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

I just skimmed the patch.

> Source/WebCore/ChangeLog:8
> +        Updated the grammar to notice IDSEL for term -webkit-element(#elementid). CSSParser

Please include a link to the spec.

> Source/WebCore/ChangeLog:23
> +        * css/CSSGrammar.y:
> +        * css/CSSParser.cpp:
> +        (WebCore::CSSParser::parseContent):
> +        (WebCore::CSSParser::parseFillImage):
> +        (WebCore):
> +        (WebCore::CSSParser::parseWebKitElement):
> +        * css/CSSParser.h:

Please describe your changes in this section of the ChangeLog.

> Source/WTF/wtf/Platform.h:1054
> +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(QT) || (PLATFORM(WIN) && !OS(WINCE) && !PLATFORM(WIN_CAIRO))
> +#define ENABLE_CSS_WEBKIT_ELEMENT 1

How did you pick which platforms to enable this on?  I suspect everyone will want it disabled until it's a bit more complete.
Also, if I can bikeshed over the name, I would call it CSS_ELEMENT.

> Source/WebCore/css/CSSGrammar.y:1461
> +  | IDSEL maybe_space {
> +#if ENABLE(CSS_WEBKIT_ELEMENT)
> +      $$.id = 0;
> +      $$.unit = CSSPrimitiveValue::CSS_WEBKIT_ELEM;

Can you explain this change to the grammar? Would be good to note in the ChangeLog.

> Source/WebCore/css/CSSParser.cpp:7253
> +    if (arg->unit != CSSPrimitiveValue::CSS_WEBKIT_ELEM && arg->unit != CSSPrimitiveValue::CSS_PARSER_HEXCOLOR)

Why CSS_PARSER_HEXCOLOR?

> Source/WebCore/css/CSSPrimitiveValue.cpp:1144
> +        case CSS_WEBKIT_ELEM:
> +            text = "#" + String(m_value.string);

Shouldn't this include the -webkit-element()?

> Source/WebCore/css/CSSPrimitiveValue.h:137
> +        CSS_WEBKIT_ELEM = 116,

We normally avoid abbreviations. I would name this CSS_ELEMENT or CSS_ELEMENT_FUNCTION.

> Source/WebCore/css/CSSPrimitiveValue.h:189
> +    bool isWebKitElement() const { return primitiveType() == CSS_WEBKIT_ELEM; }

Nit: isElement.  The reason for dropping WebKit is that at some point we would drop the vendor prefix and it would just be element.

> Source/WebCore/css/WebKitCSSElementValue.cpp:42
> +    {
> +    }

Please left align.

> Source/WebCore/css/WebKitCSSElementValue.cpp:47
> +    {
> +    }

Please left align.

> Source/WebCore/css/WebKitCSSElementValue.cpp:52
> +String WebKitCSSElementValue::customCssText() const
> +    {
> +        return "-webkit-element(" + CSSValueList::customCssText() + ")";
> +    }

Remove extra indention.

> Source/WebCore/css/WebKitCSSElementValue.cpp:57
> +PassRefPtr<WebKitCSSElementValue> WebKitCSSElementValue::cloneForCSSOM() const
> +    {
> +        return adoptRef(new WebKitCSSElementValue(*this));
> +    }

Ditto.

> Source/WebCore/css/WebKitCSSElementValue.cpp:60
> +void WebKitCSSElementValue::reportDescendantMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const
> +    {

Ditto.

> Source/WebCore/css/WebKitCSSElementValue.h:45
> +    public:
> +        static PassRefPtr<WebKitCSSElementValue> create()
> +        {
> +            return adoptRef(new WebKitCSSElementValue());
> +        }

Indent is wrong. Please see the coding style guide: http://www.webkit.org/coding/coding-style.html

> Source/WebCore/css/WebKitCSSElementValue.h:55
> +        WebKitCSSElementValue(const WebKitCSSElementValue& cloneFrom);

Nit: explicit

> LayoutTests/ChangeLog:22
> +        * fast/css/webkit-element-parsing-expected.txt: Added.
> +        * fast/css/webkit-element-parsing-invalid-expected.txt: Added.
> +        * fast/css/webkit-element-parsing-invalid.html: Added.
> +        * fast/css/webkit-element-parsing.html: Added.

Nit: I would put these in a subdirectory so it's easy for ports that disable to feature to skip the whole directory.
Speaking of which, since you didn't enable on Chromium, I think you need to update platform/chromium/TestExpectations

> LayoutTests/fast/css/script-tests/webkit-element-parsing-invalid.js:3
> +description("Test the parsing of the -webkit-element function.");
> +
> +// These have to be global for the test helpers to see them.

You could inline the script-tests .js files into the html file.  There's no real benefit to having separate files for them.

> LayoutTests/fast/css/script-tests/webkit-element-parsing.js:54
> +                "#elementA");

Shouldn't the expected text be -webkit-element(#elementA)?

[Anish]

Created attachment 163180 [details]
CSS Element grammar and parsing patch. Not full implementation.

incorporated Tony Chang's review suggestions

[Benjamin Poulain]

Comment on attachment 163180 [details]
CSS Element grammar and parsing patch. Not full implementation.

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

Some comments + this looks a little light on testing.

> Source/WTF/ChangeLog:3
> +        Support parsing -webkit-element() CSS image type with ENABLE_CSS_ELEMENT

"typeÂ"

> Source/WTF/ChangeLog:5
> +        Since only grammar and parsing is implemented, only enables on MAC Plaform
> +        and tests are skipped on others.

Isn't this the description? This ChangeLog looks a bit messed up.

I don't understand why "only grammar and parsing is implemented" => "only enables on MAC Plaform".
You can have good reason to only enable this on Mac, but I don't get the relation with grammar && parsing.

> Source/WebCore/ChangeLog:9
> +        saves the string (#elementID) as CSS_ELEMENT_FUNCTION  CSSParser

"CSS_ELEMENT_FUNCTION  CSSParser" -> Missing period?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:21077
> +				F5493E6715EDA7AA0097FE55 /* CSSElementValue.cpp */,
> +				F5493E6815EDA7AA0097FE55 /* CSSElementValue.h */,
>  				FBD6AF8415EF21D4008B7110 /* BasicShapeFunctions.cpp */,

The file section should be sorted alphabetically by filename.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25541
>  				31B801BB15EC58C700CE643D /* LayoutTypesInlineMethods.h in Headers */,
> +				F5493E6A15EDA7AA0097FE55 /* CSSElementValue.h in Headers */,
>  				FBD6AF8815EF25C9008B7110 /* CSSBasicShapes.h in Headers */,

The build section should be sorted alphabetically by filename.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28634
>  				52CCA9E915E3F64C0053C77F /* DOMDOMNamedFlowCollection.mm in Sources */,
> +				F5493E6915EDA7AA0097FE55 /* CSSElementValue.cpp in Sources */,
>  				FBD6AF8915EF25DB008B7110 /* BasicShapeFunctions.cpp in Sources */,

Ditto.

> Source/WebCore/css/CSSElementValue.h:55
> +    CSSElementValue(const CSSElementValue& cloneFrom);

Not sure what this is fore. Did you mean to use WTF_MAKE_NONCOPYABLE?

> Source/WebCore/css/CSSPrimitiveValue.cpp:1145
> +            text = "#" + String(m_value.string);

"#" -> '#'?

[Anish]

CSSGrammar.y:
The '#' creates an IDSEL in CSSParser.cpp. IDSEL is parsed as a selector grammar in the file. For there to be a way to extract the value inside the function, I needed to create an IDSEL rule under term. Else it would fail the grammar by going into selector. Adding IDSEL catches the case where there is a '#' inside the function -webkit-element() and then creates the CSSPrimitiveValue CSS_ELEMENT_FUNCTION which stores #elementID for use by the styler and render.

platform.h:
I have restricted it to MAC because it is the platform I have been developing on and testing on. 

CSSPrimitiveValue.cpp:
It is just "#" because the "-webkit-element()" is derived from CSSElement Value. CSS_ELEMENT_FUNCTION just holds the parameters of the function (#elementID). I have to use double quotes or else it thinks '#' is an int.

CSSParser.cpp:
I followed a very similar approach as -webkit-image-set() when handling the parsing. It is only used as a background-image and those parsing methods handle background-image.

Tests:
Again, I took the example of the parsing tests of -webkit-image-set(). The expected text is '#elementA' because it checks to first ensure background-image's elementRule is a ValueList and then checks that its inner value is the #elementID. -webkit-image-set() follows the same process.

[Anish]

Created attachment 163229 [details]
CSS Element grammar and parsing patch. Not full implementation.

updated changelog and cleaned up code. See above comment for details about bigger changes

[Tony Chang]

Comment on attachment 163229 [details]
CSS Element grammar and parsing patch. Not full implementation.

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

Please send an email to webkit-dev explaining that you're going to be working on this feature.  See http://trac.webkit.org/wiki/AddingFeatures or you can use a previous email as a template (like the email about CSS Device Adaptation).

> Source/WTF/ChangeLog:8
> +        Reviewed by Tony Chang.

You are only supposed to fill this in after you get an r+. Most of the time, webkit-patch or bugzilla will do it for you.  Please restore this to Reviewed by NOBODY (OOPS!).

> Source/WebCore/ChangeLog:6
> +        Reviewed by Tony Chang.

Please restore this to Reviewed by NOBODY (OOPS!).

> Source/WTF/wtf/Platform.h:1055
> +#if PLATFORM(MAC)
> +#define ENABLE_CSS_ELEMENT 1
> +#endif

It's more common for new features to be disabled by default on all platforms until it is useful.  You can enable it during development by making sure that build-webkit --enable-css-element works. See Scripts/webkitperl/FeatureList.pm.

> Source/WebCore/css/CSSElementValue.cpp:40
> +CSSElementValue::CSSElementValue()
> +    : CSSValueList(CSSElementClass, SpaceSeparator)

Why does this inherit from CSSValueList? It looks like there can only be 1 ID.  I would probably inherit from CSSValue.

> Source/WebCore/css/CSSParser.cpp:7250
> +    RefPtr<CSSElementValue> webElem = CSSElementValue::create();

Nit: webElem -> cssElement

> Source/WebCore/css/CSSParser.h:218
> +    PassRefPtr<CSSValue> parseWebKitElement(CSSParserValueList*);

Nit: I would probably call this parseCSSElement.

> Source/WebCore/css/CSSPrimitiveValue.cpp:120
> +    case CSSPrimitiveValue:: CSS_ELEMENT_FUNCTION:

Nit: Remove the extra space after ::.

> Source/WebCore/css/CSSPrimitiveValue.cpp:1145
> +        case CSS_ELEMENT_FUNCTION:
> +            text = "#" + String(m_value.string);

This should be "-webkit-element(#" + String(m_value.string) + ")". That is, if you get the cssText, it should include the -webkit-element() part. See CSSImageSetValue::customCssText for how this works.

> LayoutTests/ChangeLog:6
> +        Reviewed by Tony Chang.

Please restore this to Reviewed by NOBODY (OOPS!).

> LayoutTests/ChangeLog:18
> +        * platform/efl/Skipped:
> +        * platform/qt/Skipped:

I think you need to skip the directory in platform/chromium/TestExpectations too.

> LayoutTests/fast/css/cssElement/webkit-element-parsing-invalid.html:15
> +function testInvalidImageSet(description, property, rule)

Nit: You probably don't mean ImageSet here.

> LayoutTests/fast/css/cssElement/webkit-element-parsing.html:34
> +function testelementRule(description, property, rule, expectedTexts)

Nit: testelementRule -> testElementRule

> LayoutTests/fast/css/cssElement/webkit-element-parsing.html:54
> +            subRule = elementRule[0].cssText;
> +            shouldBe("subRule", "'" + expectedTexts + "'");

I think part of the confusion is that you're inheriting from a CSSValueList.  If you don't inherit from list, then you don't have to use [0] and you can check elementRule.cssText directly, which should include -webkit-element().

[Tony Chang]

Also, can you add some tests that verify an ID like #fff gets parsed properly (rather than getting parsed as a color)?

[Anish]

The reason I chose CSSValueList instead of just CSSValue is for a few reasons. Currently, there are already set ways of extracting a CSSValue from a CSSValueList. For me to create webkit-element() as a value would require different means of extracting the elementID. The specification for this feature does not suggest what the type should be and seeing that the specification is still not mature, I thought it would be safer to stick with a CSSValueList incase CSS Element can take more than one argument. Since CSSParser is one small check to ensure only one ID, to allow multiple IDs would be a very simple change. Lastly, seeing how Mozilla implemented this feature as a CSSPrimitiveValue and the troubles they ran into (https://bugzilla.mozilla.org/show_bug.cgi?id=506826) made me want to deter from that. 
That is why I chose CSSValueList over CSSValue. What are your thoughts? I can change it if need be.

[Tony Chang]

(In reply to comment #19)
> The reason I chose CSSValueList instead of just CSSValue is for a few reasons. Currently, there are already set ways of extracting a CSSValue from a CSSValueList. For me to create webkit-element() as a value would require different means of extracting the elementID. 

That's just a single getter method.

> The specification for this feature does not suggest what the type should be and seeing that the specification is still not mature, I thought it would be safer to stick with a CSSValueList incase CSS Element can take more than one argument.

If the spec changes to allow multiple params, then we can switch the code to use a CSSValueList.  It's not clear that a list would be the best abstraction without knowing what the multiple values are.

> Lastly, seeing how Mozilla implemented this feature as a CSSPrimitiveValue and the troubles they ran into (https://bugzilla.mozilla.org/show_bug.cgi?id=506826) made me want to deter from that. 

Which comments on the bug are you looking at?  I'm not seeing the problem in the bug thread, but maybe I'm not understanding the moz code.

[Benjamin Poulain]

I think we should not land this.

From webkit-dev, it is not clear to me anyone will finish this feature in the short term.

This patch is simple enough to be rebased and integrated later as needed. On the other hand, it is intrusive enough to cause useless maintenance if we land it.

[Tony Chang]

(In reply to comment #21)
> I think we should not land this.
> 
> From webkit-dev, it is not clear to me anyone will finish this feature in the short term.
> 
> This patch is simple enough to be rebased and integrated later as needed. On the other hand, it is intrusive enough to cause useless maintenance if we land it.

I agree.  If someone else at Adobe plans on finishing the feature, s/he should upload the updated patch.

[Anish]

Created attachment 163510 [details]
Patch v1: Grammar & Parsing

Incorporated the suggestions by Tony Chang and made CSS Element a CSSPrimitiveValue instead of a CSSValueList. Looking at how CSS Variables handles its parsing and grammar, it made more sense to make it a CSSPrimitiveValue instead of a CSS Value (the usage of -webkit-var(foo) is a CSSPrimitiveValue whereas the setting of it is a CSSValue). Updated tests to test that #fff is an elementID and not a color and to look out for CSSPrimitiveValue. ENABLE_CSS_ELEMENT is no longer in Platform.h, it is a command line feature disabled by default. Please review this patch as whoever continues it will benefit greatly if not use this.

[Tony Chang]

Comment on attachment 163510 [details]
Patch v1: Grammar & Parsing

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

This change seems much better. I just have some style nits.

As mentioned earlier, I don't think we should land this change until someone is ready to finish implementing.  I'm going to remove the review flag to try and avoid confusion.

> Source/WebCore/ChangeLog:10
> +        CSSPrimitiveValue::CSS_ELEMENT_FUNCTION. Webkit-element is treated similarly to webkit-image-set 

Nit: Webkit-element -> -webkit-element

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:21216
> +				93CA4C9C09DF93FA00DF8677 /* maketokenizer */,

This seems like an unintentional change.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28715
> +				SYMROOT = bhayani/WebKit/WebKitBuild;

This seems like an unintentional change.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28724
> +				SYMROOT = bhayani/WebKit/WebKitBuild;

This seems like an unintentional change.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28733
> +				SYMROOT = bhayani/WebKit/WebKitBuild;

This seems like an unintentional change.

> Source/WebCore/css/CSSParser.cpp:7246
> +    if (arg->unit != CSSPrimitiveValue::CSS_ELEMENT_FUNCTION && arg->unit != CSSPrimitiveValue::CSS_PARSER_HEXCOLOR)

Nit: The CSS_PARSER_HEXCOLOR check probably deserves a comment.

> LayoutTests/fast/css/cssElement/webkit-element-parsing.html:48
> +    if (elementRule) {
> +            subRule = elementRule.cssText;
> +            shouldBe("subRule", "'" + expectedTexts + "'");

Nit: weird indent

[yisibl]

Is no one continuing to follow up on this feature?

I'm really looking forward to this feature. There is some movement over on the chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=108972 But it looks like the new Houdini stuff needs to be ironed out first.

This blog post shows some neat possibilities in the form of several demos: https://iamvdo.me/en/blog/css-element-function

[jonjohnjohnson]

In the previously linked blink issue (https://bugs.chromium.org/p/chromium/issues/detail?id=108972), there's talk of similarities for implementing and specifying backdrop-filter (https://github.com/w3c/fxtf-drafts/issues/53).

I'm really hoping element() can seamlessly fall into place for webkit, and all vendors, when backdrop-filter is nailed down.

[Sebastian Zartner]

For what it's worth, this function is now defined in CSS Images 4:
https://drafts.csswg.org/css-images-4/#element-notation

Sebastian