Now that CSS filters are about to land, I think we should consider enabling the layout.css.filters.enabled pref by default.

Mybie not yet? https://dl.dropboxusercontent.com/u/3779856/golebaby/private/screeny/Zrzut%20ekranu%20z%202014-08-22%20o%2012.30.48.png

(In reply to Kuba Niewiarowski from comment #1) > Mybie not yet? > https://dl.dropboxusercontent.com/u/3779856/golebaby/private/screeny/ > Zrzut%20ekranu%20z%202014-08-22%20o%2012.30.48.png Bug 948265 hasn't landed yet.

Bug 1021564 also affects the CSS filters. I think this is something we definitively want to fix before we go on and enable the CSS filters.

Absolutely, that's on the top of my todo list for next week.

We'll want to relnote this once it is enabled.

Is there anything left to do before we do this? Sending an "intent to ship" message, maybe?

(In reply to Markus Stange [:mstange] from comment #8) > Is there anything left to do before we do this? Sending an "intent to ship" > message, maybe? I just need to land the two patches you reviewed recently- one to fix a regression, one to add a test. Otherwise, I don't think we need any other changes. I'll look into sending an intent to ship :)

Oh right, I forgot about the regression.

(In reply to Tim Nguyen [:ntim] from comment #11) > Created attachment 8488748 [details] [diff] [review] > Patch Thanks for the patch, Tim. I'll send an intent to ship after 1065606 and 1058776 land on mozilla-central.

(In reply to Max Vujovic from comment #12) > (In reply to Tim Nguyen [:ntim] from comment #11) > > Created attachment 8488748 [details] [diff] [review] > > Patch > > Thanks for the patch, Tim. I'll send an intent to ship after 1065606 and > 1058776 land on mozilla-central. No problem ;) Since the regressions are fixed, I think we can ship this.

(In reply to Tim Nguyen [:ntim] from comment #13) > No problem ;) Since the regressions are fixed, I think we can ship this. Sounds good to me. I started an intent to ship thread, and there's been some discussion [1]. Not sure what happens next- Maybe ask dbaron and roc for a review on the patch? [1]: https://groups.google.com/forum/#!topic/mozilla.dev.platform/ujWvBvtugGY

A few questions: 1. which part of http://dev.w3.org/fxtf/filters/ are you actually talking about turning on? 2. how complete is our test coverage in automated testing? 3. what are the known bugs and known missing features? Or, in other words, which part of the answer to (1) do we not implement?

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #15) > A few questions: > > 1. which part of http://dev.w3.org/fxtf/filters/ are you actually talking > about turning on? I’m talking about accepting the entire syntax for the CSS filter property: http://dev.w3.org/fxtf/filters/#FilterProperty Right now, Firefox accepts a small subset of the syntax for the filter property: filter: [ <url> | none ] After turning this on, Firefox will accept the full syntax for the CSS filter property: filter: [ <filter-function-list> | none ] <filter-function-list> = [ <filter-function> | <url> ]+ <filter-function> = [ <blur()> | <brightness()> | <contrast()> | <drop-shadow()> | <grayscale()> | <hue-rotate()> | <invert()> | <opacity()> | <sepia()> | <saturate()> In other words Firefox will now accept: (a) All of the filter shorthand functions. (b) Multiple SVG filter <url>s or filter shorthand functions chained together. It will render and animate those according to spec. > 2. how complete is our test coverage in automated testing? I’m not sure how we measure test coverage in Gecko. Subjectively, I believe test coverage is very good. For a second opinion, Markus is familiar with our coverage. The rendering of individual shorthand filter functions is tested under layout/reftests/svg/filters/css-filters. Various normal and extreme values are tested. The rendering of chained shorthand filter functions is tested under .../css-filter-chains. The rendering of chained SVG filter <url>s is tested under .../svg-filter-chains. I’ve covered all the tricky cases I can think of there, mostly related to filter region clipping. The rendering of shorthand filter functions and SVG filter <url>s chained together is tested under .../css-svg-filter-chains. This also covers tricky cases related to filter region clipping. We also have parsing and animation tests. > 3. what are the known bugs and known missing features? Or, in other words, > which part of the answer to (1) do we not implement? There are no known incorrectness bugs with the new features we’re turning on (filter shorthand functions and filter chaining). In general, filter performance could benefit from hardware acceleration on all platforms (bug 869496). Currently, filters are only accelerated on Windows (using D2D). There are other parts of the Filter Effects spec we continue to not support: (a) In SVG filters, which we’ve been shipping for a long while, we don't support the “BackgroundImage” [1] and “BackgroundAlpha” [2] keywords as input to a filter primitive. (b) We don’t support the filter() function for CSS image values [3]. This is a fairly stand-alone feature that can be considered and shipped independently. [1]: http://dev.w3.org/fxtf/filters/#valdef-in-backgroundimage [2]: http://dev.w3.org/fxtf/filters/#valdef-in-backgroundalpha [3]: http://dev.w3.org/fxtf/filters/#FilterCSSImageValue

Comment on attachment 8488748 [details] [diff] [review] Patch Thanks for all the info. That sounds good. r=dbaron

I ran the patch through the try bots [1] and it looks good except for an error only on B2G regarding a filters parsing test: 4814 INFO TEST-UNEXPECTED-PASS | /tests/layout/style/test/test_value_computation.html | should not get initial value for 'filter:url('feed:javascript:5')' on elementn. - url("feed:javascript:5") should not equal none I'm not sure exactly what this result means and why it's only happening on B2G. Regarding the test case, 'filter:url('feed:javascript:5')' is supposed to parse correctly (without crashing the browser, for example), and it's supposed to return 'none' for getComputedStyle ('none' is the filter property's initial value). This is similar to the other tests beside it in test_value_computation.html in the gBadComputed hash [2]. I believe these tests, which are intended to result in a "bad"/initial computed style, use Mochitest's "todo" test functions, which I read can trigger Tinderbox to make noise when they start passing [3]. Though, if "passing" means 'filter:url('feed:javascript:5')' is returning something other than "none" for getComputedStyle, that's not good. I'm trying to get a B2G build to reproduce this, but my connection is slow today. I'll probably have more success tomorrow. If someone has any ideas, please feel free to share :) [1]: https://tbpl.mozilla.org/?tree=Try&rev=1460572ed4d5 [2]: http://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_value_computation.html#43 [3]: https://developer.mozilla.org/en-US/docs/Mochitest

(In reply to Max Vujovic from comment #18) > Regarding the test case, 'filter:url('feed:javascript:5')' is supposed to > parse correctly (without crashing the browser, for example), and it's > supposed to return 'none' for getComputedStyle ('none' is the filter > property's initial value). If that's the case, then it should be listed in the initial_values section in property_database.js instead of the other_values section. But Firefox has a feed: protocol handler at http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/FeedConverter.js#565 , which might mean that in Firefox it's not being treated as 'none'; this handler probably isn't present for B2G.

(I think if you want to test invalid URL behavior, you should use something more clearly invalid than "feed:".)

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #19) > (In reply to Max Vujovic from comment #18) > > Regarding the test case, 'filter:url('feed:javascript:5')' is supposed to > > parse correctly (without crashing the browser, for example), and it's > > supposed to return 'none' for getComputedStyle ('none' is the filter > > property's initial value). > > If that's the case, then it should be listed in the initial_values section > in property_database.js instead of the other_values section. That makes a lot more sense. Thanks! > > But Firefox has a feed: protocol handler at > http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/ > FeedConverter.js#565 , which might mean that in Firefox it's not being > treated as 'none'; this handler probably isn't present for B2G. Sounds likely. Thanks for the pointer. (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #20) > (I think if you want to test invalid URL behavior, you should use something > more clearly invalid than "feed:".) Agreed. This test is intended to test exactly that type of invalid URL, though. IIRC, "feed:javascript:5" parses correctly, but doesn't result in a valid nsURL object. Most invalid URL tests would fail at parsing. The test case is from bug 913990, where a null nsURL pointer from a correctly parsed URL was causing an assertion.

(In reply to Max Vujovic from comment #21) > (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from > comment #19) > > (In reply to Max Vujovic from comment #18) > > > Regarding the test case, 'filter:url('feed:javascript:5')' is supposed to > > > parse correctly (without crashing the browser, for example), and it's > > > supposed to return 'none' for getComputedStyle ('none' is the filter > > > property's initial value). > > > > If that's the case, then it should be listed in the initial_values section > > in property_database.js instead of the other_values section. > > That makes a lot more sense. Thanks! When I did this, the results were more clear on B2G. B2G returns the original filter string instead of none for the computed style. I think we should turn this computed style test into a crashtest, since that's what we're really interested in. I reopened bug 913990 and commented there [2]. [1]: https://tbpl.mozilla.org/?tree=Try&rev=4a7200fb1b9d [2]: https://bugzilla.mozilla.org/show_bug.cgi?id=913990#c31

Rebased Tim's patch. https://hg.mozilla.org/integration/mozilla-inbound/rev/697c4b245de0 https://tbpl.mozilla.org/?tree=Try&rev=9e0647f42c14

Added to the 35 release notes with "CSS filters enabled by default" as wording. CSS filters pointing to "https://developer.mozilla.org/en-US/docs/Web/CSS/filter"

Doc updated: https://developer.mozilla.org/en-US/docs/Web/CSS/filter (compatibility info at the bottom) and https://developer.mozilla.org/en-US/Firefox/Releases/35

layout.css.filters.enabled is "true" by default on Firefox 35 Beta 2 (BuildID: 20141208150535) on Windows 7 x64, Mac OS X 10.9.5, Ubuntu 12.04 x86. Given that this feature has automated tests and additional manual testing was performed in bug 948265, I think we can call this Verified.