Fix bug around SVG dimensions

[dkotter]

Description of the Change

In PR #19, a bug was fixed around how we calculate the image dimensions of an SVG. Prior to that fix, we would always default to using the viewBox dimensions and not the width and height attributes, even though the latter were looked at first. The fix in #19 ensured we properly considered those attributes but this led to potential bugs when an SVG had different dimensions in the height and width attributes as compared to the viewBox attributes.

For instance, if an SVG had a viewBox of 0 0 32 32 and a width of 200 and height of 200, even though we look at height and width first, because of the bug, those values were never used and instead, 32x32 would be used as the SVG dimensions (and stored as image meta). With the bug fix introduced in #19, we now properly use the height and width attributes first but in this example, we still have 32x32 stored, so we end up with two different image dimensions. This can cause the SVG to use improper dimensions and be smaller or bigger than previous.

In addition, while researching this I found that this bug was introduced in #11, in an attempt to fix issues when the height or width attributes were percentages. That bug was brought back by the changes in #19.

This PR attempts to fix all of these issues:

• To maintain backwards compatibility, we now utilize the dimensions in the viewBox attribute first. My personal opinion here is that the height and width attributes are probably better to use as a default but that will unfortunately lead to backwards compatibility breaks now. I've introduced a new filter (safe_svg_use_width_height_attributes) that can be used to change that behavior for sites that want to reverse the default order here:
  Usage

add_filter( 'safe_svg_use_width_height_attributes', '__return_true' );

• I've added a helper method that checks the last value in a string. This is now used to check if the height or width attribute ends in a percent sign and if so, we don't use that value

Alternate Designs

We don't necessarily need to provide a filter here to change the default order of attribute usage but I figured it would be nice to allow users that option. We could just permanently swap the order and provide no way to change it.

We could also keep the order as-is and change the newly added filter to swap to using viewBox first. This would cause the same issues that were reported but our response to those users could be to use the new filter to fix that. I do personally think using height and width first makes the most sense but I don't like knowingly breaking user's sites.

Possible Drawbacks

None I'm aware of

Verification Process

Upload and output an SVG that has different values in the height and width attributes as compared to the viewBox attribute. With the filter in the default state, the resulting image tag should use the viewBox dimensions. Change the filter:

add_filter( 'safe_svg_use_width_height_attributes', '__return_true' );

and now the resulting image tag should use the other attribute values

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests passed.

Changelog Entry

Added - New filter, safe_svg_use_width_height_attributes, that can be used to change the order of attributes we use to determine the SVG dimensions
Fixed - Use the viewBox attributes first for image dimensions. Ensure we don't use image dimensions that end with percent signs

[peterwilsoncc]

LGTM

Added a note about the hook docs but feel free to correct me if OSP use the WP coding standards.

[dkotter]

Added a note about the hook docs but feel free to correct me if OSP use the WP coding standards.

I'd prefer to use the WP coding standards but I had forgotten that in order to generate docs using https://github.com/matzeeable/wp-hookdoc, you need to follow the JSDoc standard (unless there's a workaround here I'm not aware of)

[peterwilsoncc]

Thanks Darin, the only alternative I am aware of is to use https://github.com/WordPress/phpdoc-parser. That's not so much a workaround as different library.

[cadic]

LGTM!