Bug 44650 - Support element() CSS image type
Summary: Support element() CSS image type
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Anish
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-08-25 16:57 PDT by Ojan Vafai
Modified: 2016-05-23 05:30 PDT (History)
47 users (show)

See Also:


Attachments
This sets the foundation for -webkit-element by adding it to the grammar, parser, and values (33.09 KB, patch)
2012-09-07 16:23 PDT, Anish
no flags Details | Formatted Diff | Diff
CSS Element grammar and parsing patch. Not full implementation. (33.78 KB, patch)
2012-09-10 12:23 PDT, Anish
no flags Details | Formatted Diff | Diff
CSS Element grammar and parsing patch. Not full implementation. (34.06 KB, patch)
2012-09-10 15:26 PDT, Anish
no flags Details | Formatted Diff | Diff
Patch v1: Grammar & Parsing (49.33 KB, patch)
2012-09-11 20:44 PDT, Anish
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-08-25 16:57:17 PDT
See http://hacks.mozilla.org/2010/08/mozelement/ for more info. Looks awesome IMO.
Comment 1 Anthony Ricaud 2011-08-25 08:37:50 PDT
FWIW, this has been added to a draft spec : http://dev.w3.org/csswg/css3-images/#element-reference
Comment 2 Simon Fraser (smfr) 2011-09-27 16:58:25 PDT
<rdar://problem/10197315>
Comment 3 Simon Fraser (smfr) 2011-10-28 11:35:42 PDT
Adam, can you think of any security issues in the CSS element() function? Could you add some notes here?
Comment 4 Adam Barth 2011-10-28 11:59:06 PDT
> 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.
Comment 5 Ojan Vafai 2011-10-28 12:04:40 PDT
(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.
Comment 6 Simon Fraser (smfr) 2011-10-28 12:34:10 PDT
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.
Comment 7 David Mulder 2011-12-12 07:48:00 PST
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).
Comment 8 Joseph Pearson 2012-01-31 18:34:31 PST
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
Comment 9 Pete Boere 2012-02-03 04:59:04 PST
Another use case is to create watermark backgrounds (view dabblet in firefox):

http://dabblet.com/gist/1729950
Comment 10 bop 2012-06-05 10:21:34 PDT
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).
Comment 11 Anish 2012-09-07 16:23:44 PDT
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
Comment 12 Tony Chang 2012-09-07 17:07:28 PDT
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)?
Comment 13 Anish 2012-09-10 12:23:02 PDT
Created attachment 163180 [details]
CSS Element grammar and parsing patch. Not full implementation.

incorporated Tony Chang's review suggestions
Comment 14 Benjamin Poulain 2012-09-10 14:00:06 PDT
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);

"#" -> '#'?
Comment 15 Anish 2012-09-10 15:14:52 PDT
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.
Comment 16 Anish 2012-09-10 15:26:15 PDT
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
Comment 17 Tony Chang 2012-09-10 16:04:44 PDT
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().
Comment 18 Tony Chang 2012-09-10 16:05:58 PDT
Also, can you add some tests that verify an ID like #fff gets parsed properly (rather than getting parsed as a color)?
Comment 19 Anish 2012-09-10 19:02:02 PDT
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.
Comment 20 Tony Chang 2012-09-11 10:25:25 PDT
(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.
Comment 21 Benjamin Poulain 2012-09-11 13:26:26 PDT
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.
Comment 22 Tony Chang 2012-09-11 13:47:57 PDT
(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.
Comment 23 Anish 2012-09-11 20:44:10 PDT
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.
Comment 24 Tony Chang 2012-09-12 16:19:25 PDT
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
Comment 25 yisibl 2016-05-23 05:30:18 PDT
Is no one continuing to follow up on this feature?