Created attachment 127041 [details] Reproduction Please see the attached reproduction for an example. Note that the reproduction works properly in Opera.
Created attachment 127191 [details] Patch Added a proposed patch. The root cause of this bug was that SVGSVGElement::currentViewportSize was returning the size of the SVG viewport plus CSS borders and paddings, causing the CTM calculation to be off. In the patch, SVGSVGElement::currentViewportSize uses contentBoxRect instead of frameRect to compute the viewport size for SVG root elements.
Attachment 127191 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource git.webkit.org[0: 17.254.20.231]: errno=Connection refused fatal: unable to connect a socket (Connection refused) Died at Tools/Scripts/update-webkit line 162. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 127210 [details] Patch Trying the EWS bots again.
Comment on attachment 127210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127210&action=review Nice patch Max, almost there! > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:12 > + <script src="../../fast/js/resources/js-test-pre.js"></script> I think it would be helpful, if this testcase would use "window.enablePixelTesting = true" before including js-test-pre.js, to dump pixels as well. > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:32 > + <svg id="svgRoot1" xmlns="http://www.w3.org/2000/svg" Inside a HTML5 document, no xmlns is needed here. > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:34 > + viewbox="0 0 100 100""> Oh, when the HTML5 parses parses this file, lower case attributes are allowed?? The correct name is 'viewBox'. I'm not sure if HTML5 explicitly allows this, if not please use "viewBox". > Source/WebCore/ChangeLog:22 > + SVGSVGElement::currentViewportSize was using the SVG root element's frameRect to compute > + the viewport size. However, the frameRect includes borders and paddings, which should > + not be considered part of the SVG viewport. > + > + SVGSVGElement::currentViewportSize now uses the contentBoxRect instead of the frameRect. > + The contentBoxRect corresponds to the SVG viewport and does not include borders and > + paddings. Double explanation, either is okay. > Source/WebCore/svg/SVGSVGElement.cpp:557 > FloatRect frameRect = toRenderSVGViewportContainer(renderer())->viewport(); I'd rename this as well for consistency to avoid confusion.
(In reply to comment #4) > (From update of attachment 127210 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127210&action=review > Oh, when the HTML5 parses parses this file, lower case attributes are allowed?? The correct name is 'viewBox'. > I'm not sure if HTML5 explicitly allows this, if not please use "viewBox". It allows it, yes. Everything is not case sensitive on HTML5. That is a great problem for us on other places. We have a bug report for "linearGradient" where is causes problems. Don't know the exact problem right now.
Created attachment 127263 [details] Patch (In reply to comment #4) > (From update of attachment 127210 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127210&action=review > > Nice patch Max, almost there! Thanks Nikolas! > > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:12 > > + <script src="../../fast/js/resources/js-test-pre.js"></script> > > I think it would be helpful, if this testcase would use "window.enablePixelTesting = true" before including js-test-pre.js, to dump pixels as well. I've added this. Please forgive my newness, but what exactly does window.enablePixelTesting do? > > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:32 > > + <svg id="svgRoot1" xmlns="http://www.w3.org/2000/svg" > > Inside a HTML5 document, no xmlns is needed here. > > > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:34 > > + viewbox="0 0 100 100""> > > Oh, when the HTML5 parses parses this file, lower case attributes are allowed?? The correct name is 'viewBox'. > I'm not sure if HTML5 explicitly allows this, if not please use "viewBox". I kept this as "viewbox" since the convention for HTML5 seems to be keeping all attribute names lowercase. The closest mention I found in the HTML5 spec was section 4.14.1 Case-sensitivity, which implies that "viewbox" should work: "Attribute and element names of HTML elements in HTML documents must be treated as ASCII case-insensitive for the purposes of selector matching." (http://www.whatwg.org/specs/web-apps/current-work/multipage/selectors.html#case-sensitivity) Some search results I found suggest that lowercase is the convention: "HTML5 attributes are case insensitive and may be written in all uppercase or mixed case, although the most common convention is to stick with lowercase." (http://www.tutorialspoint.com/html5/html5_syntax.htm) > > Source/WebCore/ChangeLog:22 > > + SVGSVGElement::currentViewportSize was using the SVG root element's frameRect to compute > > + the viewport size. However, the frameRect includes borders and paddings, which should > > + not be considered part of the SVG viewport. > > + > > + SVGSVGElement::currentViewportSize now uses the contentBoxRect instead of the frameRect. > > + The contentBoxRect corresponds to the SVG viewport and does not include borders and > > + paddings. > > Double explanation, either is okay. Removed the first paragraph. > > Source/WebCore/svg/SVGSVGElement.cpp:557 > > FloatRect frameRect = toRenderSVGViewportContainer(renderer())->viewport(); > > I'd rename this as well for consistency to avoid confusion. Good idea. I renamed "frameRect" to "viewportRect" in this section.
(In reply to comment #4) > (From update of attachment 127210 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127210&action=review > > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:32 > > + <svg id="svgRoot1" xmlns="http://www.w3.org/2000/svg" > > Inside a HTML5 document, no xmlns is needed here. Oops, forgot to mention that I changed this as well. Now all the SVG tags fit on one line :)
Comment on attachment 127263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127263&action=review r- because the pixel result is missing now. You enabled it explicitly with window.enablePixelTesting > LayoutTests/ChangeLog:9 > + * svg/dom/SVGLocatable-getCTM-svg-root-expected.txt: Added. > + * svg/dom/SVGLocatable-getCTM-svg-root.html: Added. This is missing the pixel result now.
(In reply to comment #8) > (From update of attachment 127263 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127263&action=review > > r- because the pixel result is missing now. You enabled it explicitly with window.enablePixelTesting > > > LayoutTests/ChangeLog:9 > > + * svg/dom/SVGLocatable-getCTM-svg-root-expected.txt: Added. > > + * svg/dom/SVGLocatable-getCTM-svg-root.html: Added. > > This is missing the pixel result now. Dirk - I'll update the patch with the pixel results. I'll also rearrange the elements in the test case so that they all fit in 800x600. Nikolas - Dirk taught me how to create pixel tests, so no need to explain anymore :).
Created attachment 127424 [details] Patch - Updated the patch with the png expectation file under the platform/mac folder. I kept the expected.txt file in the same folder as the test because the text results should be the same on all platforms. - Reduced the sizes of the elements in the test case so that they fit in the 800x600 snapshot. - Removed the red rects that were previously under the SVG root elements. The red rects will not be rendered with the correct dimensions until bug 78613 is fixed, and they are not essential to testing getCTM() on the SVG root. Bug 78613 will provide specific tests for rendering SVG elements under an SVG root element with borders, paddings, and margins.
Comment on attachment 127424 [details] Patch LGTM.
(In reply to comment #11) > (From update of attachment 127424 [details]) > LGTM. Thanks, Eric!
Comment on attachment 127424 [details] Patch Attachment 127424 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11534981 New failing tests: svg/dom/SVGLocatable-getCTM-svg-root.html
Comment on attachment 127424 [details] Patch Phew, glad Eric gave r+ and not me - chromium expectations were not updated ;-)
(In reply to comment #14) > (From update of attachment 127424 [details]) > Phew, glad Eric gave r+ and not me - chromium expectations were not updated ;-) Sorry guys, this is my first patch with pixel tests. Nikolas - I looked at bug 34714 where you described the procedure for generating chromium baselines. I'll do that, reupload a patch, and keep you from repeating yourself :)
Created attachment 127626 [details] Patch Trying again with the chromium bot. Added the following lines to platform/chromium/test_expectations.txt: +// New test, needs a rebaseline +BUGWK78631 : svg/dom/SVGLocatable-getCTM-svg-root.html = IMAGE Also, updated LayoutTests/ChangeLog to mention test_expectations.txt.
Comment on attachment 127626 [details] Patch Passed cr-linux. Setting r?, cq?.
Comment on attachment 127626 [details] Patch Clearing flags on attachment: 127626 Committed r108441: <http://trac.webkit.org/changeset/108441>
All reviewed patches have been landed. Closing bug.
Committed r114411: <http://trac.webkit.org/changeset/114411>
(In reply to comment #20) > Committed r114411: <http://trac.webkit.org/changeset/114411> Can you be more explicit what you committed in the future please? This makes it easier to follow bug reports :) (Especially because the patch was already committed and you just updated the results).