getBoundingClientRect() returns the incorrect value on elements with the CSS zoom property

• Component: DOM
• Status: NEW ---

[Max Vujovic]

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.

[Dirk Schulze]

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.

[Max Vujovic]

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.
"""

[WebKit Review Bot]

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

[Max Vujovic]

Comment on attachment 126527 [details]
Patch

Canceling review. I'll take a look at the EWS test failures first.

[Max Vujovic]

(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.

[Silvia Pfeiffer]

(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?

[Silvia Pfeiffer]

(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);

[Max Vujovic]

(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?

[Silvia Pfeiffer]

> 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()));

[Max Vujovic]

(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?

[Silvia Pfeiffer]

(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.

[Chris Rebert]

This is still buggy as of Safari Version 8.0.7 (10600.7.12).

[Radar WebKit Bug Importer]

<rdar://problem/32148538>

[Vik]

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.

[Simon Fraser (smfr)]

The CSS zoom property is non-standard, and its use is discouraged. Is there a reason you're using it?

[Vik]

(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. :(