Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVG Gradient Generation Support #33

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mhemesath
Copy link

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.

@superstructor
Copy link
Contributor

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

@mhemesath
Copy link
Author

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!

@superstructor
Copy link
Contributor

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

@mhemesath
Copy link
Author

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?

@superstructor
Copy link
Contributor

  • 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 Added radial-gradient incl svg and canvas #12

Thanks!

@tj
Copy link
Collaborator

tj commented Aug 1, 2011

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

@mhemesath
Copy link
Author

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
Copy link
Author

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.

@superstructor
Copy link
Contributor

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
Copy link
Contributor

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.

@mhemesath
Copy link
Author

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
Copy link
Author

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.

@superstructor
Copy link
Contributor

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);
    }
  }));

@tj
Copy link
Collaborator

tj commented Aug 4, 2011

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

@mhemesath
Copy link
Author

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
Copy link
Author

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

@superstructor
Copy link
Contributor

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

@mhemesath
Copy link
Author

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.

@superstructor
Copy link
Contributor

Good points. I agree.

@mhemesath
Copy link
Author

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!

* Default vendor prefixes.
*/

gradient-formats = png svg css
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?=

only assigns if not already defined

@superstructor
Copy link
Contributor

@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
Copy link
Contributor

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 =)

@mhemesath
Copy link
Author

@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.

@igor10k
Copy link

igor10k commented Apr 8, 2013

No progress here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants