Bug 5966 - <image> does not preserveAspectRatio
Summary: <image> does not preserveAspectRatio
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/Graphics/SVG/Test/2...
Keywords: SVGHitList
Depends on:
Blocks: 5977 7219
  Show dependency treegraph
 
Reported: 2005-12-06 03:49 PST by Eric Seidel (no email)
Modified: 2006-03-03 08:19 PST (History)
2 users (show)

See Also:


Attachments
implementation (7.56 KB, patch)
2006-02-13 14:32 PST, Alexander Kellett
mjs: review-
Details | Formatted Diff | Diff
cleaned up patch (7.82 KB, patch)
2006-02-28 14:14 PST, Alexander Kellett
mjs: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-12-06 03:49:07 PST
<image> and <feImage> do not respect viewBox or preserveAspectRatio

This can be clearly seen by viewing the W3C <image> test case:
http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-coords-viewattr-01-b.html

This could be done using parts of khtml, if things were refactored properly so that this logic was pulled 
out of the layer.  This could also just be done manually in the kcanvas render tree w/o too much trouble.  
Some code is already written on the DOM side to calculate the proper localTransform to represent this 
transform/clip.
Comment 1 Eric Seidel (no email) 2006-01-26 14:55:23 PST
Bumping to P2 and adding to the SVGHitList.  This is a pretty basic feature of SVG images.  We would not ship SVG w/o this support.
Comment 2 Alexander Kellett 2006-02-13 14:32:11 PST
Created attachment 6468 [details]
implementation
Comment 3 Maciej Stachowiak 2006-02-20 23:25:07 PST
Alexander and I discussed this on IRC. Marking r- because the algorithm to scale/crop is confusing, incorrectly commented, duplicative, and possibly buggy in some cases. Alexander has good ideas on how to improve this though.
Comment 4 Alexander Kellett 2006-02-28 14:14:38 PST
Created attachment 6779 [details]
cleaned up patch
Comment 5 Eric Seidel (no email) 2006-02-28 14:22:24 PST
Comment on attachment 6779 [details]
cleaned up patch

 namespace WebCore {
+using namespace WebCore;

That seems unecessary.

Why not just pass 0?

+    int x = 0, y = 0;
+    if (!shouldPaint(paintInfo, x, y))

Effectively the same code for:
+                case SVG_PRESERVEASPECTRATIO_XMIDYMIN:
+                case SVG_PRESERVEASPECTRATIO_XMIDYMID:
+                case SVG_PRESERVEASPECTRATIO_XMIDYMAX:
+                    srcRect.setX(image()->width() / 2 - srcRect.width() / 2);
+                    break;


is repeated 4 times... Could we perhaps use a function there?

int newLength = calcAlignedLength(int, int, align)
srcRect.setX(newLength)

I think it's best to talk about this over IRC or at least get mjs to review it, given that he's looked at it once.
Comment 6 Maciej Stachowiak 2006-02-28 18:00:26 PST
Comment on attachment 6779 [details]
cleaned up patch

+        if (origDestWidth > (origDestHeight / widthToHeightMultiplier)) {
+            destRect.setWidth(origDestHeight / widthToHeightMultiplier);
+            switch(aspectRatio->align()) {
+                case SVG_PRESERVEASPECTRATIO_XMINYMID:
+                case SVG_PRESERVEASPECTRATIO_XMIDYMID:
+                case SVG_PRESERVEASPECTRATIO_XMAXYMID:
+                    destRect.setX(origDestWidth / 2 - destRect.width() / 2);
+                    break;
+                case SVG_PRESERVEASPECTRATIO_XMINYMAX:
+                case SVG_PRESERVEASPECTRATIO_XMIDYMAX:
+                case SVG_PRESERVEASPECTRATIO_XMAXYMAX:
+                    destRect.setX(origDestWidth - destRect.width());
+                    break;
+            }
+        }

I think this part is wrong, I think it should be grouping the switch blocks by XMID and XMAX, not YMID and YMAX. Shouldn't it? The other setX branch does that. Other than this apparent bug, looks fine.

r- for the bug.

I think you could consider refactoring it like Eric said but I don't believe this is required.
Comment 7 Darin Adler 2006-03-03 07:25:40 PST
What's going on with this bug?

There's a change log entry that mentions this bug number, but the bug is still open, and not marked FIXED.

And also svg/W3C-SVG-1.1/coords-viewattr-02-b.svg is showing up as a failure in layout tests. Is that because we fixed it and haven't updated the results?
Comment 8 Darin Adler 2006-03-03 07:27:56 PST
I'd really like to get layout tests passing again on the buildbot. Please see my question above.