Bug 46775

Summary: getBoundingClientRect does not work with SVG <text>
Product: WebKit Reporter: Cosmin Truta <ctruta>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, krit, mdelaney7, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case
none
Fix, layout test and code cleanup
none
The same patch as before, with just a little more cleanup (two indentation fixes in JavaScript)
zimmermann: review-
Fix and augmented layout test
zimmermann: review-
Fix and layout test
zimmermann: review-
Fix and layout test, ChangeLog fixed
none
Fix and layout test
none
Fix and layout test.
none
Fix and layout test, incl. Xcode project fix none

Cosmin Truta
Reported 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
Attachments
Test case (1.32 KB, image/svg+xml)
2010-09-30 16:57 PDT, Cosmin Truta
no flags
Fix, layout test and code cleanup (9.32 KB, patch)
2010-10-05 11:13 PDT, Cosmin Truta
no flags
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-
Fix and augmented layout test (14.46 KB, patch)
2010-10-07 21:19 PDT, Cosmin Truta
zimmermann: review-
Fix and layout test (14.43 KB, patch)
2010-10-08 06:01 PDT, Cosmin Truta
zimmermann: review-
Fix and layout test, ChangeLog fixed (14.41 KB, patch)
2010-10-08 06:42 PDT, Cosmin Truta
no flags
Fix and layout test (14.41 KB, patch)
2010-10-08 09:43 PDT, Cosmin Truta
no flags
Fix and layout test. (14.32 KB, patch)
2010-10-15 18:34 PDT, Cosmin Truta
no flags
Fix and layout test, incl. Xcode project fix (15.94 KB, patch)
2010-10-20 12:14 PDT, Cosmin Truta
no flags
Cosmin Truta
Comment 1 2010-09-30 16:57:51 PDT
Created attachment 69398 [details] Test case Oops, forgot to attach the test case. Here it is.
Cosmin Truta
Comment 2 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?
Cosmin Truta
Comment 3 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.
Cosmin Truta
Comment 4 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)
Nikolas Zimmermann
Comment 5 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 :-)
Nikolas Zimmermann
Comment 6 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.
Cosmin Truta
Comment 7 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.
Nikolas Zimmermann
Comment 8 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.
Nikolas Zimmermann
Comment 9 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 :-)
Cosmin Truta
Comment 10 2010-10-08 06:01:30 PDT
Created attachment 70234 [details] Fix and layout test s/boundingBoxForElement/boundingBox/
Nikolas Zimmermann
Comment 11 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"...>
Cosmin Truta
Comment 12 2010-10-08 06:42:46 PDT
Created attachment 70242 [details] Fix and layout test, ChangeLog fixed s/boundingBoxForElement/boundingBox/ inside ChangeLog
Nikolas Zimmermann
Comment 13 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!
WebKit Commit Bot
Comment 14 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
Cosmin Truta
Comment 15 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".
Eric Seidel (no email)
Comment 16 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.
Cosmin Truta
Comment 17 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.
Eric Seidel (no email)
Comment 18 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.
Cosmin Truta
Comment 19 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.)
Nikolas Zimmermann
Comment 20 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? :-)
WebKit Commit Bot
Comment 21 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
Cosmin Truta
Comment 22 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.
Eric Seidel (no email)
Comment 23 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.
Cosmin Truta
Comment 24 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.
Cosmin Truta
Comment 25 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.
Cosmin Truta
Comment 26 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.
Cosmin Truta
Comment 27 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?
Cosmin Truta
Comment 28 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.
Cosmin Truta
Comment 29 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.
Eric Seidel (no email)
Comment 30 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.
Cosmin Truta
Comment 31 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.
Nikolas Zimmermann
Comment 32 2010-10-22 05:23:22 PDT
Comment on attachment 71317 [details] Fix and layout test, incl. Xcode project fix LGTM, r=me.
WebKit Commit Bot
Comment 33 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
Cosmin Truta
Comment 34 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 :-(
Cosmin Truta
Comment 35 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?
Dirk Schulze
Comment 36 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>
Dirk Schulze
Comment 37 2010-10-22 10:44:12 PDT
All reviewed patches have been landed. Closing bug.
Dirk Schulze
Comment 38 2010-10-22 12:01:13 PDT
Note You need to log in before you can comment on or make changes to this bug.