[Webkit-unassigned] [Bug 46775] getBoundingClientRect does not work with SVG <text>

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69828|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #6 from Nikolas Zimmermann <zimmermann at kde.org>  2010-10-06 00:10:49 PST ---
(From update of attachment 69828)
View in context: https://bugs.webkit.org/attachment.cgi?id=69828&action=review

> LayoutTests/svg/custom/getBoundingClientRect-js.svg:19
> +                var img = document.createElementNS("http://www.w3.org/2000/svg", "image");

I'd propose to split this in several tests, one for getBoundingClientRect on text, one for image, one for g, one for rect, one for text.
If this ever breaks, it's not easy to spot what exactly is broken.

> WebCore/dom/Element.cpp:490
> +            const FloatRect& localRect = static_cast<const SVGStyledLocatableElement*>(svgElement)->getBBox();

I think we should introduce a "bool boundingBoxForSVGElement(FloatRect&)" method in SVGElement, that includes these checks. It feels awkward to have this logic in Element.cpp.

This could would simplify to:
if (isSVGElement()) {
    FloatRect localRect;
    if (static_cast<const SVGElement*>(this)->boundingBoxForSVGElement(localRect))
        quads.append(renderer()->localToAbsoluteQuad(localRect));
}

The SVGElement::boundingBoxForSVGElement method would utilize, isStyledLocatable() cast itself to a SVGStyledLocatableElement, and return getBBox().
Additionally it checks the isText() case, for both cases it fills the passed in localRect reference, and returns true. Otherwhise it'll return false.

> WebCore/svg/SVGElement.h:58
> +        virtual bool isText() const { return false; }

This is not needed. In the new boundingBoxForSVGElement() method, you can use:
if (hasTagName(SVGNames::textTag)) {
    SVGTextElement* textElement = static_cast<SVGTextElement*>(element);
...

> WebCore/svg/SVGStyledTransformableElement.cpp:-103
> -SVGElement* SVGStyledTransformableElement::nearestViewportElement() const
> -{
> -    return SVGTransformable::nearestViewportElement(this);
> -}
> -
> -SVGElement* SVGStyledTransformableElement::farthestViewportElement() const
> -{
> -    return SVGTransformable::farthestViewportElement(this);
> -}
> -
> -FloatRect SVGStyledTransformableElement::getBBox(StyleUpdateStrategy styleUpdateStrategy) const
> -{
> -    return SVGTransformable::getBBox(this, styleUpdateStrategy);
> -}
> -

Good catches!

> WebCore/svg/SVGTextElement.h:46
> +        virtual bool isText() const { return true; }

Not needed, see the other comments.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.