nib icon indicating copy to clipboard operation
nib copied to clipboard

SVG Gradient Generation Support

Open mhemesath opened this issue 13 years ago • 24 comments

Here's a start on SVG support for gradient image generation.

Changes

  • Additional stylus method for linear-gradient-svg
  • Added SVG Gradient Examples (unfortunately has same quality as CSS)
  • Changed Gradient node to defer node-canvas calls until the data uri is generated. This way, the same object can be used for SVG and PNG images.

This is a rough start. Wasn't 100% confident on what direction to take this. Let me know what you think. Unfortunately, the quality is the same as CSS3 from what I could see in Chrome.

mhemesath avatar Jul 31 '11 01:07 mhemesath

Hi Mike,

Thanks for the code.

I think SVG support is important not so much for quality but for IE9 and Opera that do not have equivalent working CSS3 gradients. For <= IE8 I personally prefer PIE.htc and IE10 has the -ms- vendor prefixed gradients.

I did some initial SVG support some time ago for my radial gradients patch at https://github.com/visionmedia/nib/pull/12/files#L2R321 and https://github.com/visionmedia/nib/pull/12/files#L5R78. That codes more of a working proof of concept for radial gradients and not ready for merging though.

I also wasn't sure of the best way to go with it. My demo just renders the SVG string with stylus, then encodes it with an exported function as a bit of a nasty hack.

Using Jade to render the XML is an interesting idea. The only question is if the dependency on Jade is worth it for the amount of XML to generate.

Whatever the solution is I think the Gradient object needs to be broken up eventually, although that may be out of scope of this issue. The SVG mechanism should be the same for linear and radial gradients, and share common code if possible. Possibly something like Gradient object with LinearGradient and RadialGradient extending it. Also either with a deferred to ("composition") SVGGradient object or common SVG code in the Gradient object.

I also think web browser support options should determine if SVG is rendered or not, instead of calling multiple mixins. Deferring to mixins is fine, like the current linear gradient mixin defers to a canvas image mixin. Currently the web browser support options consists of "config" variables like support-for-ie and a vendor prefixes function. Not sure if that is sufficient or if we need more robust configuration handling yet. All I did for my demo was add a support-for-svg var.

Is #31 a duplicate of this issue ?

A heads up that you missed removing a debug console.log from gradient.js.

Great to see more contributions for nib so keep up the good work =) Look forward to hearing your thoughts on the points above.

Cheers Isaac

superstructor avatar Jul 31 '11 06:07 superstructor

Thanks for the feedback. Yeah I was on the fence with including Jade, but it was already a dev dependency, and I figured it cleaned up the XML rendering logic a bit so I included it. But yeah, you are right, it probably isn't worth including it.

I'm not so sure that browser support options should determine if SVG is rendered or not. Currently, PNG is higher quality than SVG. However, as SVG is a lossless format, it is the better option when the size has to be scaled to fit the content and that size is potentially large. For these reasons, I think that only the user can determine if SVG is an appropriate option, based on size, browsers, quality of the needed gradient.

The current gradient code was very much hard coded towards linear gradients and the canvas implementation, so I kinda wanted to just get the pull request out to get feedback on how the core contributors though this feature should be integrated in, or if at all.

Yeah, I noticed after I pushed the pull request I had the console.log in statement in SORRY! I was going to push a new commit tomorrow to remove it, but hadn't had time.

31 is a duplicate issue I guess. Github makes pull requests an issue. I logged issue 31 to see if anyone would comment on it for initial feedback or direction. I didnt' hear anything, so I just started coding.

As far as configuration goes, my opinion is that it would make more sense to specify when create specific gradients if you want them to be generated with CSS, PNG inline or SVG inline. Depending on the gradient and desired browser support, any one implementation could be optimal.

Thanks!

mhemesath avatar Jul 31 '11 07:07 mhemesath

Good points on browser support and configuration options. I think you are right since as you say SVG and PNG can both cover some browsers lacking support for CSS, only the user can decide which is optimal.

However it is configured the essential point is the user should only need to call a single top-level gradient mixin once per gradient definition to avoid duplicating gradient information (such as colors, stops etc).

Something similar to the vendor mixin (in vendor.styl) only and ignore parameters combined with a gradient-types list in config.styl (like vendor-prefixes) might be a good idea ? That should allow something like linear-gradient(top, yellow, blue, only: css svg) or the equivalent linear-gradient(top, yellow, blue, ignore: png).

Yep the current code is very much linear gradient and canvas oriented. It is likely you could get this merged before radial gradients which would put that issue out of scope of this changeset and my responsibility to refactor when I finally merge radial gradient support. When I do I'll aim to keep the mechanisms the same, share common code, etc.

Thanks

superstructor avatar Jul 31 '11 09:07 superstructor

Are multiple formats ever going to be needed? If I'm using PNG, I can't see a reason that CSS3 or SVG would be needed. Likewise, off the top of my head I think that every browser that supports SVG won't need CSS3 gradients in place. I think that the linear-gradient declaration should just take a single format to be used, with a default if none is specified.

linear-gradient(top, yellow, blue, svg)

So if thats cool, then I think we're landing on the following changes to this pull request (correct me if I'm wrong).

  1. Remove Jade dependency
  2. Reduce gradients to single top level call
  3. Remove console.log DOH

Also:

  • Going to move canvas check to the PNG data URI call so node-canvas isn't needed if SVG is used
  • Do we want to set the size onto the SVG gradient, or should I just have it scale to fit content as it currently is? Or should we set size if specified, else have it scale?

mhemesath avatar Jul 31 '11 15:07 mhemesath

  • Defaults: Yes good idea. I think it should default to the value of the list gradient-formats or gradient-types in config.styl or something similar.
  • Multiple formats: Yes I think they are needed as some might prefer fall-backs depending on support. Formats should always be output in the order PNG, then SVG, then CSS. Browsers with support will override PNG with SVG, and everything with CSS.

This is useful for cross-browser support e.g. IE6-8: PNG IE9: SVG Opera: SVG (for radial at least, Opera has CSS linear but only SVG radial) IE10, Mozilla, Webkit etc: CSS

Also I plan to add PIE.htc support soon, another common cross browser strategy might be the list "css svg pie" (note list should not infer any ordering of properties).

With the combination of a config list, only and ignore params it should be easy for a user to choose if they just want single formats or multiple formats ?

  • Keyword argument: Since there can be any number of color stops (variable arguments for stops) I suggest format options needs to be keyword argument(s) ("named parameter"). You could let it be put into the stops param then pull it out if it looked like a format but I don't personally think thats a good way to do it.

https://github.com/LearnBoost/stylus/blob/master/docs/kwargs.md https://github.com/LearnBoost/stylus/blob/master/docs/vargs.md

  • Jade dependency: Up to @visionmedia if a Jade dep is good or not, I don't have a strong opinion on it. Might be fine. It is an interesting idea.
  • Yes linear-gradient single top-tevel call that defers to linear-gradient-svg similar to the canvas implementation.
  • Canvas check: Yep you will need to change it but I suggest
    • keep a check in nib.js so you can conditionally export functions to stylus and for has-canvas
    • always require gradient.js
    • put a try/catch around the require for canvas at the top of gradient.js
    • if the png functions are not exported due to check in nib.js i don't think you'll need to do the check in the data uri call
  • Size: I think scaling to fit the content is the best option for now. For linear there is currently only one dimension specified. In some cases (not all) for radial gradients we do need knowledge of the box size, so for that its possible I'll specify the size of SVGs but don't worry about that for linear gradients. If your interested the suggested param format is discussed on #12

Thanks!

superstructor avatar Jul 31 '11 18:07 superstructor

cool I'll try and take a closer look soon thanks man

tj avatar Aug 01 '11 15:08 tj

Cool, if we are using keyword arguments, can I propose we also add opacity and a vector direction option?

I know in svg for sure it's trivial to create gradients that are not parallel to the x or y axis

mhemesath avatar Aug 01 '11 16:08 mhemesath

We might be able to apply a filter to the SVG gradient to give it a smoother appearance. The SVG gradients appearance only seems to be rough when the size is very small; such as when the gradient is applied to a button. However, from what I've seen, at those small sizes, the PNG images are smaller anyway, and look better so maybe its not worth it.

I'll investigate.

mhemesath avatar Aug 01 '11 21:08 mhemesath

can I propose we also add opacity and a vector direction option

Sure that would be cool. W3C gradient syntax also supports direction in degrees (moz, new webkit etc). Old webkit syntax doesn't.

It would probably also be possible to replicate for the PNGs, but feel free to leave that for later implementation.

It shouldn't be an issue that different formats have different levels of support for the extra options.

superstructor avatar Aug 03 '11 21:08 superstructor

Also I've been thinking more about the use of Jade. If its not too difficult moving the XML to an external linear-gradient.jade or similar file could be a very nice clean way to separate "SVG templates" from the other code.

superstructor avatar Aug 03 '11 21:08 superstructor

Yeah, I thought about that, but loading in the file would be an async operation. Should I just read in the jade file sync? The stylesheets should only be compiled in development so it shouldnt' be that big of a deal I wouldn't think.

Or is there a better way to do it?

mhemesath avatar Aug 03 '11 22:08 mhemesath

External templates is a good idea as when I get to the background noise generator.. I"ll have additional SVG templates for those filters as well.

mhemesath avatar Aug 03 '11 22:08 mhemesath

Yeah, I thought about that, but loading in the file would be an async operation. Should I just read in the jade file sync? The stylesheets should only be compiled in development so it shouldnt' be that big of a deal I wouldn't think.

Good point. I don't think you can do an async call in the methods that get called from stylus expecting a return value obviously.

Maybe you could load the template once uncompiled into a var when the JS is first loaded, which is before the stylus starts calling methods. Then just compile the template per call from stylus.

It is possible to compile nib-based stylus in production at runtime without ever storing the compiled CSS, but this is usually cached for performance reasons anyway. This is how I usually use nib. The code for Express/Connect is like:

  app.use(stylus.middleware({
      src: __dirname + '/public'
    , dest: __dirname + '/public'
    , compile: function(str, path) {
      return stylus(str)
        .include(require('nib').path)
        .set('filename', path)
        .set('warn', true)
        .set('compress', false);
    }
  }));

superstructor avatar Aug 03 '11 22:08 superstructor

yeah we can just load / compile the template and cache the function

tj avatar Aug 04 '11 16:08 tj

I think I'm leaning towards making the size be a named argument as well, being its only needed on 1 of the 3 formats.

mhemesath avatar Aug 05 '11 14:08 mhemesath

@superstructor have you had a chance to look at this yet?

mhemesath avatar Aug 10 '11 13:08 mhemesath

Thanks for the updates and sorry for the slow reply. Looking good. Having the SVG in an external file is nice.

build-gradient could use a clearer name although i'm not sure what that would be. Its a shame the gradient object has to be created separately for SVG and PNG but no big deal, and probably not an easy way to avoid it.

I think I'm leaning towards making the size be a named argument as well, being its only needed on 1 of the 3 formats.

If you do decide to do so it would be nice to maintain backwards compatibility with the current syntax.

Cheers

superstructor avatar Aug 10 '11 22:08 superstructor

So, I take back that comment on the 'size' named arg. I think it is fine the way it is. The gradient constructor just needs to be modified to not take a size. I think the size should be set on the gradient object only if one exists. I also think we should honor size on SVG images if it is passed in.

Scaling a SVG gradient to always fit the container is not always ideal. Take for example if I want to fade from light gray to solid white in 200px on the body. If the SVG always scales to content, I'd have to use a pseudo class to achieve that, when SVG can easily be limited to that size.

Only 1 dimension should suffice for the SVG image. The width of the image parallel to the gradient vector will be the length specified by size, and the perpendicular axis can scale to 100%. Of course, a gradient vector that is not parallel to the standard X/Y axis would throw this logic off a bit, but its just a thought.

mhemesath avatar Aug 11 '11 02:08 mhemesath

Good points. I agree.

superstructor avatar Aug 11 '11 09:08 superstructor

Ok, I pushed the updates I mentioned for sizing. This code should be good to go. I want to mention though, that combing the size and start make a huge headache. My vote is to completely move size to a named argument, which would simplify things a lot, but would be a non-passive change.

I apologize for the length of this pull request. Please let me know if you have any corrections or changes. Thanks guys!

mhemesath avatar Aug 12 '11 01:08 mhemesath

@visionmedia what can we do to help to get this merged ? Once this is done I can finish #12 which has been open for a year ;-)

Cheers

superstructor avatar May 11 '12 04:05 superstructor

This will need to be re-written on top of #94. I've closed #12 (radial) as will do a new pull request for that when #94 is finished.

If you don't have time to re-write this Mike @mhemesath I'm happy to take on both linear and radial SVG/canvas support.

Cheers =)

superstructor avatar May 16 '12 22:05 superstructor

@superstructor I may have time this weekend to get a start on this. I regularly use nib in a Rails project, so I should prioritize this.

mhemesath avatar May 17 '12 13:05 mhemesath

No progress here?

igor10k avatar Apr 08 '13 00:04 igor10k