A start on SVG "quirks mode" by tmickel · Pull Request #56 · scratchfoundation/scratch-render

tmickel · 2016-10-24T16:06:59Z

I definitely don't think this is complete, but hopefully it can provide some thinking for how to approach Scratch 2.0 quirks.

Example project:
https://scratch.mit.edu/projects/127228166/

Before "quirks mode":

After "quirks mode":

Differences:

Scratch 2.0 appears to ignore SVG width, height, and viewBox properties. Many SVGs exported from Scratch 2.0 appear to draw things in negative coordinates or otherwise outside the given viewBox, so we need to fix those in order to use the browser's SVG renderer. In this implementation, the bounds of the SVG are directly measured by asking the browser to draw it as a DOM element, and then taking getBBox measurements, before re-drawing the SVG again with corrected measurements. I tried to do this several different ways - this was the best way I found so far.
Transforms text in a bunch of ways - strips out x and y properties that Scratch 2.0 appears to set incorrectly and otherwise ignore, fixes non-standard line break treatment, and adds in fonts (which was actually quite tricky).

There is no doubt in my mind that this has bugs, is incomplete (I'm sure there are more quirks), and maybe these could be done better some other way. Would be happy to have more ideas!

# Conflicts: # src/Drawable.js

cwillisf

Nice work! The code looks pretty good -- I have one nitpick that could go either way. How many browsers have you tested this with?

I'm worried about license compatibility so I'm going to mark this as "Request changes" for the moment, but if it turns out that the licenses are in fact compatible then you can consider this approved.

Sign in to view

+
+                                 Apache License
+                           Version 2.0, January 2004
+                        http://www.apache.org/licenses/


Sign in to view

+        document.body.appendChild(svgSpot);
+        svgSpot.appendChild(this._svgTag);
+        // Take the bounding box, and then destroy the element.
+        let bbox = this._svgTag.getBBox();


cwillisf · 2016-10-26T18:39:30Z

@tmickel do you know of a way to tell (from code) that a particular SVG was created in the Scratch 2.0 style? I'm imagining a world where some of our SVGs require this special treatment but others can be render as-is... how do we tell the difference?

tmickel · 2016-10-31T17:17:32Z

@cwillisf Unfortunately I'm not sure if there's an easy way to accomplish what you're talking about. The Scratch 2.0 paint editor does insert a comment into SVGs, along the lines of . However, all current SVGs are rendered in Scratch 2.0 with these quirks, whether or not they've been processed through the 2.0 paint editor. It seems possible to me that people have adjusted their externally-edited/created SVGs to deal with the quirks - by trial and error, or whatever. So, I'd recommend that we render all vector graphics from 2.0 with this.

But, it would make sense to me for future assets and imports to use a different renderer (ideally a pure browser renderer that respects viewboxes and whatever, maybe). In that case we should probably explicitly mark new assets to distinguish when we can use a "standards mode" renderer? Any thoughts @thisandagain ?

thisandagain · 2016-10-31T18:19:50Z

Thanks for putting this together @tmickel . This looks like a great start.

I like the idea of tagging new SVGs so we can use a less "quirky" rendering approach and (hopefully) split with some of the pain that we see in 2.0:
scratchfoundation/scratch-flash#963
scratchfoundation/scratch-flash#1175
scratchfoundation/scratch-flash#1200
scratchfoundation/scratch-flash#1103
scratchfoundation/scratch-flash#411
scratchfoundation/scratch-flash#135

ed-cooper · 2016-10-31T21:27:13Z

How do you plan to handle the case for a SVG file being imported?

For example, an SVG file made in Inkscape or something similar would work best being rendered by the browser, but an SVG that had previously been downloaded from Scratch might work best in quirks mode.

My suggestion would be to treat all imported SVGs as normal SVGs (ie not quirks mode)

Also:

people have adjusted their externally-edited/created SVGs to deal with the quirks - by trial and error, or whatever.

I've been using Inkscape for SVGs for quite a while now. Whilst Scratch 2.0s support for more advanced SVG features (eg. shadows) is ... interesting, I never had any quirks mode problems myself

tmickel · 2016-10-31T22:14:52Z

@chooper100 Any SVG that could have previously been rendered by Scratch 2.0 would be rendered in quirks mode. That includes SVGs created by external tools, and SVGs created in the paint editor. Treating import-SVGs differently would no doubt lead to broken projects, because they are rendered in 2.0 with these quirks and possibly more. I'm not sure I understand what you're saying, I don't think these are treated differently in scratch-flash if they're made in external tools. Do you have an example of an SVG created in Inkscape, for example, where the viewBox is smaller than the graphic, and Scratch renders the clipped graphic?

cwillisf · 2016-11-01T14:31:46Z

My interpretation of @chooper100's comments:

When Scratch 3.0 loads a Scratch 2.0 project, the SVGs should render in "quirks mode"
When Scratch 3.0 imports an SVG independently, it should render without "quirks mode"
Is that what you had in mind, @chooper100?

I think we can continue to discuss when (and when not) to use Scratch 2.0 quirks-mode SVG rendering independently of this PR. As long as we want to do it in at least one case, we'll need some code like this :)

ed-cooper · 2016-11-02T23:15:42Z

@tmickel Yes, I meant what @cwillisf said

I've never had a problem where an Inkscape SVG has been clipped

tmickel · 2016-11-04T16:21:33Z

@cwillisf Comments addressed I think - any other thoughts?

By the way, I tested in Chrome, Firefox, and Safari. I didn't test in IE or Edge.

tmickel · 2016-11-04T16:37:53Z

Update:

In my VM, it looks like IE 11 wasn't even rendering SVGs before this pull request. Not sure why - there are no console complaints.
Edge was rendering SVGs before this pull request, but not after. Also no console complaints.

cwillisf

👍
For the record, my SVG code was broken in IE from the beginning. I'm curious what's up with Edge, but IMO it shouldn't hold up this PR.

CosmicWebServices · 2016-11-08T00:04:29Z

👏

* Start of an SVG rendering quirks mode * Provide font defaults in SVG quirks mode * Remove svg-to-image dependency * Remove fonts and use package fonts instead. * try/finally for getBBox

tmickel added 2 commits October 24, 2016 11:36

Start of an SVG rendering quirks mode

17745bb

Provide font defaults in SVG quirks mode

5edb35d

tmickel added the pr - needs review label Oct 24, 2016

tmickel assigned cwillisf Oct 24, 2016

tmickel mentioned this pull request Oct 24, 2016

SVGs with text appear not to render #28

Closed

tmickel added 2 commits October 24, 2016 13:20

Merge branch 'develop' into feature/quirks-mode

6e6d419

# Conflicts: # src/Drawable.js

Remove svg-to-image dependency

8f17aa9

tmickel modified the milestone: November 14 Oct 26, 2016

cwillisf requested changes Oct 26, 2016

View reviewed changes

tmickel added 2 commits November 4, 2016 12:04

Remove fonts and use package fonts instead.

828e7aa

try/finally for getBBox

8673b4f

cwillisf approved these changes Nov 7, 2016

View reviewed changes

cwillisf assigned tmickel and unassigned cwillisf Nov 7, 2016

cwillisf added pr - good to merge and removed pr - needs review labels Nov 7, 2016

tmickel merged commit b35f684 into scratchfoundation:develop Nov 7, 2016

tmickel deleted the feature/quirks-mode branch November 7, 2016 22:41

Conversation

tmickel commented Oct 24, 2016

Uh oh!

cwillisf left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

cwillisf commented Oct 26, 2016

Uh oh!

tmickel commented Oct 31, 2016

Uh oh!

thisandagain commented Oct 31, 2016

Uh oh!

ed-cooper commented Oct 31, 2016

Uh oh!

tmickel commented Oct 31, 2016

Uh oh!

cwillisf commented Nov 1, 2016

Uh oh!

ed-cooper commented Nov 2, 2016

Uh oh!

tmickel commented Nov 4, 2016

Uh oh!

tmickel commented Nov 4, 2016

Uh oh!

cwillisf left a comment

Choose a reason for hiding this comment

Uh oh!

CosmicWebServices commented Nov 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants