520506 - Incorrect text getBBox in SVG

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Vincent Hardy]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6; en-us) AppleWebKit/531.9 (KHTML, like Gecko) Version/4.0.3 Safari/531.9
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3

In Firefox, getBBox on text elements returns the text's visual bounds, and does not account for the text's ascent and descent. From the SVG specification, section 7.11
(http://www.w3.org/TR/SVG/coords.html#ObjectBoundingBox), the full
glyph cell should be accounted for:

"For 'text' elements, for the purposes of the bounding box calculation, each
glyph is treated as a separate graphics element. The calculations assume that
all glyphs occupy the full glyph cell. For example, for horizontal text, the
calculations assume that each glyph extends vertically to the full ascent and
descent values for the font."


Reproducible: Always

Steps to Reproduce:
1. var elt = getElementById("myTextElementId"); // e.g., text with "Hello world"
    var elt2 = getElementById("myOtherTextElementId"); // e.g., text with "The quick brown fox"
2. var bbox = elt.getBBox(); 
    var bbox2 = elt2.getBBox();
3. alert(bbox.height  + " / " + bbox2.height)
Actual Results:  
The text's bounding box is the tight bounds around the text content.

Expected Results:  
Account for the full font's descent/ascent.

Comment 1

[Robert Longson [:longsonr]]

It would be great if you could attach a complete testcase using the Add an attachment facility.

Comment 2

[Vincent Hardy]

Attachment: SVG file illustrating the issue.

The file contains text-bbox.svg and text-bbox.js to reproduce the issue, along with text-bbox.png to show the expected bbox.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

Comment 5

[Jonathan Watt [:jwatt]]

What's the problem with AddBoundingBoxesToPath in this case?

Comment 6

[Robert Longson [:longsonr]]

AddBoundingBoxesToPath gives you the extents of the full glyph cell assuming that each glyph extends vertically to the full ascent and descent values for the font. AddCharactersToPath gives you the actual extent of the character.

For getBBox according to the spec reference in comment 0 we want the former. For the covered region we want something as small as possible for best performance, hence the latter is more appropriate.

Comment 7

[Robert Longson [:longsonr]]

AddBoundingBoxesToPath gives you the extents of the full glyph cell assuming that each glyph extends vertically to the full ascent and descent values for the font. AddCharactersToPath gives you the actual extent of the character.

For getBBox according to the spec reference in comment 0 we want the former. For the covered region we want something as small as possible for best performance, hence the latter is more appropriate.

Comment 8

[Robert Longson [:longsonr]]

Attachment: patch

Comment 9

[Jonathan Watt [:jwatt]]

Ah, yes indeed. I misread your patch initially and thought you were changing AddBoundingBoxesToPath to AddCharactersToPath throughout, which of course would make no sense to solve this bug. :-)

I'm not sure about changing to AddCharactersToPath for covered region calculations. I think we probably won't save much in terms of invalidation area, and unfortunately it adds a fair bit to the expense of the calculation of the area itself. Or do you disagree?

Comment 10

[Robert Longson [:longsonr]]

Attachment: patch

Unfortunately some characters in some fonts extend outside the bounding box (bug 525095 has an example). However I probably should address that bug there.

Comment 11

[Jonathan Watt [:jwatt]]

So changing to AddCharactersToPath for covered region calculation would fix some artifact bugs? :-(

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 412792 [details] [diff] [review]
patch

Anyway, this patch looks great for this bug. r=jwatt

Comment 13

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/mozilla-central/rev/af81717d2be9

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 412792 [details] [diff] [review]
patch

Requesting approval1.9.2. Fixes a common complaint, standards compliance issue, with one line patch exercising a path we already exercise in another way. Very safe.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 412792 [details] [diff] [review]
patch

a192=beltzner

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/44750cd255a6