826114 - Support Filter Effects specification feColorMatrix "saturate" type range

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Dominic]

When using SVG feColorMatrix filters on HTML elements as described here (https://developer.mozilla.org/en-US/docs/CSS/filter), the "saturate" type only accepts values between 0 and 1. If a value outside of that range is entered, the entire element isn't rendered. See the example here: http://jsfiddle.net/dominic_p/wSbaR/.

According to the W3C spec (https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#feColorMatrixTypeAttribute) values outside of this range should over or under saturate the element.

I hope this helps.

Comment 1

[Robert Longson [:longsonr]]

The current SVG specification says 0-1 is the range: http://www.w3.org/TR/SVG/filters.html#feColorMatrixElement

The referenced document is an Editor's Draft of the proposed new version. We'll likely implement this at some point.

Comment 2

[Dominic]

@Robert, that's a good point. But, isn't the whole concept of SVG filters being applied in CSS an un-finalized spec at the moment? My thinking is: if Firefox is going to support an experimental feature, we should try to keep to the specs for that feature if possible. Does that make sense?

Comment 3

[Robert Longson [:longsonr]]

I'm not sure what you mean, we don't support CSS filters either.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> isn't the whole concept of SVG filters being applied in CSS an un-finalized spec at the
> moment?

Applying SVG filters via the "filter" CSS property is part of SVG 1.1.

Comment 5

[Robert Longson [:longsonr]]

So we support the first case of https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#FilterFunction because that's also in SVG 1.1 but not the others which are new.

Comment 6

[Dominic]

Ok, that makes sense. I never understood why only the first case of the filter functions was supported, but that makes perfect sense now.

If you ask me, support for the rest of the filter functions listed in the draft would be awesome, but that's probably a separate issue. :)

Comment 7

[javismiles]

I vote also for this change or enhancement, im currently having trouble because of this issue, as using a value of 2 or 3 in chrome creates strong saturation, but fails totally to render in firefox because it doesnt accept values over 1.
I can see that in firefox, 0 means no color=bw, and 1 means 100% saturation,  basically the normal color. But then how are we supposed to oversaturate or increase the color saturation of an image in firefox using this filter? we simply cant, we can only maintain the color or decrease its saturation, but there is no way to increase the saturation with this filter. 
Until there is a solution, if you know another way to increase the color saturation by using a different svg filter please let us know, thanks very much

Comment 8

[Cameron McCormack (:heycam)]

As https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#feColorMatrixValuesAttribute says values can be outside the range [0,1], we should allow this.  It should be a relatively straightforward fix in SVGFEColorMatrix.cpp: just remove the early exit if the saturation value is outside the range [0,1].  We already clamp the resulting colour values at the end of the function.

Comment 9

[javismiles]

Any news about this? Many of us find it rather absurd that we cannot allow users to increase the saturation applying SVG filters because Firefox is not supporting it; Consulting any designer will confirm the importance of allowing the increase of saturation to make colors more vivid, hopefully you can activate this soon, thank you

Comment 10

[javismiles]

This is the part that seems to require the change:
File SVGFEColorMatrixElement.cpp : 

  case SVG_FECOLORMATRIX_TYPE_SATURATE:

    if (values.Length() != 1)
      return NS_ERROR_FAILURE;

    s = values[0];

    if (s > 1 || s < 0)
      return NS_ERROR_FAILURE;


Seems the change would be:
    if (s < 0)
      return NS_ERROR_FAILURE;

Comment 11

[Robert Longson [:longsonr]]

If you follow the instructions here you should be able to attach a correctly formatted patch: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

We need a testcase too that's normally 2 files but can be only one if you can create something that displays entirely as lime with the fix and not-lime without. Otherwise if you could create 2 files one which displays the same always (the reference) and one which displays the same as the reference if this issue is fixed and differently if it isn't would be OK.

If you need any additional help feel free to ask.

Comment 12

[javismiles]

Dear Robert, thank you very much, thing is i have no experience with patches, Git, mozilla firefox code, c++ etc, i wish i could do what you are writing about, i would have done it yesterday, but i really can't, all i'm able to do is hopefully somebody else who is an experienced developer and interested in this fix can do it; Just to express it better, i go to the link you provide, and a phrase like this
"Here is a very easy way to get this setup working using mq.  First, you need to edit your ~/.hgrc file.  You only need to do this once.  Your hgrc file should have the following contents at least:"
sounds like chinese to me, no idea what any of that means :) mq, hgrc, etc, etc, really im not the right person to do that, very unfortunately (i wish i could :) )

Comment 13

[Robert Longson [:longsonr]]

Creating the reference and the testcase and attaching them would require no coding experience. How about tackling that and I'll do the rest.

Comment 14

[javismiles]

Of course, i'm happy to help, where can i find instructions for creating reference and test case? are they here also ? https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F  , if not send me instructions and i will be happy to do it

Comment 15

[Robert Longson [:longsonr]]

Create 2 SVG files. One (the testcase) which displays correctly only if this issue is fixed. The other (the reference) displays what the reference would show if the bug is fixed but does that whether or not the bug is fixed. So the reference must work around the bug in some way.

The most common way is to create a testcase that displays completely lime if the bug is fixed (see pass.svg) in which case we don't need a reference as we can reuse the pass.svg reference file.

There are hundreds of examples here:
http://mxr.mozilla.org/mozilla-central/find?text=&string=layout%2Freftests%2Fsvg

Comment 16

[javismiles]

DEar Robert,
thank you again for your support with this case,
I have created the testcase, these are the files

1) The reference as you suggested is the full lime color at
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/filters/pass.svg?force=1

2) The testcase is here:
http://volandino.com/tests/firefox/testcasefirefoxsaturate.html

A value of 11 is being applied over SVG saturation filter to the image

Running the testcase in google chrome generates the same Lime color than the reference
Running the testcase in firefox generates nothing because values of saturation over 1 just output nothing


In the link above the code contains a normal png image, but i also attach here the code of the testcase with base64 image integrated:

<!doctype html>
<html>
<head>
<meta charset="utf-8">
<title>TestCase OverSaturate, should obtain Lime color as in reference</title>

<svg width="0%" height="0%">
  <defs>

   <filter id="SaturateHigh">
      <feColorMatrix in="SourceGraphic" type="saturate" values="11" result="A"/>
   </filter>

  </defs>
</svg> 

<style>.base{filter:url(#SaturateHigh);-webkit-filter:url(#SaturateHigh)}</style>

</head>
<body>
<div class="base"><img src="data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAUDBAQEAwUEBAQFBQUGBwwIBwcHBw8LCwkMEQ8SEhEPERETFhwXExQaFRERGCEYGh0dHx8fExciJCIeJBweHx7/2wBDAQUFBQcGBw4ICA4eFBEUHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh7/wgARCACTASwDASIAAhEBAxEB/8QAFgABAQEAAAAAAAAAAAAAAAAAAAEF/8QAFwEBAQEBAAAAAAAAAAAAAAAAAAEEBv/aAAwDAQACEAMQAAABzhk4IAAAEBQAAAAAAAAAQFBAUAAAAAAAAAAAAAAABYQFAAAAABAUAEBQAAQFAAAAAABAUAAAAAAAAAAEBQAAAAAAAAAQFAAAAAAAAAAAAAAAAAAAAABAUAAAEBQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQFAAAAAAAAAAAAABAUBQgAQFAAAABLABQAAAAARQ//8QAFBABAAAAAAAAAAAAAAAAAAAAkP/aAAgBAQABBQI0v//EABQRAQAAAAAAAAAAAAAAAAAAAHD/2gAIAQMBAT8BPv/EABQRAQAAAAAAAAAAAAAAAAAAAHD/2gAIAQIBAT8BPv/EABQQAQAAAAAAAAAAAAAAAAAAAJD/2gAIAQEABj8CNL//xAAUEAEAAAAAAAAAAAAAAAAAAACQ/9oACAEBAAE/ITS//9oADAMBAAIAAwAAABD7777/AO+++++++++/+/8Avvvvvvvvvvvvvvvvr/vvvvvv/vv/AL77/wC++++++/8Avvvvvvvvvvv/AL777777777/AO+++++++++++++++++++++/++++/wDvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv/vvvvvvvvvvvvvv/AL6L7/777779/wC+++++/j//xAAUEQEAAAAAAAAAAAAAAAAAAABw/9oACAEDAQE/ED7/xAAUEQEAAAAAAAAAAAAAAAAAAABw/9oACAECAQE/ED7/xAAcEAEAAwACAwAAAAAAAAAAAAABABAgMYBBUFH/2gAIAQEAAT8Q6zPQp974yRjhyV9yww8whRbX/9k=" alt=""/></div>
</body>
</html>

Comment 17

[Robert Longson [:longsonr]]

Attachment: patch

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8362211 [details] [diff] [review]
patch

Switching the review to Markus, since he worked on this.

Comment 19

[javismiles]

thank you very much, sorry for asking as i'm new to these processes, what is next? is this likely to be fixed or we don't really know until it is reviewed and approved? just wondering, thank you again very much

Comment 20

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=8dc06ae80f1f

Comment 21

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01a501a19293

Comment 22

[javismiles]

question in response to previous comment (OS: Windows 7 → All), my app is running on Firefox Linux 64bits, Centos 6, Linux 64bit, will the patch apply as well to linux, in my case linux is the most critical OS i need to cover (windows as well, but linux even more because my live server is linux)

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/01a501a19293

Comment 24

[Robert Longson [:longsonr]]

javismiles: Given this landed more than 24 hours ago, it will have made it to nightly builds by now http://nightly.mozilla.org/ should you wish to check them out.

Comment 25

[javismiles]

thank you lots Robert, i just copied you also in this other bug in case you can give me a hand to make a patch also there, its for an upper font size limit, https://bugzilla.mozilla.org/show_bug.cgi?id=928449  , thank you again for all your support

Comment 26

[javismiles]

Robert, this patch , the saturation one, does it apply only to windows or also to Linux? cause i need to apply it to Linux, thanks lots

Comment 27

[Robert Longson [:longsonr]]

If you visit the link in comment 25 you can find out yourself ;-)

Comment 28

[Robert Longson [:longsonr]]

Comment 24 I mean.

Comment 29

[javismiles]

got you thank u Robert, so i see there is also linux there thats great,
could you also tell me whats the filename where this patch was applied in the source of Xulrunner?

i can see that the patch seems to be in
a/gfx/src/FilterSupport.cpp
     1.2 +++ b/gfx/src/FilterSupport.cpp
     1.3 @@ -318,17 +318,17 @@ ComputeColorMatrix(uint32_t aColorMatrix
etc

but i dont find a gfx/src/FilterSupport.cpp
in the Xulrunner source code,

reason i ask is because i have rebuilt xulrunner with a solution also to the font size issue and i may also try to do this change myself in the source of xulrunner, but i need to identify the file where the patch is applied

thanks lots

Comment 30

[Robert Longson [:longsonr]]

It's not in Xulrunner at all.

Comment 31

[javismiles]

that file may not be, but the code has to be,
reason is, i am currently using SlimerJS + Xulrunner, and i am applying SVG filters, and i am using Xulrunner, and i can apply saturate (<1) and all the other filters, so obviously the code for the SVG saturate filter is in Xulrunner, so i need to find what is the file in Xulrunner where similar comparison (<1) is being done

Comment 32

[Robert Longson [:longsonr]]

I'm afraid I don't know.

Comment 33

[javismiles]

no probs will search for it, thank you again for your help and support, thanks lots Robert