Bug 77998 - getBoundingClientRect() returns the incorrect value on elements with the CSS zoom property
Summary: getBoundingClientRect() returns the incorrect value on elements with the CSS ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-02-07 09:59 PST by Max Vujovic
Modified: 2024-10-21 16:34 PDT (History)
12 users (show)

See Also:


Attachments
Reproduction (1.28 KB, text/html)
2012-02-07 09:59 PST, Max Vujovic
no flags Details
Patch (29.38 KB, patch)
2012-02-10 09:44 PST, Max Vujovic
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Max Vujovic 2012-02-07 09:59:32 PST
Created attachment 125866 [details]
Reproduction

In the attached reproduction, the upper red div is 200px by 200px with a CSS zoom property of 200%. getBoundingClientRect() should return 400 x 400, but it returns 200 x 200.

As a point of comparison, the lower green div is 200px by 200px with a CSS transform of scale(2). getBoundingClientRect() correctly returns 400 x 400.
Comment 1 Dirk Schulze 2012-02-07 14:26:28 PST
Since 'zoom' does not include zooming of the viewport (http://dev.w3.org/csswg/css-device-adapt/#the-lsquozoomrsquo-property), the current behavior in WebKit seems to be indeed wrong.
Comment 2 Max Vujovic 2012-02-10 09:44:24 PST
Created attachment 126527 [details]
Patch

Thanks for looking at this, Dirk.

I've put up a patch that fixes the issue, and described the change in the ChangeLog like so:

"""
        * dom/Element.cpp:
        (WebCore::Element::getBoundingClientRect):

            After the element's absoluteQuads are united into a single bounding rect, they need to
            be transformed from screen coordinates to viewport coordinates.

            Before, we were using adjustFloatRectForAbsoluteZoom in the transformation, which
            divided screen coordinates by effectiveZoom. However, effectiveZoom includes both page
            zoom and the CSS zoom property. The CSS zoom property affects elements but does not
            affect the viewport, so it should not be involved in the transformation.

            Now, we are using adjustFloatRectForPageZoom and the pageZoomFactor to transform from
            screen coordinates to viewport coordinates, which does not involve CSS zoom.

        * rendering/RenderObject.h:
        (WebCore::adjustFloatRectForPageZoom):

            This is a new function, defined underneath similar functions like 
            adjustFloatRectForAbsoluteZoom and adjustFloatRectForPageScale.
"""
Comment 3 WebKit Review Bot 2012-02-10 10:27:12 PST
Comment on attachment 126527 [details]
Patch

Attachment 126527 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11486733

New failing tests:
fast/transforms/bounding-rect-zoom.html
css3/zoom-coords.xhtml
media/video-controls-zoomed.html
Comment 4 Max Vujovic 2012-02-10 12:06:35 PST
Comment on attachment 126527 [details]
Patch

Canceling review. I'll take a look at the EWS test failures first.
Comment 5 Max Vujovic 2012-02-10 15:02:37 PST
(In reply to comment #3)
> New failing tests:
> fast/transforms/bounding-rect-zoom.html

This one I just need to update.

> css3/zoom-coords.xhtml

I'm not sure why this test is failing, since it passes on my dev machine. 

> media/video-controls-zoomed.html

I can't reproduce this failure either on my dev machine. 

I'm thinking some fundamental issues with using integers for layout might be causing these failures, but it's hard to be sure without the individual test results from the bot.

Here is a test case that uses getBoundingClientRect and demonstrates the issue:
https://bug-60318-attachments.webkit.org/attachment.cgi?id=98021

This test case is attached to the meta bug for switching away from integer layout units (bug 60318).

For getBoundingClientRect in particular, here's an example of how the integer layout units issue plays out:
- Suppose we have a 10px wide div and the page has been zoomed in twice. 
- The page zoom factor is 1.2*1.2 = 1.44.
- The div's subpixel width is 10 * 1.44 = 14.4.
- The div's RenderBox::width() will return 14, as an integer.
- getBoundingClientRect will call absoluteQuads, which uses RenderBox::width().
- absoluteQuads will return a width of 14 for the div.
- getBoundingClientRect will undo the page zoom factor by doing 14 / 1.44 = 9.7222...
- getBoundingClientRect will return 9.7222..., not 10 as it should.
Comment 6 Silvia Pfeiffer 2012-08-19 22:47:31 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > New failing tests:
> > fast/transforms/bounding-rect-zoom.html
> 
> This one I just need to update.
> 
> > css3/zoom-coords.xhtml
> 
> I'm not sure why this test is failing, since it passes on my dev machine. 
> 
> > media/video-controls-zoomed.html
> 
> I can't reproduce this failure either on my dev machine.

I believe this latter one just needs a rebaseline because it uses zoom on body.

I'm right now implementing changes in the video controls based on the width of the video and my video width calculation is off when there is CSS zoom being used:

    float zoomLevel = style()->effectiveZoom();
    IntRect mediaBox = pixelSnappedIntRect(toRenderMedia(mediaElement->renderer())->contentBoxRect());
    int mediaWidth = static_cast<int>(mediaBox.width() * zoomLevel);

I currently have to multiply the zoomLevel by  document()->frame()->pageZoomFactor() to get the actual width.

Might it be better to fix the value that effectiveZoom() returns?
Comment 7 Silvia Pfeiffer 2012-08-19 23:48:17 PDT
(In reply to comment #6)
>     float zoomLevel = style()->effectiveZoom();
>     IntRect mediaBox = pixelSnappedIntRect(toRenderMedia(mediaElement->renderer())->contentBoxRect());
>     int mediaWidth = static_cast<int>(mediaBox.width() * zoomLevel);

This should have been:

 int mediaWidth = static_cast<int>(mediaBox.width() / zoomLevel);
Comment 8 Max Vujovic 2012-08-20 11:21:08 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > New failing tests:
> > > fast/transforms/bounding-rect-zoom.html
> > 
> > This one I just need to update.
> > 
> > > css3/zoom-coords.xhtml
> > 
> > I'm not sure why this test is failing, since it passes on my dev machine. 
> > 
> > > media/video-controls-zoomed.html
> > 
> > I can't reproduce this failure either on my dev machine.
> 
> I believe this latter one just needs a rebaseline because it uses zoom on body.
> 
> I'm right now implementing changes in the video controls based on the width of the video and my video width calculation is off when there is CSS zoom being used:
> 
>     float zoomLevel = style()->effectiveZoom();
>     IntRect mediaBox = pixelSnappedIntRect(toRenderMedia(mediaElement->renderer())->contentBoxRect());
>     int mediaWidth = static_cast<int>(mediaBox.width() * zoomLevel);
> 
> I currently have to multiply the zoomLevel by  document()->frame()->pageZoomFactor() to get the actual width.
> 
> Might it be better to fix the value that effectiveZoom() returns?

I was under the impression that effectiveZoom already includes both pageZoomFactor and the CSS zoom of the element and all its ancestors. Is different than what you're seeing?
Comment 9 Silvia Pfeiffer 2012-08-20 17:05:50 PDT
> I was under the impression that effectiveZoom already includes both pageZoomFactor and the CSS zoom of the element and all its ancestors. Is different than what you're seeing?

You are correct. It works for me now. I just had to use round() to get the correct width since the straight up calculation sometimes returned a float that was too small. I've gone with this now:

    float mediaWidthFloat = toRenderMedia(mediaElement->renderer())->contentBoxRect().width().toFloat();
    int mediaWidth = round(adjustFloatForAbsoluteZoom(mediaWidthFloat, style()));
Comment 10 Max Vujovic 2012-08-24 09:47:45 PDT
(In reply to comment #9)
> > I was under the impression that effectiveZoom already includes both pageZoomFactor and the CSS zoom of the element and all its ancestors. Is different than what you're seeing?
> 
> You are correct. It works for me now. I just had to use round() to get the correct width since the straight up calculation sometimes returned a float that was too small. I've gone with this now:
> 
>     float mediaWidthFloat = toRenderMedia(mediaElement->renderer())->contentBoxRect().width().toFloat();
>     int mediaWidth = round(adjustFloatForAbsoluteZoom(mediaWidthFloat, style()));

Tricky- good catch! Just curious, does that mean you had media elements whose size in CSS pixels was < 1px?
Comment 11 Silvia Pfeiffer 2012-08-26 18:56:43 PDT
(In reply to comment #10)
> Tricky- good catch! Just curious, does that mean you had media elements whose size in CSS pixels was < 1px?

No, I don't think people would do that.

What happened, though, was that I had to make a decision on which elements to hide in the video controls based on the width of the video and I was not seeing them being hidden at the appropriate pixel widths. This fixed it.

As we're moving away from int, we're likely to see such cases more often.
Comment 12 Chris Rebert 2015-07-04 22:51:09 PDT
This is still buggy as of Safari Version 8.0.7 (10600.7.12).
Comment 13 Radar WebKit Bug Importer 2017-05-11 19:28:56 PDT
<rdar://problem/32148538>
Comment 14 Vik 2021-03-02 13:35:15 PST
Is this issue still being worked on? 
I am trying to track an element's position from the top of the viewport using getBoundingClientRect().top but it returns incorrect value when the <body/> has CSS zoom property applied to it. 
I'm thinking it is related to this issue.
Comment 15 Simon Fraser (smfr) 2021-03-02 13:37:33 PST
The CSS zoom property is non-standard, and its use is discouraged. Is there a reason you're using it?
Comment 16 Vik 2021-03-02 13:42:17 PST
(In reply to Simon Fraser (smfr) from comment #15)
> The CSS zoom property is non-standard, and its use is discouraged. Is there
> a reason you're using it?

We build a no-code content-builder tool and we use the zoom property to scale our web pages across devices of varying sizes. One of our features uses getBoundingClientRect().top and it's not working on Safari due to this issue. :(
Comment 17 Jan De Poorter 2024-09-10 07:13:04 PDT
(In reply to Simon Fraser (smfr) from comment #15)
> The CSS zoom property is non-standard, and its use is discouraged. Is there
> a reason you're using it?

Given that the CSS zoom property is now standard (https://github.com/w3c/csswg-drafts/pull/9699), can this issue be looked at once more, it is still present in Safari Technology Preview Release 202 (Safari 18.0, WebKit 19620.1.4.8), and this issue isn't present in other rendering engines.

Thank you!