Remove redundant translation in SVG crop, add tests with SVG files

tests/test_svg_coordinate_transforms.py

@@ -1,10 +1,9 @@
 from functools import partial
 
 from willow.svg import (
-    get_viewport_to_user_space_transform,
-    transform_rect_to_user_space,
     SvgImage,
     ViewportToUserSpaceTransform,
+    get_viewport_to_user_space_transform,
 )
 
 from .test_svg_image import SvgWrapperTestCase
@@ -42,71 +41,6 @@ def test_preserve_aspect_ratio_none(self):
         transform = get_viewport_to_user_space_transform(svg)
         self.assertEqual(transform, ViewportToUserSpaceTransform(2, 1.25, 0, 0))
 
-    def test_transform_rect_to_user_space_translate_x(self):
-        svg_wrapper = partial(
-            self.get_svg_wrapper, width=100, height=50, view_box="0 0 25 25"
-        )
-        params = [
-            # transform = (2, 2, 0, 0)
-            ("xMinYMid meet", (10, 10, 20, 20), (5, 5, 10, 10)),
-            # transform = (2, 2, 25, 0)
-            ("xMidYMid meet", (10, 10, 20, 20), (-7.5, 5, -2.5, 10)),
-            # transform = (2, 2, 50, 0)
-            ("xMaxYMid meet", (10, 10, 20, 20), (-20, 5, -15, 10)),
-        ]
-        for preserve_aspect_ratio, rect, expected_result in params:
-            with self.subTest(preserve_aspect_ratio=preserve_aspect_ratio, rect=rect):
-                svg = SvgImage(svg_wrapper(preserve_aspect_ratio=preserve_aspect_ratio))
-                self.assertEqual(
-                    transform_rect_to_user_space(svg, rect), expected_result
-                )
-
-    def test_transform_rect_to_user_space_translate_y(self):
-        svg_wrapper = partial(
-            self.get_svg_wrapper, width=50, height=100, view_box="0 0 25 25"
-        )
-        params = [
-            # transform = (2, 2, 0, 0)
-            ("xMidYMin meet", (10, 10, 20, 20), (5, 5, 10, 10)),
-            # transform = (2, 2, 0, 25)
-            ("xMidYMid meet", (10, 10, 20, 20), (5, -7.5, 10, -2.5)),
-            # transform = (2, 2, 0, 50)
-            ("xMidYMax meet", (10, 10, 20, 20), (5, -20, 10, -15)),
-        ]
-        for preserve_aspect_ratio, rect, expected_result in params:
-            with self.subTest(preserve_aspect_ratio=preserve_aspect_ratio, rect=rect):
-                svg = SvgImage(svg_wrapper(preserve_aspect_ratio=preserve_aspect_ratio))
-                self.assertEqual(
-                    transform_rect_to_user_space(svg, rect), expected_result
-                )
-
-    def test_complex_user_space_origin_transform(self):
-        svg = SvgImage(
-            self.get_svg_wrapper(
-                width=100,
-                height=100,
-                view_box="-50 -50 100 100",
-                preserve_aspect_ratio="none",
-            )
-        )
-        self.assertEqual(
-            transform_rect_to_user_space(svg, (0, 0, 50, 50)),
-            (-50, -50, 0, 0),
-        )
-
-        svg = SvgImage(
-            self.get_svg_wrapper(
-                width=200,
-                height=200,
-                view_box="-50 -50 100 100",
-                preserve_aspect_ratio="none",
-            )
-        )
-        self.assertEqual(
-            transform_rect_to_user_space(svg, (0, 0, 50, 50)),
-            (-25, -25, 0, 0),
-        )
-
 
 class PreserveAspectRatioMeetTestCase(SvgWrapperTestCase):
     def test_portrait_view_box(self):

tests/test_svg_image.py

@@ -1,16 +1,17 @@
 import unittest
 from io import BytesIO
 from itertools import product
+from pathlib import Path
 from string import Template
 
 from defusedxml import ElementTree
 
-from willow.image import Image, SvgImageFile, BadImageOperationError
+from willow.image import BadImageOperationError, Image, SvgImageFile
 from willow.svg import (
-    SvgWrapper,
     InvalidSvgAttribute,
     InvalidSvgSizeAttribute,
     SvgImage,
+    SvgWrapper,
     ViewBox,
 )
 
@@ -266,7 +267,29 @@ def test_resize_throws(self):
         with self.assertRaises(BadImageOperationError):
             svg_image_file.resize((10, 0))
 
-    def test_crop(self):
+    def test_save_as_svg(self):
+        f = BytesIO(b'<svg width="13" height="13" viewBox="0 0 13 13"></svg>')
+        svg_image_file = Image.open(f)
+        svg = SvgImage(image=SvgWrapper(svg_image_file.dom))
+        written = svg.save_as_svg(BytesIO())
+        self.assertIsInstance(written, SvgImageFile)
+        self.assertEqual(written.dom.getroot().get("width"), "13")
+        self.assertEqual(written.dom.getroot().get("height"), "13")
+
+    def test_get_frame_count(self):
+        svg = SvgImage(self.get_svg_wrapper())
+        self.assertEqual(svg.get_frame_count(), 1)
+
+    def test_resize_preserves_original_image(self):
+        f = BytesIO(b'<svg width="42" height="42" viewBox="0 0 42 42"></svg>')
+        svg_image_file = Image.open(f)
+        svg = SvgImage.open(svg_image_file)
+        svg.resize((21, 21))
+        self.assertEqual(svg.get_size(), (42, 42))
+
+
+class SvgCropTestCase(SvgWrapperTestCase):
+    def test_simple_crop(self):
         f = BytesIO(b'<svg width="42" height="42" viewBox="0 0 42 42"></svg>')
         svg_image_file = Image.open(f)
         cropped = svg_image_file.crop((5, 5, 15, 15))
@@ -281,6 +304,79 @@ def test_crop(self):
             [5, 5, 10, 10],
         )
 
+    def test_crop_with_aspect_ratio_mismatch(self):
+        """
+        Test cropping SVGs where the viewport aspect ratio does not match the viewBox aspect ratio.
+        """
+
+        def get_original_crop_rect(svg_image):
+            # left, top, right, bottom (not left, top, width, height,
+            # like viewBox but the same in this case)
+            return (0, 0, svg_image.image.width, svg_image.image.height)
+
+        base_dir = Path("tests/images/svg")
+        x_mid_y_mid_meet = base_dir / "x_mid_y_mid_meet"
+        files = [
+            (
+                x_mid_y_mid_meet / "translated-y-no-scale.svg",
+                x_mid_y_mid_meet / "translated-y-no-scale-cropped-original.svg",
+                get_original_crop_rect,
+            ),
+            (
+                x_mid_y_mid_meet / "translated-y-2x-scale.svg",
+                x_mid_y_mid_meet / "translated-y-2x-scale-cropped-original.svg",
+                get_original_crop_rect,
+            ),
+            (
+                x_mid_y_mid_meet / "translated-x-no-scale.svg",
+                x_mid_y_mid_meet / "translated-x-no-scale-cropped-original.svg",
+                get_original_crop_rect,
+            ),
+            (
+                x_mid_y_mid_meet / "translated-x-2x-scale.svg",
+                x_mid_y_mid_meet / "translated-x-2x-scale-cropped-original.svg",
+                get_original_crop_rect,
+            ),
+            (
+                x_mid_y_mid_meet / "translated-y-no-scale.svg",
+                x_mid_y_mid_meet / "translated-y-no-scale-cropped-150-0-450-400.svg",
+                lambda _: (150, 0, 450, 400),  # vertical slice 0.5 width centered
+            ),
+            (
+                x_mid_y_mid_meet / "translated-y-no-scale.svg",
+                x_mid_y_mid_meet / "translated-y-no-scale-cropped-150-100-450-300.svg",
+                # crop to the blue-bordered square in the center
+                lambda _: (150, 100, 450, 300),
+            ),
+        ]
+        for original_file_path, expected_file_path, get_rect in files:
+            with self.subTest(
+                original_file_path=original_file_path,
+                expected_file_path=expected_file_path,
+            ):
+                with open(original_file_path, "rb") as f:
+                    original_file = Image.open(f)
+                original_file_path = SvgImage.open(original_file)
+                cropped = original_file_path.crop(get_rect(original_file_path))
+                with open(expected_file_path, "rb") as f:
+                    expected_file = Image.open(f)
+                expected_file_path = SvgImage.open(expected_file)
+                self.assertEqual(
+                    expected_file_path.image.view_box, cropped.image.view_box
+                )
+                self.assertEqual(expected_file_path.image.width, cropped.image.width)
+                self.assertEqual(expected_file_path.image.height, cropped.image.height)
+
+    def test_crop_preserves_original_image(self):
+        """
+        Cropping should create a new image, leaving the original untouched.
+        """
+        f = BytesIO(b'<svg width="42" height="42" viewBox="0 0 42 42"></svg>')
+        svg_image_file = Image.open(f)
+        svg = SvgImage.open(svg_image_file)
+        svg.crop((0, 0, 10, 10))
+        self.assertEqual(svg.image.view_box, ViewBox(0, 0, 42, 42))
+
     def test_crop_with_transform(self):
         # user coordinates scaled by 2 and translated 50 units south
         svg = SvgImage(
@@ -318,36 +414,6 @@ def test_crop_throws(self):
             # top == bottom
             svg_image_file.crop((0, 10, 10, 10))
 
-    def test_save_as_svg(self):
-        f = BytesIO(b'<svg width="13" height="13" viewBox="0 0 13 13"></svg>')
-        svg_image_file = Image.open(f)
-        svg = SvgImage(image=SvgWrapper(svg_image_file.dom))
-        written = svg.save_as_svg(BytesIO())
-        self.assertIsInstance(written, SvgImageFile)
-        self.assertEqual(written.dom.getroot().get("width"), "13")
-        self.assertEqual(written.dom.getroot().get("height"), "13")
-
-    def test_get_frame_count(self):
-        svg = SvgImage(self.get_svg_wrapper())
-        self.assertEqual(svg.get_frame_count(), 1)
-
-    def test_crop_preserves_original_image(self):
-        """
-        Cropping should create a new image, leaving the original untouched.
-        """
-        f = BytesIO(b'<svg width="42" height="42" viewBox="0 0 42 42"></svg>')
-        svg_image_file = Image.open(f)
-        svg = SvgImage.open(svg_image_file)
-        svg.crop((0, 0, 10, 10))
-        self.assertEqual(svg.image.view_box, ViewBox(0, 0, 42, 42))
-
-    def test_resize_preserves_original_image(self):
-        f = BytesIO(b'<svg width="42" height="42" viewBox="0 0 42 42"></svg>')
-        svg_image_file = Image.open(f)
-        svg = SvgImage.open(svg_image_file)
-        svg.resize((21, 21))
-        self.assertEqual(svg.get_size(), (42, 42))
-
 
 class SvgViewBoxTestCase(unittest.TestCase):
     def test_view_box_re(self):

willow/svg.py

@@ -69,26 +69,30 @@ def get_viewport_to_user_space_transform(
         translate = 0
         return ViewportToUserSpaceTransform(scale, scale, translate, translate)
 
-    aspect_ratio = svg.image.preserve_aspect_ratio.split()
+    preserve_aspect_ratio = svg.image.preserve_aspect_ratio.split()
     try:
-        align, meet_or_slice = aspect_ratio
+        align, meet_or_slice = preserve_aspect_ratio
     except ValueError:
-        align = aspect_ratio[0]
+        align = preserve_aspect_ratio[0]
         meet_or_slice = None
 
     scale_x = svg.image.width / view_box.width
     scale_y = svg.image.height / view_box.height
 
     if align == "none":
+        # if align is "none", the viewBox will be scaled non-uniformly,
+        # so we keep and use both scale_x and scale_y
         x_position = "min"
         y_position = "min"
     else:
         x_position = align[1:4].lower()
         y_position = align[5:].lower()
         choose_coefficient = max if meet_or_slice == "slice" else min
+        # all other values of preserveAspectRatio's `align' force
+        # uniform scaling, so choose the appropriate coefficient and
+        # use it for scaling both axes
         scale_x = scale_y = choose_coefficient(scale_x, scale_y)
 
-    # Translations to be applied after scaling
     translate_x = 0
     if x_position == "mid":
         translate_x = (svg.image.width - view_box.width * scale_x) / 2
@@ -273,40 +277,20 @@ def write(self, f):
         self.dom.write(f, encoding="utf-8")
 
 
-def transform_rect_to_user_space(svg: "SvgImage", rect):
-    # As well as scaling and translating the input rect to handle the
-    # preserveAspectRatio attribute, we need to account for the fact
-    # that the origin in the viewport coordinate space (i.e. as a
-    # viewer perceives the image) is not necessarily the same as
-    # the origin in the user coordinate space. For example, if we
-    # may have an SVG with a viewBox of (-8, -8, 10, 10), what the
-    # viewer perceives as (0, 0), is (-8, -8) in the user space.
-    left, top, right, bottom = rect
-    width = right - left
-    height = bottom - top
-    left += svg.image.view_box.min_x
-    top += svg.image.view_box.min_y
-    right = left + width
-    bottom = top + height
-
-    transform = get_viewport_to_user_space_transform(svg)
-    return transform((left, top, right, bottom))
-
-
 class SvgImage(Image):
     def __init__(self, image):
         self.image: SvgWrapper = image
 
     @Image.operation
-    def crop(self, rect, transformer=transform_rect_to_user_space):
+    def crop(self, rect, get_transformer=get_viewport_to_user_space_transform):
         left, top, right, bottom = rect
         if left >= right or top >= bottom:
             raise BadImageOperationError(f"Invalid crop dimensions: {rect}")
 
         viewport_width = right - left
         viewport_height = bottom - top
 
-        transformed_rect = transformer(self, rect) if callable(transformer) else rect
+        transformed_rect = get_transformer(self)(rect)
         left, top, right, bottom = transformed_rect
 
         svg_wrapper = copy(self.image)