Bug 46775 - getBoundingClientRect does not work with SVG <text>
Summary: getBoundingClientRect does not work with SVG <text>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-28 19:22 PDT by Cosmin Truta
Modified: 2010-10-22 12:01 PDT (History)
5 users (show)

See Also:


Attachments
Test case (1.32 KB, image/svg+xml)
2010-09-30 16:57 PDT, Cosmin Truta
no flags Details
Fix, layout test and code cleanup (9.32 KB, patch)
2010-10-05 11:13 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
The same patch as before, with just a little more cleanup (two indentation fixes in JavaScript) (9.94 KB, patch)
2010-10-05 12:36 PDT, Cosmin Truta
zimmermann: review-
Details | Formatted Diff | Diff
Fix and augmented layout test (14.46 KB, patch)
2010-10-07 21:19 PDT, Cosmin Truta
zimmermann: review-
Details | Formatted Diff | Diff
Fix and layout test (14.43 KB, patch)
2010-10-08 06:01 PDT, Cosmin Truta
zimmermann: review-
Details | Formatted Diff | Diff
Fix and layout test, ChangeLog fixed (14.41 KB, patch)
2010-10-08 06:42 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
Fix and layout test (14.41 KB, patch)
2010-10-08 09:43 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
Fix and layout test. (14.32 KB, patch)
2010-10-15 18:34 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff
Fix and layout test, incl. Xcode project fix (15.94 KB, patch)
2010-10-20 12:14 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cosmin Truta 2010-09-28 19:22:22 PDT
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
Comment 1 Cosmin Truta 2010-09-30 16:57:51 PDT
Created attachment 69398 [details]
Test case

Oops, forgot to attach the test case. Here it is.
Comment 2 Cosmin Truta 2010-09-30 17:09:28 PDT
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?
Comment 3 Cosmin Truta 2010-10-05 11:13:15 PDT
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.
Comment 4 Cosmin Truta 2010-10-05 12:36:20 PDT
Created attachment 69828 [details]
The same patch as before, with just a little more cleanup (two indentation fixes in JavaScript)
Comment 5 Nikolas Zimmermann 2010-10-06 00:00:52 PDT
(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 :-)
Comment 6 Nikolas Zimmermann 2010-10-06 00:10:49 PDT
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.
Comment 7 Cosmin Truta 2010-10-07 21:19:02 PDT
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.
Comment 8 Nikolas Zimmermann 2010-10-08 01:48:42 PDT
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.
Comment 9 Nikolas Zimmermann 2010-10-08 01:50:16 PDT
(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 :-)
Comment 10 Cosmin Truta 2010-10-08 06:01:30 PDT
Created attachment 70234 [details]
Fix and layout test

s/boundingBoxForElement/boundingBox/
Comment 11 Nikolas Zimmermann 2010-10-08 06:41:31 PDT
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"...>
Comment 12 Cosmin Truta 2010-10-08 06:42:46 PDT
Created attachment 70242 [details]
Fix and layout test, ChangeLog fixed

s/boundingBoxForElement/boundingBox/ inside ChangeLog
Comment 13 Nikolas Zimmermann 2010-10-08 07:00:52 PDT
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!
Comment 14 WebKit Commit Bot 2010-10-08 07:29:43 PDT
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
Comment 15 Cosmin Truta 2010-10-08 09:43:34 PDT
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".
Comment 16 Eric Seidel (no email) 2010-10-11 01:50:06 PDT
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.
Comment 17 Cosmin Truta 2010-10-11 09:13:15 PDT
(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.
Comment 18 Eric Seidel (no email) 2010-10-13 15:51:13 PDT
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.
Comment 19 Cosmin Truta 2010-10-15 18:34:55 PDT
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.)
Comment 20 Nikolas Zimmermann 2010-10-17 12:34:57 PDT
Comment on attachment 70941 [details]
Fix and layout test.

r=me. I hope you can still work on the svg/zoom/page/ tests? :-)
Comment 21 WebKit Commit Bot 2010-10-17 13:10:45 PDT
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
Comment 22 Cosmin Truta 2010-10-18 08:30:49 PDT
(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.
Comment 23 Eric Seidel (no email) 2010-10-18 08:58:50 PDT
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.
Comment 24 Cosmin Truta 2010-10-18 12:55:31 PDT
(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.
Comment 25 Cosmin Truta 2010-10-18 13:53:09 PDT
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.
Comment 26 Cosmin Truta 2010-10-18 18:27:42 PDT
(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.
Comment 27 Cosmin Truta 2010-10-19 22:37:46 PDT
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?
Comment 28 Cosmin Truta 2010-10-20 10:10:51 PDT
(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.
Comment 29 Cosmin Truta 2010-10-20 12:14:38 PDT
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.
Comment 30 Eric Seidel (no email) 2010-10-21 08:45:09 PDT
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.
Comment 31 Cosmin Truta 2010-10-21 21:55:43 PDT
(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.
Comment 32 Nikolas Zimmermann 2010-10-22 05:23:22 PDT
Comment on attachment 71317 [details]
Fix and layout test, incl. Xcode project fix

LGTM, r=me.
Comment 33 WebKit Commit Bot 2010-10-22 05:52:33 PDT
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 commit-queue@webkit.org...
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
Comment 34 Cosmin Truta 2010-10-22 08:12:12 PDT
(In reply to comment #33)
> (From update of attachment 71317 [details])
> Rejecting patch 71317 from commit-queue.

This is becoming increasingly painful :-(
Comment 35 Cosmin Truta 2010-10-22 09:23:18 PDT
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?
Comment 36 Dirk Schulze 2010-10-22 10:43:59 PDT
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>
Comment 37 Dirk Schulze 2010-10-22 10:44:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 38 Dirk Schulze 2010-10-22 12:01:13 PDT
Committed r70323: <http://trac.webkit.org/changeset/70323>