Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@chetbox
Copy link

@chetbox chetbox commented Nov 13, 2014

Some SVG editors and exporters do not include the <svg> "width" and "height" attributes.
e.g.

<svg version="1.1"
         xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
         x="0px"
         y="0px"
     viewBox="0 0 104 144">
...

SVGKit currently will not render these files. We found that Adobe Illustrator exports files like this if you check the "Responsive" option when exporting.

Additionally, some editors and exporters set percentages as their "width" and "height" values to indicate a relative size at which to display the image to a browser. e.g.

<svg version="1.1"
         xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
         x="0px"
         y="0px"
         width="100%"
         height="100%"
     viewBox="0 0 104 144">
...

SVGKit currently crashes when rendering these files. We've found that Inskscape saves files like this when saving as "SVG Plain".

This change ensures that:

  • Percentage widths and heights are ignored
  • If no width and height attributes are present, we simply grab the values from the viewBox attribute.

@adamgit
Copy link
Contributor

adamgit commented Nov 13, 2014

Off the top of my head, you cannot take width and height from viewbox -
IIRC its illegal.

I'm checking now in spec, but this stuff is subtle.

On 13 November 2014 17:06:12 Chetan Padia notifications@github.com wrote:

Some SVG editors and exporters do not include the <svg> "width" and
"height" attributes.
e.g.

<svg version="1.1"
         xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
         x="0px"
         y="0px"
   viewBox="0 0 104 144">
...

SVGKit currently will not render these files. We found that Adobe
Illustrator exports files like this if you check the "Responsive" option
when exporting.

Additionally, some editors and exporters set percentages as their "width"
and "height" values to indicate a relative size at which to display the
image to a browser. e.g.

<svg version="1.1"
         xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
         x="0px"
         y="0px"
         width="100%"
         height="100%"
   viewBox="0 0 104 144">
...

SVGKit currently crashes when rendering these files. We've found that
Inskscape saves files like this when saving as "SVG Plain".

This change ensures that:

  • Percentage widths and heights are ignored

  • If no width and height attributes are present, we simply grab the
    values from the viewBox attribute.
    You can merge this Pull Request by running:

    git pull https://github.com/chetbox/SVGKit
    2.x-infer-width-and-height-from-viewbox

Or you can view, comment on it, or merge it online at:

#237

-- Commit Summary --

  • ignore percentage 'width' and 'height' attributes on
  • infer 'width' and 'height' from viewBox
  • Merge branch '1.2.0-ignore-svg-percentage-width-and-height' into
    1.2.0-infer-width-and-height-from-viewbox
  • removed width parsing duplication
  • Merge branch '1.2.0-infer-width-and-height-from-viewbox' into
    2.x-infer-width-and-height-from-viewbox

-- File Changes --

M Source/DOM classes/SVG-DOM/SVGLength.h (1)
M Source/DOM classes/SVG-DOM/SVGLength.m (12)
M Source/DOM classes/Unported or Partial DOM/SVGSVGElement.m (83)

-- Patch Links --

https://github.com/SVGKit/SVGKit/pull/237.patch
https://github.com/SVGKit/SVGKit/pull/237.diff


Reply to this email directly or view it on GitHub:
#237

@adamgit
Copy link
Contributor

adamgit commented Nov 13, 2014

A good svg to test with is the world map - it has a negative view box origin (which is horrible, but legal!).

Also, do you have some test SVGs you can release as public domain and include in thedemo project to demonstrate your changes working? Make sure no one breaks this in future...

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt this be part of the existing method?

@adamgit
Copy link
Contributor

adamgit commented Nov 14, 2014

Going through this in detail, I like what you're trying to do, but I think there's a cleaner way.

  1. Add a method to SVGLength: "-(float) convertToPixelsUsingParentLength:(SVGLength) parent;". On absolute SVGLengths, this returns the existing 'convert-to-px'; on relative SVGLengths, it calculates the px of the parent SVGLength, and then multiplies by its relative amount.
  2. -- EDIT: actually, better method is one that takes an SVGElement as argument. That way we can isolate the "specific to SVGSVGElement" code from 3. below into this one method above. --
  3. Wherever we read the ".width" property of an SVGElement, we currently always assume it is absolute, and we crash / treat as 0 otherwise (which is incorrect!). Instead, we use the above method, and pass-in the SVGLength: "element.parent.width". In the special case of an SVGSVGElement, we override this and pass in "new SVGLength( element.viewBox.width )" (which might also be 0, but that's OK - in that case, we have no other options anyway!)

This way, we fix "relative" width / height everywhere, AND we keep the information about original viewport (which your patch currently discards), AND - I believe - the example SVG's you have will all be fixed.

...AND, as a bonus, other percentages for width/hegith on the root SVG will all "just work".

@chetbox
Copy link
Author

chetbox commented Nov 23, 2014

Looking at this in more detail there are two problems here:

  • If <svg> specifies width="X%" and/or `height="Y%" we see a crash because we're trying to convert to a pixel value which isn't possible.
  • If the root <svg> element does not specify a width or height attribute the SVG still renders correctly until you try to scale the image. i.e. After [svgkImage scaleToFitInside:rect] the image renders as a horizontal line.

It is trivial to create test cases:

  • For the former case above: Edit an existing SVG and set the width and height attributes of <svg> to 100%.
  • For the latter: Edit an existing SVG to remove the width and height attributes of <svg>. (This loads correctly.) Now Add a [loadedImage scaleToFitInside:CGRectMake(200, 200)] in DetailViewCotroller.m and it now renders incorrectly.

Both of these cases are perfectly valid SVG files as far as popular editors are concerned.

I've been trying to work through this and I can't quite work out how to structure it. Your instructions seem clear enough, but there are a few things that don't quite make sense to me.

  • SVGSVGElement.width and SVGSVGElement.height are SVGLengths. We need something that attempts to convert a relative SVGLength into a pixel SVGLength. I can't see how to create an absolute SVGLength without throwing away percentage information as you have suggested. Your suggestion of "new SVGLength( element.viewBox.width )" is what my svgLengthFromNumber is intended for. (There is no existing constructor to create a SVGLength from anything but a NSString.)
  • You second point above doesn't quite work unless I make width and height public on SVGSVGElement.

I believe all we need to do is:

  • If a percentage width or height has been supplied and there is no concrete parent unit type, ignore it. (Either when rendering or when parsing.)
  • If no absolute width or height is specified we must set it using the viewBox to ensure that scaling works correctly.

Any suggestions of how to proceed?

@adamgit
Copy link
Contributor

adamgit commented Nov 23, 2014

I think therrs a lot more than two problems. One fix can fix those AND several others.

Eg i believe we crash anywhere there is a % width or height.

Eg when svg width is a percentage, there are several cases where we should be calculating actual numbers - using the viewbox is actively wrong.

I'll have a look again later today, but I think we can retain percentage info easily enough.

@adamgit
Copy link
Contributor

adamgit commented Nov 23, 2014

I've made a personal branch / fork to play around with this:

https://github.com/adamgit/SVGKit/tree/FixPercentLengths

I've made a commit that does:

  1. Adds a draft version of the "convert any percentage-SVGLength into the corect absolute/pixels SVGLenght" (in SVGHelperUtilities)
  2. Alters SVGSVGElement so that instead of naively converting to pixels, and failing if it hits a percentage, it uses the above method to generate the "correct" absolute pixels
  3. Deleted some code at end of SVGSVGElement method that shouldn't have been there
  4. Added a method to convert SVGLength units into CSS primitive units (needed to make the first method above be correct)

As far as I can tell, this solves most of the above problem. What's missing:

  1. in "SVGLengthFromLengthInSVGTree", replace:
    1. NSAssert(FALSE, @"Intelligent code goes here... (do things like use the ViewBox to guess a width/height)");"
  2. ...with...
    1. The code proposed earlier in this thread
  3. AND: look at the CALayer generation code, and make sure that wherever it needs "pixel value" of width or height, it uses the new SVGLengthFromLengthInSVGTree method instead, to guarantee that percentage lengths are handled

It may also be that we need to edit the "transform" code - if Percentage SVGlengths are legal inside a transform, then we should use SVGLengthFromLengthInSVGTree there too.

@adamgit
Copy link
Contributor

adamgit commented Nov 23, 2014

(in case it's not clear - I'm suggesting you fork that fork, see if you can easily apply the fixes on top.

If so, you can PR to my fork, and I can then merge and PR back into SVGKit master.

Or ditto if anyone else wants to fix this)

Because: this is quite a complex fix, impacting a bunch of files, and I'm not sure we'll get it right first time. I don't want to mess around too much in SVGKit upstream with this.

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.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.