RESOLVED WONTFIX 75601
[Chromium] ImageDocument should have a viewport width equal to the image's instrinsic width 
https://bugs.webkit.org/show_bug.cgi?id=75601
Summary [Chromium] ImageDocument should have a viewport width equal to the image's in...
Fady Samuel
Reported 2012-01-04 21:42:01 PST
[Chromium] ImageDocument should have a viewport width equal to the image's instrinsic width
Attachments
Patch (4.08 KB, patch)
2012-01-04 21:42 PST, Fady Samuel 		no flags
DetailsFormatted DiffDiff
Patch (9.17 KB, patch)
2012-01-05 08:03 PST, Fady Samuel 		eric: review-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Fady Samuel
Comment 1 2012-01-04 21:42:20 PST
Created attachment 121213 [details] Patch
Fady Samuel
Comment 2 2012-01-04 21:43:45 PST
This is not the final version of the patch, I plan to move some of this code to ChromeClientImpl but I thought I'd upload a working version now to see if this is along the lines of what people wanted. Thanks.
Grace Kloba
Comment 3 2012-01-04 23:14:50 PST
Comment on attachment 121213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121213&action=review > Source/WebCore/html/ImageDocument.cpp:211 > + rootElement->appendChild(body, ec); Any reason change this? > Source/WebCore/html/ImageDocument.cpp:299 > + viewport->setAttribute(contentAttr, String::format("width=%d", imageSize.width())); Shouldn't metatag be the child of <head>? Should we make sure that this is only added once?
Fady Samuel
Comment 4 2012-01-05 08:02:49 PST
Comment on attachment 121213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121213&action=review >> Source/WebCore/html/ImageDocument.cpp:299 >> + viewport->setAttribute(contentAttr, String::format("width=%d", imageSize.width())); > > Shouldn't metatag be the child of <head>? > > Should we make sure that this is only added once? imageUpdated seems to be guarded by m_imageSizeIsKnown and so this should be called only once but I'm not 100% sure. I've uploaded an updated patch that moves this code to ChromeClientImpl. Also, this works, so I'm not sure what the value is of adding a head tag to an internal document type? The functionality would not change.
Fady Samuel
Comment 5 2012-01-05 08:03:11 PST
Created attachment 121279 [details] Patch
Fady Samuel
Comment 6 2012-01-05 10:35:46 PST
I'm not sure who to get to review this patch. Added fishd.
Eric Penner
Comment 7 2012-01-05 17:31:46 PST
Can you briefly explain why we want the viewport to be the size of the image rather than resizing the image? It seems that for small images you would want to use width=device-width, in order for the image to be shown at the same size across devices, while not zooming in too much on very small images. Also, what is the purpose of some code being in Chrome/ChromeClient instead of directly in ImageDocument.cpp? Is this for implementing chrome specific behavior?
John Mellor
Comment 8 2012-02-22 10:36:55 PST
(In reply to comment #7) > Can you briefly explain why we want the viewport to be the size of the image rather than resizing the image? It seems that for small images you would want to use width=device-width, in order for the image to be shown at the same size across devices, while not zooming in too much on very small images. Yeah, width=device-width does make more sense for small images. A simple way to get the desired behavior (large images fit to width, but small ones are shown at devicePixelRatio) would be: <meta name="viewport" content="width=device-width"> ... <img src="..." style="max-width: 100%;"> This should still allow you to pinch-zoom in on large images; unfortunately double-tapping on the image probably won't do anything useful (for example Chrome for Android fits the double-tapped block to the screen width, which would have no effect). I can't think of a sensible double-tap behavior that would fix this either, so we might need to special-case double-tapping on ImageDocument :|
Eric Seidel (no email)
Comment 9 2012-05-04 11:27:49 PDT
Comment on attachment 121279 [details] Patch How do we test this?
Note You need to log in before you can comment on or make changes to this bug.