Bug 46775 – getBoundingClientRect does not work with SVG <text>

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Cosmin Truta]

The test case attached is currently failing. In order to make it pass, it is necessary to comment out the if-statement that checks txt.getBoundingClientRect() at line 20.

I opened this bug to finalize the work started under the bug 42815
https://bugs.webkit.org/show_bug.cgi?id=42815

[Cosmin Truta]

Created attachment 69398 [details]
Test case

Oops, forgot to attach the test case. Here it is.

[Cosmin Truta]

Two of the methods offered by SVGStyledLocatableElement, namely getBBox and getCTM, are being missed when processing <svg:text>.
(See Element::getBoundingClientRect and SVGLocatable::getTransformToElement.)

This is happening in spite of the fact that SVGTextElement implements all methods from SVGStyledLocatableElement, exactly the same way.

Would it be a sound design decision to add SVGStyledLocatableElement to the list of parents of SVGTextElement, i.e. to make SVGTextElement itself StyledLocatable?
Or, is there a specific reason why SVGTextElement is _not_ StyledLocatable?

The only technicality that could possibly prevent this modification, is that SVGStyledElement would appear twice in the list of parents of SVGTextElement. I wonder whether SVGStyledElement could be repositioned within the list of ancestors, or be made a virtual parent to SVGStyledLocatableElement.

I suppose I could check separately for SVGStyledLocatableElement and SVGTextElement inside Element::getBoundingClientRect, in order to fix this issue, but I am not so sure this easy solution is also the most elegant. And shouldn't the same thing be done inside SVGLocatable::getTransformToElement?

[Cosmin Truta]

Created attachment 69813 [details]
Fix, layout test and code cleanup

I am submitting the straightforward solution, checking for isText in addition to isStyledLocatable.

If you want me to make SVGTextElement a subclass of SVGStyledLocatable, please let me know, and I will try implementing this new design this in a separate patch.

Also, if you want me to fix SVGLocatable::getTransformToElement by accounting for SVGTextElement in addition to SVGStyledLocatableElement when calling getCTM, please tell me, and I'll open a separate bug where I will fix it.

[Cosmin Truta]

Created attachment 69828 [details]
The same patch as before, with just a little more cleanup (two indentation fixes in JavaScript)

[Nikolas Zimmermann]

(In reply to comment #3)
> Created an attachment (id=69813) [details]
> Fix, layout test and code cleanup
> 
> I am submitting the straightforward solution, checking for isText in addition to isStyledLocatable.
Will check your patch soon.

> 
> If you want me to make SVGTextElement a subclass of SVGStyledLocatable, please let me know, and I will try implementing this new design this in a separate patch.
No, that would be wrong, SVGTextContent doesn't implement the SVGLocatable interface, but SVGTransformable. Though we can't use SVGStyledTransformable as base class for SVGTextElement, because SVGTextContentElement already inherits from SVGStyledElement. To make it clear:

SVGTextElement : SVGTextPositioningElement, SVGTransformable
SVGTextPositioningElement : SVGTextContentElement
SVGTextContentElement : SVGStyledElement, SVGTests, SVGLangSpace, SVGExternalResourcesRequired

This class hiearchy 1:1 maps the SVG specification.

> 
> Also, if you want me to fix SVGLocatable::getTransformToElement by accounting for SVGTextElement in addition to SVGStyledLocatableElement when calling getCTM, please tell me, and I'll open a separate bug where I will fix it.

Will check your patch first :-)

[Nikolas Zimmermann]

Comment on attachment 69828 [details]
The same patch as before, with just a little more cleanup (two indentation fixes in JavaScript)

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.

[Cosmin Truta]

Created attachment 70200 [details]
Fix and augmented layout test

The fix is as suggested by Nico, except that I used boundingBoxForElement instead of boundingBoxForSVGElement.
(Is this not a more appropriate name? I am a little confused here: it's either boundingBoxForElement, to be used in Element; otherwise, should it not have been simply SVGElement::boundingBox? The same naming style is used e.g. in SVGElement::instancesForElement.)

The <text> fix also includes the zooming fix, previously submitted to 42815.

The layout test is focused on zooming, not on <text>, although it does include a test for <text>, easy to identify on failure (unlike in the previous patch). 

There is still no JavaScript-driven zooming. I tried it, and it's flaky: it works most of the time, but not all the time, and the remaining times the coordinates are either off-by-one, or zero, or (rarely) contain garbage values.
Before adding such tests to zoom/page, I will look into how to make them unflaky.

[Nikolas Zimmermann]

Comment on attachment 70200 [details]
Fix and augmented layout test

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

> WebCore/svg/SVGElement.h:75
> +        bool boundingBoxForElement(FloatRect&, SVGLocatable::StyleUpdateStrategy = SVGLocatable::AllowStyleUpdate) const;

ok, you're right s/boundingBoxForElement/boundingBox/ is better.

You can't commit yet, right? r- then, waiting for your revised version.

[Nikolas Zimmermann]

(In reply to comment #7)
> Created an attachment (id=70200) [details]
> Fix and augmented layout test
> 
> The fix is as suggested by Nico, except that I used boundingBoxForElement instead of boundingBoxForSVGElement.
> (Is this not a more appropriate name? I am a little confused here: it's either boundingBoxForElement, to be used in Element; otherwise, should it not have been simply SVGElement::boundingBox? The same naming style is used e.g. in SVGElement::instancesForElement.)
Yeah, just boundingBox is fine.

> 
> The <text> fix also includes the zooming fix, previously submitted to 42815.
> 
> The layout test is focused on zooming, not on <text>, although it does include a test for <text>, easy to identify on failure (unlike in the previous patch). 
> 
> There is still no JavaScript-driven zooming. I tried it, and it's flaky: it works most of the time, but not all the time, and the remaining times the coordinates are either off-by-one, or zero, or (rarely) contain garbage values.
> Before adding such tests to zoom/page, I will look into how to make them unflaky.

Okay, I replied to you in private mail, this is really high priority. The "zoom" property is never used for SVG, and what we really needs to work, is <svg viewBox="0 0 100 100"> + zooming/scrolling to work :-)

[Cosmin Truta]

Created attachment 70234 [details]
Fix and layout test

s/boundingBoxForElement/boundingBox/

[Nikolas Zimmermann]

Comment on attachment 70234 [details]
Fix and layout test

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

> WebCore/ChangeLog:9
> +        through SVGElement::boundingBoxForElement.

Still says boundingBoxForElement here.

> WebCore/ChangeLog:27
> +        (SVGElement::boundingBoxForElement): Added.

And here.

> WebCore/dom/Element.cpp:511
> +    ASSERT(renderer());

The assertion is wrong, it would fire on <rect display="none"...>

[Cosmin Truta]

Created attachment 70242 [details]
Fix and layout test, ChangeLog fixed

s/boundingBoxForElement/boundingBox/ inside ChangeLog

[Nikolas Zimmermann]

Comment on attachment 70242 [details]
Fix and layout test, ChangeLog fixed

Looks fine. The assertion doesn't fire, as quads is empty, if there's no renderer, and we're exiting early. So it's fine as is!

[WebKit Commit Bot]

Comment on attachment 70242 [details]
Fix and layout test, ChangeLog fixed

Rejecting patch 70242 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build', '--no-clean', '--no-update', '--build', '--build-style=both', '--quiet']" exit_code: 2
Building WebKit
Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1

Full output: http://queues.webkit.org/results/4163144

[Cosmin Truta]

Created attachment 70263 [details]
Fix and layout test

Would it be possible that the patch failed to apply because I replaced text inside the diff file directly, instead of going through editing source files and running "git diff" again?
Or, is there something wrong with the WebKit tooling?

I am resubmitting the patch, which is now coming straight out of "git diff".

[Eric Seidel (no email)]

No, when build fails, it means it failed to build, not apply. :)  The commit-queue log messages got cryptic again recently.  Working on a fix.

[Cosmin Truta]

(In reply to comment #16)
> No, when build fails, it means it failed to build, not apply. :)  The commit-queue log messages got cryptic again recently.  Working on a fix.

Thank you for the clarification. I was confused by the error message, which says that the patch failed to applied, then the build failed. I thought the build refused to run *because* the patch had failed to apply.

To me, the webkit-patch script is itself an untamed beast.

[Eric Seidel (no email)]

Comment on attachment 70242 [details]
Fix and layout test, ChangeLog fixed

Cleared Nikolas Zimmermann's review+ from obsolete attachment 70242 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Cosmin Truta]

Created attachment 70941 [details]
Fix and layout test.

Same patch as before, but with the extra ASSERT removed from Element::getBoundingClientRect.

The ASSERT is not necessary, since the following statement will crash unambiguously if renderer() is null. In fact, that particular ASSERT might generate confusion, because one must look carefully to see that it's *not* a precondition.

(Eiffel's "require" and "ensure" clauses come to mind as a neat manner to separate pre- and postconditions from the rest of the assertions. This is useful both for compilation and documentation purposes.)

[Nikolas Zimmermann]

Comment on attachment 70941 [details]
Fix and layout test.

r=me. I hope you can still work on the svg/zoom/page/ tests? :-)

[WebKit Commit Bot]

Comment on attachment 70941 [details]
Fix and layout test.

Rejecting patch 70941 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build', '--no-clean', '--no-update', '--build-style=both', '--quiet']" exit_code: 2
Building WebKit
Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1

Full output: http://queues.webkit.org/results/4490014

[Cosmin Truta]

(In reply to comment #20)
> (From update of attachment 70941 [details])
> r=me. I hope you can still work on the svg/zoom/page/ tests? :-)

Thanks for the review, and yes, I'll go back to the zooming tests as soon as I finish that other failure I'm working on.

But I'm baffled by the repeated occurrence of the error that doesn't let my patch go through. I rerun webkit-patch without --quiet and the only relevant thing I'm seeing is the failure of update-webkit with exit_code=2.
It fails to run "git svn rebase" (at update-webkit:129) and I can't understand why.

[Eric Seidel (no email)]

Try running update-webkit manually.  That's a red herring, specific to your checkout.

Your patch breaks mac.  Why?  We don't know because the mac-EWS can't run for your patch because your'e not a committer and the commit-queue isn't spitting out enough information.

The path forward is to get a person with a mac to test your patch, or for us to finally fix the commit-queue to give better build log information.

[Cosmin Truta]

(In reply to comment #23)
> Try running update-webkit manually.  That's a red herring, specific to your checkout.

$ WebKitTools/Scripts/update-webkit
Updating OpenSource
Unable to determine upstream SVN information from working tree history

Died at WebKitTools/Scripts/update-webkit line 129.

> Your patch breaks mac.  Why?  We don't know because the mac-EWS can't run for your patch because your'e not a committer and the commit-queue isn't spitting out enough information.

In fact, I did run it repeatedly on Chromium's try bots, and I rerun it just now. The Mac and Linux bots were happy.

The Windows bot did complain at link-time about the removal of some methods from SVGStyledTransformableElement:
SVGStyledTransformableElement::nearestViewportElement
SVGStyledTransformableElement::farthestViewportElement
SVGStyledTransformableElement::getBBox

I probably missed this last time, because I kept getting bogus errors. Perhaps some implib must be updated. I will look into it, although it could be difficult, because my development platform is Linux.

But I received no error from applying the patch, and no breakage on Mac. (I mean no breakage on chromium-mac; WebKit-mac could be a different deal.)

> The path forward is to get a person with a mac to test your patch, or for us to finally fix the commit-queue to give better build log information.

I have a Mac laptop, which I'm not using for Mac development because building is very slow, but I will try running it overnight.

[Cosmin Truta]

Actually, I wonder:

As a general rule, when adding and/or removing methods in existing classes, I need not modify anything besides the involved .cpp/.h source files, correct?

I would only need to modify build files (*.mk, CMakeLists.txt, GNUmakefile.am, *.gyp, *.gypi, *.pro, *.vcproj, *.pbxproj) when adding/removing entire files, yes?

The linker error that I'm getting from the Windows try bot (specifically when creating chrome_dll.exp) only makes sense if some list of exported symbols is not updated. Otherwise, the patch has nothing Windows-specific. Perhaps that list (if it exists) is not being automatically updated?

I reissued a git-try command with --clobber enabled. Let's see if forcing a clean build will make a difference.

[Cosmin Truta]

(In reply to comment #25)
> I reissued a git-try command with --clobber enabled. Let's see if forcing a clean build will make a difference.

Success: this time, the Windows build was clean.

I really don't know what could be wrong with this patch.

[Cosmin Truta]

Ok, I run a WebKit build on Mac (without Chromium, just WebKit proper) and compilation dies because, in my patch, I added #include "SVGLocatable.h" in SVGElement.h. Here are the relevant errors:

WebKit/WebKitBuild/Release/WebCore.framework/PrivateHeaders/SVGElement.h:27:26: error: SVGLocatable.h: No such file or directory
WebKit/WebKitBuild/Release/WebCore.framework/PrivateHeaders/SVGElement.h:75: error: 'SVGLocatable' has not been declared

According to the instructions at http://trac.webkit.org/wiki/DeveloperTips, I should add SVGLocatable.h to "Copy headers" in the WebCore target inside the WebCore xcode project. But that header is already referenced in that list. What else could be missing?

[Cosmin Truta]

(In reply to comment #27)
> According to the instructions at http://trac.webkit.org/wiki/DeveloperTips, I should add SVGLocatable.h to "Copy headers" in the WebCore target inside the WebCore xcode project. But that header is already referenced in that list. What else could be missing?

Response from Niko:

"I think, you have to set the role of the file to private. That's done in Xcode, my right clicking on SVGLoctable.h, and executing "Set Role -> Private".
Otherwhise the file is not copied into the PrivateHeaders directory in the framework. You can even manually do that, grep for "Private" in the project.pbxproj files for existing examples.
You just have to copy the SETTINGS=".." stuff to the right SVGLoctable.h location."

Hmm.. Ok. I did that, while saying on the record that this is very unintuitive and very hacky. Not to mention that it requires the developer to have a Mac platform for testing non-Mac-related features.

I will make another submission, shortly.

[Cosmin Truta]

Created attachment 71317 [details]
Fix and layout test, incl. Xcode project fix

I really really hope the build will work well this time.
And if it does, I think I should update the DeveloperTips page in the wiki.

[Eric Seidel (no email)]

Comment on attachment 70941 [details]
Fix and layout test.

Cleared Nikolas Zimmermann's review+ from obsolete attachment 70941 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Cosmin Truta]

(In reply to comment #20)
> I hope you can still work on the svg/zoom/page/ tests? :-)

Yes. I resolved the flakiness by replacing the call to "../resources/testPageZoom.js" with a direct call to window.eventSender.zoomPageIn(). It turns out that the asynchronous nature of testPageZoom.js caused inconsistent results.

On the other hand, the off-by-one errors still remain. I opened the bug 48110 to continue the work over there. The fix for that bug will include a layout test that checks getBoundingClientRect with zooming.

[Nikolas Zimmermann]

Comment on attachment 71317 [details]
Fix and layout test, incl. Xcode project fix

LGTM, r=me.

[WebKit Commit Bot]

Comment on attachment 71317 [details]
Fix and layout test, incl. Xcode project fix

Rejecting patch 71317 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71317]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=71317&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=46775&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 71317 from bug 46775.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Nikolas Zimmermann', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4728008

[Cosmin Truta]

(In reply to comment #33)
> (From update of attachment 71317 [details])
> Rejecting patch 71317 from commit-queue.

This is becoming increasingly painful :-(

[Cosmin Truta]

This time around, I'm completely clueless. I run "WebKitTools/Scripts/webkit-patch apply-attachment 71317 [details]" on a fresh svn checkout on my Mac, and everything went fine, without any issue.

Isn't something broken on the server?

[Dirk Schulze]

Comment on attachment 71317 [details]
Fix and layout test, incl. Xcode project fix

Clearing flags on attachment: 71317

Committed r70317: <http://trac.webkit.org/changeset/70317>

[Dirk Schulze]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

Committed r70323: <http://trac.webkit.org/changeset/70323>