Bug 46203 – getClientRects()[0] != getBoundingClientRect()

• Component: SVG
• Status: RESOLVED FIXED

[Mike Bostock]

Created attachment 68265 [details]
CSSOM-SVG test case.

According to CSSOM View 7.1, getClientRects():

"""
If the element does not have an associated CSS layout box and is in the http://www.w3.org/2000/svg namespace return a ClientRectList object containing a single ClientRect object that describes the bounding box of the element as defined by SVG specification.
"""

The expected behavior of getClientRects() is that it return a single-element array containing the "tight bounding box ... on the geometry of all contained graphics elements" per SVGLocatable.getBBox. The value of svg.getClientRects()[0] should therefore equal svg.getBBox(). Furthermore, svg.getClientRects()[0] should equal svg.getBoundingClientRect(), per CSSOM "Otherwise, return a ClientRect object describing the smallest rectangle that includes the first rectangle in list and all of the remaining rectangles of which the height or width is not zero."

However, the actual behavior of getClientRects() is that it returns a single-element array with the size of the SVG container.

This is partly a regression from Safari 5, where getClientRects()[0] and getBoundingClientRect() were consistent. However, in Safari 5 the returned value did not equal getBBox(). Firefox (3.6 & 4) arguably does not pass this test, either. For Firefox getBoundingClientRect() and getClientRects()[0] are equal, but slightly larger than the value of getBBox(), perhaps because of padding / margin / borders. Opera exhibits the opposite behavior of WebKit nightlies: it returns the correct value for getBoundingClientRect(), but the SVG container size for getClientRects()[0].

[Dirk Schulze]

(In reply to comment #0)
> Created an attachment (id=68265) [details]
> CSSOM-SVG test case.
> 
> According to CSSOM View 7.1, getClientRects():
> 
> """
> If the element does not have an associated CSS layout box and is in the http://www.w3.org/2000/svg namespace return a ClientRectList object containing a single ClientRect object that describes the bounding box of the element as defined by SVG specification.
> """
>
I wonder what it means for the outer most SVGSVGElement. I expect that in this case we can have a list of more than jut one entries:

"""
Return a ClientRectList object containing a list of ClientRect objects in content order describing the border boxes (including those with a height or width of zero) with the following constraints:

If the element on which the method was invoked has a computed value for display property of table or inline-table include both the table box and the caption box, if any, but not the anonymous container box.

Replace each anonymous block box with its child box(es) and repeat this until no anonymous block boxes are left in the final list.
"""

http://www.w3.org/TR/cssom-view/#dom-element-getclientrects

[Dirk Schulze]

Created attachment 124996 [details]
Same example as above

Same example as above to have an example that works directly after clicking on the link.

[Max Vujovic]

(In reply to comment #1)
> (In reply to comment #0)

> I wonder what it means for the outer most SVGSVGElement. I expect that in this case we can have a list of more than jut one entries:

This is one potential interpretation. As you pointed out, it really depends on the spec (http://www.w3.org/TR/cssom-view/#the-getclientrects-and-getboundingclientrect-methods).

Section 6.1 states:
"""
The getClientRects() method, when invoked, must return the result of the following algorithm:

1. If the element on which it was invoked does not have an associated CSS layout box and is not in the http://www.w3.org/2000/svg namespace return an empty ClientRectList object and stop this algorithm.

2. If the element does not have an associated CSS layout box and is in the http://www.w3.org/2000/svg namespace return a ClientRectList object containing a single ClientRect object that describes the bounding box of the element as defined by the SVG specification. [SVG]

3. Return a ClientRectList object containing a list of ClientRect objects in content order describing the border boxes (including those with a height or width of zero) with the following constraints:

If the element on which the method was invoked has a computed value for display property of table or inline-table include both the table box and the caption box, if any, but not the anonymous container box.

Replace each anonymous block box with its child box(es) and repeat this until no anonymous block boxes are left in the final list.
"""

As a summary, I want to reiterate what we've established as I understand it:

(1) The SVG root element does not fall under case #1 because the SVG root element is in the "http://www.w3.org/2000/svg" namespace.

(2) The SVG root element does not fall under case #2 because it does have an associated CSS layout box. (I'd like to find which spec specifies that the SVG root element has an associated CSS layout box, but I haven't been able to locate it yet).

(3) Thus, the SVG root element must fall under case #3. In this case, the getClientRects() method returns a list of ClientRects "describing the border boxes". Then, the question is:

What are the border boxes of an SVG root element?

One interpretation is that the SVG root element's "border boxes" simply consist of its own border box. This is what Rik Cabanier suggested, and it seems reasonable. This is what the current implementation of getClientRects() returns.

However, with this interpretation, getBoundingClientRect() should be equal to getClientRects()[0], based on the spec:

"""
The getBoundingClientRect() method, when invoked, must return the result of the following algorithm:

1. Let list be the result of invoking getClientRects() on the same element this method was invoked on.

2. If the list is empty return a ClientRect object whose top, right, bottom and left members are zero.

3. Otherwise, return a ClientRect object describing the smallest rectangle that includes the first rectangle in list and all of the remaining rectangles of which the height or width is not zero.
"""

For the SVG root element, we fall into case #3 for getBoundingClientRect(). This case should just return the one item from the getClientRects() list.

However, WebKit returns the smallest bounding rectangle surrounding the SVG root element's children.

[Dirk Schulze]

(In reply to comment #3)

> However, with this interpretation, getBoundingClientRect() should be equal to getClientRects()[0], based on the spec:
> 
> """
> The getBoundingClientRect() method, when invoked, must return the result of the following algorithm:
> 
> 1. Let list be the result of invoking getClientRects() on the same element this method was invoked on.
> 
> 2. If the list is empty return a ClientRect object whose top, right, bottom and left members are zero.
> 
> 3. Otherwise, return a ClientRect object describing the smallest rectangle that includes the first rectangle in list and all of the remaining rectangles of which the height or width is not zero.
> """
> 
> For the SVG root element, we fall into case #3 for getBoundingClientRect(). This case should just return the one item from the getClientRects() list.
> 
> However, WebKit returns the smallest bounding rectangle surrounding the SVG root element's children.

You are getting something wrong here for getBoundingClientRect():
Step 1 says: take the list of getClientsRect() (which just includes the rect 960 x 400)
Step 2 don't consider empty rects from the list (we don't have empty rects here)
Step 3 take the unit of all rects of the remaining list
--> The result is: 960 x 400

That means, for the SVG root element in the example we should get  getClientRects()[0] == getBoundingClientRect() == 960 x 400 !!!

[Max Vujovic]

(In reply to comment #4)
> (In reply to comment #3)
> You are getting something wrong here for getBoundingClientRect():
> Step 1 says: take the list of getClientsRect() (which just includes the rect 960 x 400)
> Step 2 don't consider empty rects from the list (we don't have empty rects here)
> Step 3 take the unit of all rects of the remaining list
> --> The result is: 960 x 400
> 
> That means, for the SVG root element in the example we should get  getClientRects()[0] == getBoundingClientRect() == 960 x 400 !!!

I completely agree! In the example, getClientRects()[0] should equal getBoundingClientRect(), which should equal 960 x 400. Sorry for the confusion :)

[Max Vujovic]

Created attachment 126194 [details]
Patch

Hi Dirk,

I've put up a patch for review. I'll wait for EWS bots to finish before setting cq?.

I've tried to detail my changes and approach in the WebCore ChangeLog and the LayoutTests ChangeLog.

Here is the most important information from the WebCore ChangeLog. You can also see this in the patch:
"""
        Change the behavior of Element::getClientRects and Element::getBoundingClientRect for SVG
        elements to match the CSSOM View Module spec:
        http://www.w3.org/TR/cssom-view/#the-getclientrects-and-getboundingclientrect-methods.

        Before, Element::getClientRects would return an empty list for inner SVG elements that did 
        not descend from RenderBoxModelObject. Now, Element::getClientRects returns a list with one
        ClientRect corresponding to the element's SVG bounding box, as specified in CSSOM.

        Before, Element::getBoundingClientRect would return the SVG bounding box for SVG root
        elements. Now, Element::getBoundingClientRect returns the CSS bounding box for SVG root
        elements, as specified in CSSOM.

        This patch uses RenderBox::absoluteQuads to compute the CSS bounding box for SVG root
        elements. This is the correct approach. However, there is a separate bug which causes
        RenderBox::absoluteQuads to return an incorrect value on RenderSVGRoot in certain cases.
        The absoluteQuads bug is:

        https://bugs.webkit.org/show_bug.cgi?id=78165

        This means that Element::getClientRects and Element::getBoundingClientRect will not return
        the correct value for SVG root elements in certain cases until the absoluteQuads bug is
        fixed.

        The test files for Element::getClientRects and Element::getBoundingClientRect on SVG
        elements have been updated. However, one of the test files will fail until the
        absoluteQuads bug is fixed. The test file is:

        LayoutTests/svg/custom/zoom-zoom-coords.xhtml.

        Updating the zoom-zoom-coords test file to match the CSSOM spec also revealed a separate bug
        that involves Element::getClientRects, HTML elements, and CSS zoom:
        https://bugs.webkit.org/show_bug.cgi?id=77998.

        This bug must also be fixed for the entire zoom-zoom-coords test file to pass.
"""

Thank you,
Max

[Dirk Schulze]

Comment on attachment 126194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126194&action=review

You can set cq? from the beginning. If one bot fails, it will set the cq automatically to cq-.

> LayoutTests/svg/custom/boundingBox.html:5
> +    body { margin: 0 0 0 0; }

margin: 0; would be enough.

> LayoutTests/svg/custom/boundingBox.html:24
> +        document.getElementById("result").innerHTML = boundingBoxMatchesExpected ? 
> +            "PASS" : "FAIL: boundingBox is " + boundingBox.width + " x " + boundingBox.height + " @ (" + boundingBox.left + ", " + boundingBox.top + "), but should be  "
> +            + expectedBoundingBox.width + " x " + expectedBoundingBox.height + " @ (" + expectedBoundingBox.left + ", " + expectedBoundingBox.top + ").";

Who is responsible for that?!? Can you order it a bit more and use more lines? An if else might make it more readable.

> LayoutTests/svg/custom/getClientRects.html:20
> +            function $(id)
> +            {

We usually use the same style rules for tests like for normal code, so
function name() {
    ....

> Source/WebCore/ChangeLog:50
> +            Now, both Element::getClientRects and Element::getBoundingClientRect use the helper
> +            method Element::clientQuads, eliminating duplicate code that adjusted the element's
> +            absolute quads for viewport zoom and scroll position.

This code differed before your patch!

> Source/WebCore/ChangeLog:60
> +            Now, this method calls Element::clientQuads. It unites the quads and returns a single
> +            bounding ClientRect, as before.

It does something different than before.

> Source/WebCore/dom/Element.cpp:506
> +    RenderObject* ro = renderer();

please use RenderObject* renderer = this->renderer():

At first: WebKit demands full names; second: renderer is common name.

> Source/WebCore/dom/Element.cpp:507
> +    if (isSVGElement() && (!ro || (ro && !ro->isSVGRoot()))) {

that doesn't make sense. Remove the !ro, use:
if (isSVGElement() && renderer && !renderer->isSVGRoot()) instead.

> Source/WebCore/dom/Element.cpp:508
> +        // This case handles inner SVG elements which do not have a CSS layout box.

This comment is wrong, at least partly. Just

// This case handles SVG elements which do not have a CSS layout box.

Remove the 'inner'.

> Source/WebCore/dom/Element.cpp:520
> +    if (RenderBoxModelObject* box = renderBoxModelObject()) {
> +        // This case handles elements with a CSS layout box.

Can you move that to an early return:

RenderBoxModelObject* box = renderBoxModelObject():
if (!box)
    return;

When you are on it, can you name it renderBoxModelObject please?

> Source/WebCore/dom/Element.cpp:552
> +    clientQuads(quads);

This is different than it was before. Before you just asked the current renderer for the absoluteQuads(quads) and than moved just the first entry. Now you move every quad in the list.

The problem is...

> Source/WebCore/dom/Element.cpp:559
>      for (size_t i = 1; i < quads.size(); ++i)
>          result.unite(quads[i].boundingBox());

...that you create the unit from quads that might now be different than on the previous code.

> Source/WebCore/dom/Element.cpp:-564
> -        adjustFloatRectForPageScale(result, page->pageScaleFactor());

also here you adjust the result, not every quad.

At least moving every quad instead of just the first is a code difference. I can not say if it is correct with your patch or before your patch. You have to test it. http://msdn.microsoft.com/en-us/library/ms536433(VS.85).aspx might help.

But we can not land it before we are not sure if the code change is correct. For me it is indeed strange that we just move the first quad in the list on getBoundingClientRect(). Your behavior seems to make more sense. Nevertheless we need to verify that.

[Dirk Schulze]

Has been fixed in the meantime.