Last Comment Bug 1057180 - Turn on CSS Filters by default (by enabling about:config pref)
: Turn on CSS Filters by default (by enabling about:config pref)
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
-- normal with 1 vote (vote)
: mozilla35
Assigned To: Max Vujovic
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 1191995 1248900 913990 1021564 1057674 1057900 1058776 1060220 1060221 1065606 1066818 1083241 1085267 1153845
Blocks: css3test 869828 1079719
  Show dependency treegraph
 
Reported: 2014-08-21 19:29 PDT by Tim Nguyen :ntim
Modified: 2016-07-18 10:58 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
35+


Attachments
Patch (917 bytes, patch)
2014-09-12 11:01 PDT, Tim Nguyen :ntim
dbaron: review+
Details | Diff | Splinter Review
Patch [v2] (938 bytes, patch)
2014-10-03 10:14 PDT, Max Vujovic
no flags Details | Diff | Splinter Review

Description User image Tim Nguyen :ntim 2014-08-21 19:29:57 PDT
Now that CSS filters are about to land, I think we should consider enabling the layout.css.filters.enabled pref by default.
Comment 2 User image Tim Nguyen :ntim 2014-08-22 04:01:21 PDT
(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.
Comment 3 User image Tim Nguyen :ntim 2014-08-22 05:46:40 PDT
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.
Comment 4 User image Markus Stange [:mstange] 2014-08-22 05:55:40 PDT
Absolutely, that's on the top of my todo list for next week.
Comment 5 User image Lawrence Mandel [:lmandel] (use needinfo) 2014-08-28 12:39:33 PDT
We'll want to relnote this once it is enabled.
Comment 6 User image Tim Nguyen :ntim 2014-08-28 23:12:06 PDT Comment hidden (Obsolete)
Comment 7 User image Tim Nguyen :ntim 2014-08-29 01:19:45 PDT Comment hidden (obsolete)
Comment 8 User image Markus Stange [:mstange] 2014-09-12 05:11:54 PDT
Is there anything left to do before we do this? Sending an "intent to ship" message, maybe?
Comment 9 User image Max Vujovic 2014-09-12 10:32:27 PDT
(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 :)
Comment 10 User image Markus Stange [:mstange] 2014-09-12 10:33:37 PDT
Oh right, I forgot about the regression.
Comment 11 User image Tim Nguyen :ntim 2014-09-12 11:01:43 PDT
Created attachment 8488748 [details] [diff] [review]
Patch
Comment 12 User image Max Vujovic 2014-09-12 11:18:42 PDT
(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.
Comment 13 User image Tim Nguyen :ntim 2014-09-17 04:00:30 PDT
(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.
Comment 14 User image Max Vujovic 2014-09-18 14:25:13 PDT
(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
Comment 15 User image David Baron :dbaron: ⌚️UTC-8 2014-09-22 13:27:59 PDT
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?
Comment 16 User image Max Vujovic 2014-09-22 14:29:54 PDT
(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 17 User image David Baron :dbaron: ⌚️UTC-8 2014-09-24 15:55:15 PDT
Comment on attachment 8488748 [details] [diff] [review]
Patch

Thanks for all the info.  That sounds good.  r=dbaron
Comment 18 User image Max Vujovic 2014-10-01 14:58:49 PDT
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
Comment 19 User image David Baron :dbaron: ⌚️UTC-8 2014-10-01 15:33:22 PDT
(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.
Comment 20 User image David Baron :dbaron: ⌚️UTC-8 2014-10-01 15:37:09 PDT
(I think if you want to test invalid URL behavior, you should use something more clearly invalid than "feed:".)
Comment 21 User image Max Vujovic 2014-10-01 15:58:48 PDT
(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.
Comment 22 User image Max Vujovic 2014-10-02 11:24:27 PDT
(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
Comment 24 User image Phil Ringnalda (:philor) 2014-10-05 09:39:45 PDT
https://hg.mozilla.org/mozilla-central/rev/697c4b245de0
Comment 25 User image Sylvestre Ledru [:sylvestre] 2014-10-09 04:24:55 PDT
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"
Comment 26 User image Jean-Yves Perrier [:teoli] 2014-10-15 02:14:14 PDT
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
Comment 27 User image Florin Mezei, QA (:FlorinMezei) 2014-12-11 08:16:17 PST
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.

Note You need to log in before you can comment on or make changes to this bug.