930420 - extents of SVG glyphs fail to take account of a transform applied to the glyph element itself

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

See http://people.mozilla.org/~jkew/tests/svg-glyph-extents.html. The second of the four glyphs here includes a scale(2) transform applied to the element that is tagged as the glyph itself.

This "top-level" transform is (correctly) applied when painting, but is ignored when calculating glyph extents, and therefore we don't repaint the correct area during the animation.

The fourth glyph is similar, except that the scaled path element is enclosed within a <g> element, and this outer element is the one tagged as being the glyph. In this case, the scaling of the path is correctly taken into account, and so repainting works fine.

For reference, the SVG table in the test font contains:
 <svg xmlns='http://www.w3.org/2000/svg'>
   <rect id='glyph36' x='0' y='-1000' width='1000' height='1500' fill='red'/>
   <rect id='glyph37' x='0' y='-1000' width='1000' height='1500' fill='green' transform='scale(2)'/>
   <g id='glyph38'>
     <rect x='0' y='-1000' width='1000' height='1500' fill='red'/>
   </g>
   <g id='glyph39'>
     <rect x='0' y='-1000' width='1000' height='1500' fill='green' transform='scale(2)'/>
   </g>
 </svg>

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: respect any transform on the glyph element itself when calculating SVG glyph extents

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: reftest for SVG glyph extents with a transform on the glyph element

Here's the testcase in reftest form. Unfortunately, this does -not- actually fail when run as a reftest on current trunk, even though it clearly "fails" as viewed on screen. This appears to be a bug in how reftest handles the invalidation/repainting during the animation.

Comment 3

[Cameron McCormack (:heycam)]

Did you try making a reftest that did the same kind of thing that you showed me with the emoji glyphs?  So have a single glyph whose ink bounds go outside its glyph cell, then switch it to a different glyph, and notice that we don't invalid enough.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 821538 [details] [diff] [review]
respect any transform on the glyph element itself when calculating SVG glyph extents

Review of attachment 821538 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGUtils.cpp
@@ +1860,5 @@
>      return false;
>    }
> +
> +  gfxMatrix transform(aSVGToAppSpace);
> +  nsIContent *content = frame->GetContent();

"*" next to the type.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #3)
> Did you try making a reftest that did the same kind of thing that you showed
> me with the emoji glyphs?  So have a single glyph whose ink bounds go
> outside its glyph cell, then switch it to a different glyph, and notice that
> we don't invalid enough.

I tried this approach too, but had the same problem: although I can produce visual glitches on-screen, the reftest framework seems to completely refresh its test canvas at the end of the test, so it doesn't see the residual junk.

This seems to be a bug in reftest itself; I was going to try a bit of debugging, but bug 914925 got in the way...

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 821541 [details] [diff] [review]
reftest for SVG glyph extents with a transform on the glyph element

Jonathan tells me the reftest does in fact fail on the try server.  (I think it fails running locally on HiDPI MBPs due to the reftest harness choosing to repaint the entire frame tree rather than painting the layers.)

(BTW you don't need type="text/css" on your <style> element.)

Comment 7

[Jonathan Kew [:jfkthame]]

Yup, the reftest failed nicely on try:
https://tbpl.mozilla.org/?tree=Try&rev=347043738e2d

and passed as expected once the patch is applied:
https://tbpl.mozilla.org/?tree=Try&rev=0b7eb75f8d02

Pushed patch + reftest to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef91147d327
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44c7313305e

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9ef91147d327
https://hg.mozilla.org/mozilla-central/rev/f44c7313305e