Bug 78631 – getCTM() on SVG root element with borders, paddings, and viewbox returns incorrect values

• Component: SVG
• Status: RESOLVED FIXED

[Max Vujovic]

Created attachment 127041 [details]
Reproduction

Please see the attached reproduction for an example.

Note that the reproduction works properly in Opera.

[Max Vujovic]

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.

[WebKit Review Bot]

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.

[Max Vujovic]

Created attachment 127210 [details]
Patch

Trying the EWS bots again.

[Nikolas Zimmermann]

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.

[Dirk Schulze]

(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.

[Max Vujovic]

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.

[Max Vujovic]

(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 :)

[Dirk Schulze]

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.

[Max Vujovic]

(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 :).

[Max Vujovic]

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.

[Eric Seidel (no email)]

Comment on attachment 127424 [details]
Patch

LGTM.

[Max Vujovic]

(In reply to comment #11)
> (From update of attachment 127424 [details])
> LGTM.

Thanks, Eric!

[WebKit Review Bot]

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

[Nikolas Zimmermann]

Comment on attachment 127424 [details]
Patch

Phew, glad Eric gave r+ and not me - chromium expectations were not updated ;-)

[Max Vujovic]

(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 :)

[Max Vujovic]

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.

[Max Vujovic]

Comment on attachment 127626 [details]
Patch

Passed cr-linux. Setting r?, cq?.

[WebKit Review Bot]

Comment on attachment 127626 [details]
Patch

Clearing flags on attachment: 127626

Committed r108441: <http://trac.webkit.org/changeset/108441>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen Chenney]

Committed r114411: <http://trac.webkit.org/changeset/114411>

[Dirk Schulze]

(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).