Bug 78037 – inspector highlight of SVG root element with viewbox does not match dimensions of element

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

[Max Vujovic]

Created attachment 125921 [details]
Reproduction

When an SVG root element has a viewbox defined with dimensions not equal to the element's dimensions, the SVG root element's web inspector highlight is rendered with different dimensions. The highlight dimensions should match the element's dimensions.

Please see the attached reproduction for an example.

[Max Vujovic]

Created attachment 128082 [details]
Patch for EWS bots. Do not review.

Running the EWS bots on a potential fix.

[Max Vujovic]

Created attachment 128499 [details]
Reproduction with paddings, borders, and margins.

Adding another reproduction. The issue is more obvious when the SVG root element has paddings, borders, and margins.

[Max Vujovic]

Created attachment 128501 [details]
Screenshot of reproduction with paddings, borders, and margins.

[Nikolas Zimmermann]

The patch is interesting, I'd be happy once you explain it :-)

[Max Vujovic]

(In reply to comment #4)
> The patch is interesting, I'd be happy once you explain it :-)

Hi Niko!

I'm still figuring out how to write the inspector tests for this bug, but here's my first attempt at an explanation of the patch. Comments welcome :-) :

Functions like RenderBox::absoluteQuads and DOMNodeHighlighter::getOrDrawNodeHighlight call RenderSVGRoot::localToAbsolute, which calls RenderSVGRoot::mapLocalToContainer. These functions pass RenderSVGRoot's local CSS box coordinates to RenderSVGRoot::mapLocalToContainer, using RenderBox::width, RenderBox::contentBox, and other RenderBox methods on RenderSVGRoot. However, before this patch, RenderSVGRoot::mapLocalToContainer expected local SVG viewport coordinates, not local CSS box coordinates. This caused RenderSVGRoot::mapLocalToContainer to unnecessarily apply the localToBorderTransform, which includes the viewBox and page zoom transforms, to RenderSVGRoot's CSS box coordinates. This resulted in an incorrectly sized and positioned inspector highlight for root SVG elements, when a viewBox or page zoom was used.

After this patch, RenderSVGRoot::mapLocalToContainer expects local CSS box coordinates, like other HTML renderers. RenderSVGRoot::mapLocalToContainer does not apply the localToBorderBoxTransform anymore, which previously mapped local SVG viewport coordinates to local border box coordinates. Instead, RenderSVGRoot::localToBorderBoxTransform has been exposed for child elements in the SVG namespace to use when mapping to CSS box coordinates. This mapping occurs in SVGRenderSupport::mapLocalToContainer. This kind of mapping also occurs in SVGSVGElement::localCoordinateSpaceTransform, which getScreenCTM eventually calls.

[Nikolas Zimmermann]

(In reply to comment #5)
> (In reply to comment #4)
> > The patch is interesting, I'd be happy once you explain it :-)
> 
> Hi Niko!
> 
> I'm still figuring out how to write the inspector tests for this bug, but here's my first attempt at an explanation of the patch. Comments welcome :-) :
We should ask some inspector guys on IRC. Pavel Feldman might know for instance.
I bet there are existing tests for that though you could use as template? Never wrote an inspector test so far.
 
> Functions like RenderBox::absoluteQuads and DOMNodeHighlighter::getOrDrawNodeHighlight call RenderSVGRoot::localToAbsolute, which calls RenderSVGRoot::mapLocalToContainer. These functions pass RenderSVGRoot's local CSS box coordinates to RenderSVGRoot::mapLocalToContainer, using RenderBox::width, RenderBox::contentBox, and other RenderBox methods on RenderSVGRoot. However, before this patch, RenderSVGRoot::mapLocalToContainer expected local SVG viewport coordinates, not local CSS box coordinates. This caused RenderSVGRoot::mapLocalToContainer to unnecessarily apply the localToBorderTransform, which includes the viewBox and page zoom transforms, to RenderSVGRoot's CSS box coordinates. This resulted in an incorrectly sized and positioned inspector highlight for root SVG elements, when a viewBox or page zoom was used.
Excellent analysis. I think you should leave a comment in those functions in your patch, to make it easier for anyone reading this code to follow. Especially the boundary points where we move between coordinate systems should be commented.
 
> After this patch, RenderSVGRoot::mapLocalToContainer expects local CSS box coordinates, like other HTML renderers. RenderSVGRoot::mapLocalToContainer does not apply the localToBorderBoxTransform anymore, which previously mapped local SVG viewport coordinates to local border box coordinates. Instead, RenderSVGRoot::localToBorderBoxTransform has been exposed for child elements in the SVG namespace to use when mapping to CSS box coordinates. This mapping occurs in SVGRenderSupport::mapLocalToContainer. This kind of mapping also occurs in SVGSVGElement::localCoordinateSpaceTransform, which getScreenCTM eventually calls.
Excellent, understood your patch :-) Please reupload with tests and ChangeLog once you're done, I'm happy to review this.

[Max Vujovic]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > The patch is interesting, I'd be happy once you explain it :-)
> > 
> > Hi Niko!
> > 
> > I'm still figuring out how to write the inspector tests for this bug, but here's my first attempt at an explanation of the patch. Comments welcome :-) :
> We should ask some inspector guys on IRC. Pavel Feldman might know for instance.

Yes, I've started discussing inspector tests with Pavel on webkit-dev. Here's the thread:
https://lists.webkit.org/pipermail/webkit-dev/2012-February/019554.html

> I bet there are existing tests for that though you could use as template? Never wrote an inspector test so far.

I haven't written an inspector test before either. Pavel pointed me to an impressive inspector test harness and lots of tests in LayoutTests/inspector and LayoutTests/http/tests/inspector. 

Currently, there are no inspector tests that cover DOM node highlights. The existing inspector tests are all programmatic, and there are no pixel tests yet. 

I discussed exposing the position and dimensions of the highlights to the inspector to enable programmatic highlight tests, but Pavel thought pixel tests are a better idea. I agree that pixel tests provide better coverage, and they don't require changes to the inspector code.

I've been experimenting with DRT and the inspector test harness to try get a snapshot of the inspector highlight, but I haven't succeeded yet. I can capture the inspector and the page in the snapshot, but not the highlight. I'll have to ask Pavel for some more help in the next couple of days.

> > Functions like RenderBox::absoluteQuads and DOMNodeHighlighter::getOrDrawNodeHighlight call RenderSVGRoot::localToAbsolute, which calls RenderSVGRoot::mapLocalToContainer. These functions pass RenderSVGRoot's local CSS box coordinates to RenderSVGRoot::mapLocalToContainer, using RenderBox::width, RenderBox::contentBox, and other RenderBox methods on RenderSVGRoot. However, before this patch, RenderSVGRoot::mapLocalToContainer expected local SVG viewport coordinates, not local CSS box coordinates. This caused RenderSVGRoot::mapLocalToContainer to unnecessarily apply the localToBorderTransform, which includes the viewBox and page zoom transforms, to RenderSVGRoot's CSS box coordinates. This resulted in an incorrectly sized and positioned inspector highlight for root SVG elements, when a viewBox or page zoom was used.
> Excellent analysis. I think you should leave a comment in those functions in your patch, to make it easier for anyone reading this code to follow. Especially the boundary points where we move between coordinate systems should be commented.

Thanks, Niko. That's a good idea. I'll make sure to add comments, especially at the boundary points where the definition of "local" changes between the CSS box and the SVG viewport.

> > After this patch, RenderSVGRoot::mapLocalToContainer expects local CSS box coordinates, like other HTML renderers. RenderSVGRoot::mapLocalToContainer does not apply the localToBorderBoxTransform anymore, which previously mapped local SVG viewport coordinates to local border box coordinates. Instead, RenderSVGRoot::localToBorderBoxTransform has been exposed for child elements in the SVG namespace to use when mapping to CSS box coordinates. This mapping occurs in SVGRenderSupport::mapLocalToContainer. This kind of mapping also occurs in SVGSVGElement::localCoordinateSpaceTransform, which getScreenCTM eventually calls.
> Excellent, understood your patch :-) Please reupload with tests and ChangeLog once you're done, I'm happy to review this.

Great! I'll reupload as soon as I figure out those tests. Thank you for all your help :-)

[Max Vujovic]

Created attachment 131925 [details]
Patch for review after bots cycle.

I'm going to run the bots on this patch and then set r?, cq?

This patch contains all the same code changes as the previous one. It additionally includes:
- ChangeLog entries.
- A regression test for the inspector highlight on an SVG root element. The element has a viewBox, borders, paddings, and margins. (This kind of test is possible after window.internals.inspectorHighlightRects was exposed in bug 80338.)
- Comments for RenderSVGRoot::localToBorderBoxTransform, RenderSVGRoot::mapLocalToContainer, and the code that transforms across the SVG/HTML boundary.

[Max Vujovic]

Comment on attachment 131925 [details]
Patch for review after bots cycle.

Looks like the bots are happy, setting r?, cq?. (Only efl has a long queue right now).

[Nikolas Zimmermann]

Comment on attachment 131925 [details]
Patch for review after bots cycle.

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

Looks good, r=me!

> LayoutTests/inspector/elements/highlight-svg-root.html:51
> +<svg id="svg-root" width="100" height="100" viewbox="0 0 50 50"></svg>

s/viewBox/viewBox/

[WebKit Review Bot]

Comment on attachment 131925 [details]
Patch for review after bots cycle.

Clearing flags on attachment: 131925

Committed r111176: <http://trac.webkit.org/changeset/111176>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

Well done :)

[Max Vujovic]

(In reply to comment #10)
> (From update of attachment 131925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131925&action=review
> 
> Looks good, r=me!
> 
> > LayoutTests/inspector/elements/highlight-svg-root.html:51
> > +<svg id="svg-root" width="100" height="100" viewbox="0 0 50 50"></svg>
> 
> s/viewBox/viewBox/

Thanks for the review! I'll make those changes and put up a follow-up patch.

[Max Vujovic]

Created attachment 132643 [details]
Follow-up patch that changes "viewbox" to "viewBox" in the test.

[Max Vujovic]

Created attachment 132657 [details]
Follow-up patch that changes "viewbox" to "viewBox" in the test.

Bots were stuck on the previous patch, so I'm trying again after updating.

[Nikolas Zimmermann]

(In reply to comment #16)
> Created an attachment (id=132657) [details]
> Follow-up patch that changes "viewbox" to "viewBox" in the test.
> 
> Bots were stuck on the previous patch, so I'm trying again after updating.

You only uploaded a delta patch to the previous one, not a full one I could r+/cq+. Or did this already land (as this bug is marked as fixed?).

[Max Vujovic]

Created attachment 132848 [details]
Follow-up patch that changes "viewbox" to "viewBox" in the test.

(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=132657) [details] [details]
> > Follow-up patch that changes "viewbox" to "viewBox" in the test.
> > 
> > Bots were stuck on the previous patch, so I'm trying again after updating.
> 
> You only uploaded a delta patch to the previous one, not a full one I could r+/cq+. Or did this already land (as this bug is marked as fixed?).

The first patch for this bug already landed, so I uploaded only a delta patch. 

The patch applies correctly locally, but the bots seemed to be stuck on it, so I'm reuploading now to try again.

[Dirk Schulze]

Maybe the bots are out of sync?

[Dirk Schulze]

Another reason might be the status of the bug. I can land it manually tomorrow.

[Max Vujovic]

(In reply to comment #20)
> Another reason might be the status of the bug. I can land it manually tomorrow.

That sounds like the most likely explanation so far. Thanks Dirk :)