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

Properly handle Scratch 2.0's <image> behavior... but at what cost? #88

Closed

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Apr 27, 2019

Resolves

Resolves #58 (at least for PNGs)

Proposed Changes

This PR adds a new step to the Scratch 2.0 compatibility mode path that strips the "x" and "y" attributes from <image>s, and sets their "width" and "height" to their actual resolution.

Currently, in order to keep SvgRenderer.fromString() synchronous, this step only operates on PNG images, reading their width and height directly from their binary data*.

A much more robust solution would be to create an <img> element for each SVG <image>, and read the width and height from there. However, this only works if you read the dimensions after the <img> has loaded, which you need an event callback to do. This would make SvgRenderer.fromString() an asynchronous function, which has implications for the rest of the Scratch codebase.

Reason for Changes

Scratch 2.0 ignores <image>s' "x", "y", "width", and "height", only recognizing the "transform" attribute.

*it's not an ugly hack if it's well-commented, amirite?

@adroitwhiz adroitwhiz changed the title Properly handle Scratch 2.0's <image> behavior Properly handle Scratch 2.0's <image> behavior... but at what cost? Apr 27, 2019
@towerofnix
Copy link

@thisandagain You closed this without saying the reason?

@adroitwhiz
Copy link
Contributor Author

He probably didn't want to entertain the sheer J A N C C of my solution

@towerofnix
Copy link

Such horrifying code it put him into a state of denial that the PR had ever been created! :'D

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.

Large embedded <image>s are shrunk
3 participants