Closed Bug 826114 Opened 11 years ago Closed 10 years ago

Support Filter Effects specification feColorMatrix "saturate" type range

Categories

(Core :: SVG, enhancement)

Product:
Core
Component:
SVG
Version:
17 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: solaveritasteliberum, Assigned: longsonr)

Details

Attachments

(1 file)

patch
2.27 KB, patch
mstange
: review+
Details | Diff | Splinter Review 
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.
Component: Style System (CSS) → SVG
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.
Summary: Limited Support for SVG feColorMatrix "saturate" type → Support Filter Effects specification feColorMatrix "saturate" type range
@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?
I'm not sure what you mean, we don't support CSS filters either.
> 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.
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.
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. :)
Severity: normal → enhancement
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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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;
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.
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 :) )
Creating the reference and the testcase and attaching them would require no coding experience. How about tackling that and I'll do the rest.
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
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
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>
Attached patch patch — — Splinter Review 
Assignee: nobody → longsonr

        Attachment #8362211 -
        Flags: review?(matt.woodrow)

    
    

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

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

        Attachment #8362211 -
        Flags: review?(matt.woodrow) → review?(mstange)

    
    

    
  
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

        Attachment #8362211 -
        Flags: review?(mstange) → review+

    
  
OS: Windows 7 → All
Hardware: x86_64 → All

    
    

    
  
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)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/01a501a19293
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
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

    
    

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

    
    

    
  
Comment 24 I mean.

    
    

    
  
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

    
    

    
  
It's not in Xulrunner at all.

    
    

    
  
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

    
    

    
  
I'm afraid I don't know.

    
    

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

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