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.
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.
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 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 on attachment 126527 [details] Patch Canceling review. I'll take a look at the EWS test failures first.
(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.
(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?
(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);
(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?
> 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()));
(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?
(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.
This is still buggy as of Safari Version 8.0.7 (10600.7.12).
<rdar://problem/32148538>
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.
The CSS zoom property is non-standard, and its use is discouraged. Is there a reason you're using it?
(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. :(
(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!