Bug 42089 – reported size of image is changed when page is zoomed in (or out)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[zebul666]

Created attachment 61241 [details]
test case

The size of an image reported by javascript changes when you zoom the page with ctrl-mouse up down.

This has been tested on Safari 5.0 (7533.16) (windows xp sp3) and Chromium 5.0.375.99 (0) (linux)
Firefox 3.6.6 (linux) and Opera 10.60 (6386) (linux) and Internet Explorer 8 don't behave that way; i.e. there is no change in image property size (height, width).

I spotted the bug when using the canvas element and resizing (zooming in) with CTRL-mouse up on my page.

You can find attached img.html that shows the bug with only an image if you resize it with ctrl-mouse up. Also I found that the reported size is not even accurate because if I take a screenshot and use a pixel ruler, I found a size greater than the one reported every time.

the canvas.html shows the problem on a canvas element

but the root of the problem is the reported size of an image if zoomed in or out. Should it change or not ? If not it's a bug.

[Peter Kasting]

I'm planning to fix this, as well as the problem where the wrong image size is reported in the page title when viewing a standalone zoomed image.

[Peter Kasting]

Created attachment 65187 [details]
patch v1

This fixes the issues reported on this bug, and also reports the correct (unzoomed) image size in the page title when viewing a standalone image.

[Peter Kasting]

Hyatt - I wrote the testcase to also highlight how inaccurate this patch is for zoom factors < 1.

[WebKit Review Bot]

Attachment 65187 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/HTMLImageElement.cpp:276:  render_box is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/html/HTMLImageElement.cpp:299:  render_box is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 65190 [details]
patch v1.1

Fix style issues

[Peter Kasting]

Created attachment 65953 [details]
patch v2.0

New testcase which prints PASS/FAIL as well as the zoom level on each oval.

[James Robinson]

Patch appears to be missing expected results.  Otherwise, looks pretty good to me (although I'm not a reviewer).

[Peter Kasting]

Created attachment 65949 [details]
patch v1.2

Oops.  Forgot to svn add that file.

[Simon Fraser (smfr)]

Comment on attachment 65949 [details]
patch v1.2

Looks like the expected result is wrong, and it would be better with a script-based test that prints PASS or FAIL.

[WebKit Commit Bot]

Comment on attachment 65953 [details]
patch v2.0

Rejecting patch 65953 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20875 test cases.
fast/images/zoomed-img-size.html -> failed

Exiting early after 1 failures. 8135 tests run.
186.55s total testing time

8134 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://queues.webkit.org/results/3843193

[Peter Kasting]

The reason the current patch fails is because on Windows, the effective zoom of 5% is stored as 0.050000001, and when adjustForAbsoluteZoom() divides 1 by that, it gets a number very close to, but not quite, 20.

I don't know what happens on Mac.

I think it would be good to know why Mac and Windows differ.  But I also think adjustForAbsoluteZoom() should copy the rounding code from CSSPrimitiveValue::computeLengthInt() and bump values away from 0 by 0.01 before rounding.  I wonder how many layout tests this breaks :(

[Peter Kasting]

Created attachment 66156 [details]
patch v3.0

Here's the patch with the rounding code added to adjustForAbsoluteZoom().

[Peter Kasting]

James and I tried to step through Mac to see why it differed from Windows.  We found that the assembly instruction that did a single-precision floating point divide of "1.0 / 0.050000007" produced a result of "20.0".  I don't know why.

[James Robinson]

$ cat test.cc
#include <stdio.h>
int main() {
    unsigned bitpattern = 0x3d4ccccd;
    float f = *reinterpret_cast<float*>(&bitpattern);
    printf("f = %.15f, (int)(1.0f / f) = %d\n", f, (int)(1.0f / f));
    double d = f;
    printf("d = %.15f, (int)(1.0 / f) = %d\n", d, (int)(1.0 / d));
    return 0;
}
jamesr@jamesr:~$ ./test
f = 0.050000000745058, (int)(1.0f / f) = 20
d = 0.050000000745058, (int)(1.0 / f) = 19

so it appears that the divide is a single-precision FP divide on a mac (it compiles to a DIVSS in fact), but that it might be happening in double precision on other platforms.  Will have to check the disassembly on a platform that produces 19 to know for sure.

Either way, a fudge factor seems prudent.

[Simon Fraser (smfr)]

Would be nice to share 

    result += result < 0 ? -0.01 : +0.01;

    if (result > INT_MAX || result < INT_MIN)
        return 0;
    return static_cast<int>(result);

everywhere it occurs, perhaps with some templatized code.

[Peter Kasting]

Created attachment 66250 [details]
patch v3.1

This factors out the rounding code into a template function so it can be shared among all callers.

[Simon Fraser (smfr)]

Comment on attachment 66250 [details]
patch v3.1

> Index: WebCore/css/CSSPrimitiveValue.cpp
> ===================================================================
> --- WebCore/css/CSSPrimitiveValue.cpp	(revision 66611)
> +++ WebCore/css/CSSPrimitiveValue.cpp	(working copy)
> @@ -45,6 +45,15 @@ using namespace WTF;
>  
>  namespace WebCore {
>  
> +template<typename T, T max, T min> inline T roundForImpreciseConversion(double result)
> +{
> +    // Dimension calculations are imprecise, often resulting in values of e.g.
> +    // 44.99998.  We need to go ahead and round if we're really close to the
> +    // next integer value.
> +    result += (result < 0) ? -0.01 : +0.01;
> +    return (result > max || result < min) ? 0 : static_cast<T>(result);
> +}

Won't this cause template instantiation for each combination of, T, min and max? Maybe min and max would
be better as arguments to the function.

Also 'result' is an odd name for an input paramter.

r=me

[Peter Kasting]

Created attachment 66257 [details]
patch v3.2

This changes the name "result" to "value", which is what nearby functions use.

Template arguments will cause one extra instantiation of the function.  OTOH, they might result in better code (since the compiler will have the literal values to compare), and I think the callers read slightly more clearly than with function arguments.  (Stroustrup also tends to use template args for function bounds.)  So in the end I have a very weak preference for template args.  If you feel strongly, I'll change them.

[WebKit Commit Bot]

Comment on attachment 66257 [details]
patch v3.2

Rejecting patch 66257 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Simon Fraser', u'--force']" exit_code: 1
Last 500 characters of output:
pp
Hunk #2 FAILED at 314.
1 out of 2 hunks FAILED -- saving rejects to file WebCore/css/CSSPrimitiveValue.cpp.rej
patching file WebCore/css/CSSPrimitiveValue.h
patching file WebCore/html/HTMLImageElement.cpp
patching file WebCore/loader/ImageDocument.cpp
patching file WebCore/rendering/RenderObject.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/images/zoomed-img-size-expected.txt
patching file LayoutTests/fast/images/zoomed-img-size.html

Full output: http://queues.webkit.org/results/3966030

[Peter Kasting]

Created attachment 66448 [details]
patch v3.3

Fixed to apply cleanly

[Peter Kasting]

Created attachment 66449 [details]
patch v3.4

Hmm, last upload incomplete

[WebKit Commit Bot]

Comment on attachment 66449 [details]
patch v3.4

Rejecting patch 66449 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/XSLTProcessor.o /Users/eseidel/Projects/CommitQueue/WebCore/xml/XSLTProcessor.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/HTMLParserScheduler.o /Users/eseidel/Projects/CommitQueue/WebCore/html/parser/HTMLParserScheduler.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(409 failures)


Full output: http://queues.webkit.org/results/3956088

[Peter Kasting]

Created attachment 66540 [details]
patch v3.5

Blind attempt at fixing strange compile error.

[WebKit Commit Bot]

Comment on attachment 66540 [details]
patch v3.5

Clearing flags on attachment: 66540

Committed r66776: <http://trac.webkit.org/changeset/66776>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66776 might have broken SnowLeopard Intel Release (Tests)

This appears to be broken again in version Chrome 21.0.1180.83
Just tried the image test case and zoom-out changes the width and height