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

VERIFIED FIXED in mozilla35

Status

()

Core
Graphics
VERIFIED FIXED
3 years ago
11 months ago

People

(Reporter: ntim, Assigned: Max Vujovic)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla35
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 35+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

938 bytes, patch
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Now that CSS filters are about to land, I think we should consider enabling the layout.css.filters.enabled pref by default.
(Reporter)

Updated

3 years ago
Blocks: 869828

Comment 1

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

Comment 2

3 years ago
(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.
(Reporter)

Comment 3

3 years ago
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.

Updated

3 years ago
Depends on: 1057674
Keywords: dev-doc-needed
(Assignee)

Updated

3 years ago
Depends on: 1057900
We'll want to relnote this once it is enabled.
relnote-firefox: --- → ?
(Reporter)

Updated

3 years ago
Depends on: 1060220
(Reporter)

Updated

3 years ago
Depends on: 1060221
Comment hidden (obsolete)
Comment hidden (obsolete)

Updated

3 years ago
Blocks: 913153
Is there anything left to do before we do this? Sending an "intent to ship" message, maybe?
Flags: needinfo?(mvujovic)
(Assignee)

Comment 9

3 years ago
(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
(Reporter)

Comment 11

3 years ago
Created attachment 8488748 [details] [diff] [review]
Patch
(Assignee)

Comment 12

3 years ago
(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
(Assignee)

Updated

3 years ago
Depends on: 1066818
(Reporter)

Comment 13

3 years ago
(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.
(Assignee)

Comment 14

3 years ago
(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
(Assignee)

Updated

3 years ago
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)
(Assignee)

Comment 16

3 years ago
(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+
(Assignee)

Comment 18

3 years ago
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:".)
(Assignee)

Comment 21

3 years ago
(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.
(Assignee)

Updated

3 years ago
Depends on: 913990
(Assignee)

Comment 22

3 years ago
(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
(Assignee)

Comment 23

3 years ago
Created attachment 8499694 [details] [diff] [review]
Patch [v2]

Rebased Tim's patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/697c4b245de0
https://tbpl.mozilla.org/?tree=Try&rev=9e0647f42c14
Attachment #8488748 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/697c4b245de0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Updated

3 years ago
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"
relnote-firefox: ? → 35+
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
Keywords: dev-doc-needed → dev-doc-complete

Updated

3 years ago
Depends on: 1083241

Updated

3 years ago
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

Updated

2 years ago
Depends on: 1153845

Updated

2 years ago
Depends on: 1191995

Updated

11 months ago
Depends on: 1248900
You need to log in before you can comment on or make changes to this bug.