777476 - SVGLocatable.getBBox() fails with a <text><tspan> nodes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Martin Hejral]

Attachment: BBox-test.svg

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

//node.getBBox() call fail in FF from version 13 up to 16
var node = document.getElementById("text_tspan");
var bb = node.getBBox();
alert("BBox of <text_tspan>:   "+bb.x+", "+bb.y+", "+bb.width+", "+(bb.height));


Actual results:

Fails when called with <text><tspan> SVG DOM node construct, e.g.

<text id='text_tspan' font-size='2.80' fill='red'>
<tspan x='22' y='-1'>[22,-1]</tspan>
<tspan x='88' y='-88' text-anchor='end'>[88,-88]</tspan>
</text>

returns BBox of <text_tspan>:   0, -90.8125, 88.1875, 90.8125


Expected results:

result should be BBox of <text_tspan>:   22, -91, 66, 91

Comment 1

[Martin Hejral]

Comment on attachment 645860 [details]
BBox-test.svg

.getBBox() fails with:

<text id='text_tspan' font-size='2.80' fill='red'>
<tspan x='22' y='-1'>[22,-1]</tspan>
<tspan x='88' y='-88' text-anchor='end'>[88,-88]</tspan>
</text>

...but this node returns correct BBox:

<text id='text_only' font-size='2.80' x='22' y='-88' fill='yellow'>
[22,-88]
</text>

Comment 2

[Martin Hejral]

Operation getBBox() is very important for dynamic web apps. We develop huge documentation system and such bug causes fatal problems...

Bug appears until FF 13 up to 16...

Many thanks for help!!

Comment 3

[Robert Longson [:longsonr]]

Would you be willing to find a regression range?

Comment 4

[Robert Longson [:longsonr]]

Do do that you would follow the instructions here: https://quality.mozilla.org/docs/bugzilla/guide-to-triaging-bugs-for-firefox/finding-a-regression-window/

type about:buildconfig in the address bar in the last good build and first bad build and post the Built from link from each

Comment 5

[Martin Hejral]

Pardon, right now, after additional test on Windows XP, it seems that FF13 was still OK, but FF14 already fails...

Comment 6

[Martin Hejral]

(In reply to Robert Longson from comment #4)
> Do do that you would follow the instructions here:
> https://quality.mozilla.org/docs/bugzilla/guide-to-triaging-bugs-for-firefox/
> finding-a-regression-window/
> 
> type about:buildconfig in the address bar in the last good build and first
> bad build and post the Built from link from each

thx for info, I'm trying...

Comment 7

[Robert Longson [:longsonr]]

Around February/March this year then.

Comment 8

[Loic]

(In reply to Robert Longson from comment #3)
> Would you be willing to find a regression range?
Testcase:
Before regression: BBox of <text_tspan>:   0, -90.75, 88, 90.75
After regression: BBox of <text_tspan>:   22, -90.75, 66, 90.375

m-i
good=2012-01-23
bad=2012-01-24
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e170001f3e9b&tochange=388edf50e323


It's a regression of:
Robert Longson — Bug 647914 - Horizontal and vertical SVG paths are omitted from bbox calculations if they have siblings. r=jwatt

Comment 9

[Loic]

Sorry, I mixed "Before" and "After", just switch them.

Comment 10

[Robert Longson [:longsonr]]

Attachment: patch

Comment 11

[Jonathan Watt [:jwatt]]

The changes to SetInitialMatrix look like a good idea, but why are the necessary in this case? I can't see anything in the testcase that would result in a singular matrix.

Comment 12

[Robert Longson [:longsonr]]

The matrix isn't singular. The CharacterIterator is in error before that as there's no text to work with. The problem markup is this...

<text>               <-- whitespace here 
  <tspan x= y=/>
</tspan>

The whitespace compresses to nothing but then we still try to measure the empty glyph frame giving us 0, 0, 0, 0 which then contributes to the bounding box.

The CharacterIterator class spots that there is no text and sets mInError but then there's no way to pick that up. We don't check the matrix either so this patch kills two birds with one stone.

Comment 13

[Jonathan Watt [:jwatt]]

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

I see. Thanks for the explanation.

Comment 14

[Robert Longson [:longsonr]]

Attachment: missed a gfx->restore() call in one spot

Comment 15

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0a0c727388

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7c0a0c727388

Comment 17

[Robert Longson [:longsonr]]

Martin,

The SVG team could really do with some help catching problems like this _before_ they reach the release builds. We don't seem to have enough people testing SVG before that stage right now. The Beta builds are very stable (get virtually no changes before release), so if you'd be willing to use Beta or Aurora builds for your day-to-day browsing and report any regressions that you notice, that would be a great help. If you're interested, you can find Beta/Aurora builds here:

http://www.mozilla.org/en-US/firefox/channel/

Comment 18

[Robert Longson [:longsonr]]

Comment on attachment 646104 [details] [diff] [review]
missed a gfx->restore() call in one spot

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 647914
User impact if declined: Wrong bounding boxes for text elements that have children with whitespace between the children. Can make selection difficult.
Testing completed (on m-c, etc.): landed on m-c with test
Risk to taking this patch (and alternatives if risky): Low risk, no realistic alternatives other than not taking the patch though
String or UUID changes made by this patch: none

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Because this is already broken since 13 and we're not getting a ton of complaints I'd like to have a better sense of how widespread this issue is and what kind of user pain it might be causing, I'm inclined to let it ride the trains without a persuasive case here.

Comment 20

[Martin Hejral]

(In reply to Lukas Blakk [:lsblakk] from comment #19)
> Because this is already broken since 13 and we're not getting a ton of
> complaints I'd like to have a better sense of how widespread this issue is
> and what kind of user pain it might be causing

Lukas,

we develop web app for corporate environment which works online with huge documents converted from AutoCAD and we need to interactively work with complex vector structures like circuit diagrams etc. 
 
It is essential to have possibility to compute sizes of objects in different Coordinate Systems, for example if you need to add some new objects to the complex SVG structure...

This is exactly the case for which SVG is intended!

The fact, that there are almost no such web application at this time — this fact is no reason to block development of such apps ;-) 

Also there is big potential for such applications in various large organizations...

Comment 21

[Robert Longson [:longsonr]]

Have you checked out a nightly to confirm that you're happy this issue is fixed there Martin?

Comment 22

[Martin Hejral]

(In reply to Robert Longson from comment #21)
> Have you checked out a nightly to confirm that you're happy this issue is fixed there Martin?

Nice work Robert: My tests are OK

FF13 OK
FF14 broken
...
FF17.0a1 (2012-08-09) OK

Many thanks to everybody for great work. 

to comment #17: we will try to use beta more offen...

Robert, please one more stupid question :) This fix will appear in FF17? Is this how Mozilla development works?

Comment 23

[Robert Longson [:longsonr]]

By default, yes. If approved for aurora it jumps up one step to 16 and if approved for beta it would jump 2 steps to 15.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 646104 [details] [diff] [review]
missed a gfx->restore() call in one spot

Approving for branches since this only touches svg code, fixes a regression, and we can easily verify on 15/16 that this is resolved.  Please report back with your results once Beta 5 ships next Friday (Aug 18)

Comment 25

[Robert Longson [:longsonr]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/719ca729259e

Comment 26

[Robert Longson [:longsonr]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3ee4b5b63f68

Comment 27

[Paul Silaghi, QA [:pauly]]

(In reply to Martin Hejral from comment #0)
> Expected results:
> 
> result should be BBox of <text_tspan>:   22, -91, 66, 91

Actual results loading the test case on Nightly 17.0a1 (2012-08-14):
BBox of <text_tspan>: 21.875, -90.75, 66.25, 90.375

Is this correct? And what about the second pop-up with BBox of <text_only>? It is not important?

Comment 28

[Robert Longson [:longsonr]]

You'll get slightly different results on different platforms which is OK. If the numbers are wildly out e.g. 0, -90.75, 88, 90.75 then that's bad. 

I don't think the second dialogue is relevant, that case works.

Comment 29

[Martin Hejral]

(In reply to Paul Silaghi [QA] from comment #27)
> (In reply to Martin Hejral from comment #0)
> > Expected results:
> > 
> > result should be BBox of <text_tspan>:   22, -91, 66, 91
> 
> Actual results loading the test case on Nightly 17.0a1 (2012-08-14):
> BBox of <text_tspan>: 21.875, -90.75, 66.25, 90.375
> 
> Is this correct? 

YES, correct. By my opinion this effect is caused by different engines for font computing/rendering (fonts usually have complex vector definitions...)

> And what about the second pop-up with BBox of <text_only>?
> It is not important?

The second pop-up was added in testcase to demonstrate fact that bug will appear only when <text> contains <tspan> elements... not for simple <text> like this:

<text id='text_only' font-size='2.80' x='22' y='-88' fill='yellow'>
[22,-88]
</text>

Comment 30

[Paul Silaghi, QA [:pauly]]

Actual results loading the test case on FF 15b5:
on Mac OS X 10.6: BBox of <text_tspan>: 21.875, -90.75, 66.25, 90.375
on Win 7/64: BBox of <text_tspan>:   22, -90.75, 66, 90.375

Verified fixed based on comment 28 and comment 29.
Able to see the issue on FF 15b4.