Closed Bug 1057180 Opened 10 years ago Closed 10 years ago

Turn on CSS Filters by default (by enabling about:config pref)

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
relnote-firefox --- 35+

People

(Reporter: ntim, Assigned: mvujovic)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Now that CSS filters are about to land, I think we should consider enabling the layout.css.filters.enabled pref by default.
Blocks: 869828
(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.
Depends on: 1021564
Absolutely, that's on the top of my todo list for next week.
Depends on: 1057674
Depends on: 1057900
We'll want to relnote this once it is enabled.
relnote-firefox: --- → ?
Depends on: 1060220
Depends on: 1060221
Blocks: css3test
Is there anything left to do before we do this? Sending an "intent to ship" message, maybe?
Flags: needinfo?(mvujovic)
(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 :)
Flags: needinfo?(mvujovic)
Oh right, I forgot about the regression.
Depends on: 1065606
Attached patch Patch (obsolete) — Splinter Review
(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.
Depends on: 1058776
Depends on: 1066818
(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
Attachment #8488748 - Flags: review?(dbaron)
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?
Assignee: nobody → mvujovic
Flags: needinfo?(mvujovic)
(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
Flags: needinfo?(mvujovic)
Comment on attachment 8488748 [details] [diff] [review]
Patch

Thanks for all the info.  That sounds good.  r=dbaron
Attachment #8488748 - Flags: review?(dbaron) → review+
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.
Depends on: 913990
(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
https://hg.mozilla.org/mozilla-central/rev/697c4b245de0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Blocks: 1079719
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"
Depends on: 1083241
Depends on: 1085267
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.
Status: RESOLVED → VERIFIED
Depends on: 1153845
Depends on: 1191995
Depends on: 1248900
Depends on: 1408841
See Also: → 1408841
You need to log in before you can comment on or make changes to this bug.