Last Comment Bug 777476 - SVGLocatable.getBBox() fails with a <text><tspan> nodes
: SVGLocatable.getBBox() fails with a <text><tspan> nodes
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 14 Branch
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Robert Longson
: Paul Silaghi, QA [:pauly]
Mentors:
Depends on:
Blocks: 647914
  Show dependency treegraph
 
Reported: 2012-07-25 12:57 PDT by Martin Hejral
Modified: 2012-08-17 02:10 PDT (History)
7 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed


Attachments
BBox-test.svg (1.33 KB, image/svg+xml)
2012-07-25 12:57 PDT, Martin Hejral
no flags Details
patch (7.57 KB, patch)
2012-07-26 04:22 PDT, Robert Longson
jwatt: review+
Details | Diff | Splinter Review
missed a gfx->restore() call in one spot (7.59 KB, patch)
2012-07-26 05:36 PDT, Robert Longson
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Martin Hejral 2012-07-25 12:57:22 PDT
Created attachment 645860 [details]
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 2012-07-25 13:01:27 PDT
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 2012-07-25 13:10:26 PDT
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 2012-07-25 13:15:37 PDT
Would you be willing to find a regression range?
Comment 4 Robert Longson 2012-07-25 13:29:06 PDT
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 2012-07-25 13:39:55 PDT
Pardon, right now, after additional test on Windows XP, it seems that FF13 was still OK, but FF14 already fails...
Comment 6 Martin Hejral 2012-07-25 13:41:15 PDT
(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 2012-07-25 14:22:40 PDT
Around February/March this year then.
Comment 8 Loic 2012-07-25 15:57:14 PDT
(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 2012-07-25 15:58:52 PDT
Sorry, I mixed "Before" and "After", just switch them.
Comment 10 Robert Longson 2012-07-26 04:22:28 PDT
Created attachment 646083 [details] [diff] [review]
patch
Comment 11 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-07-26 04:30:41 PDT
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 2012-07-26 04:45:33 PDT
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] (back in October - email directly if necessary) 2012-07-26 04:58:37 PDT
Comment on attachment 646083 [details] [diff] [review]
patch

I see. Thanks for the explanation.
Comment 14 Robert Longson 2012-07-26 05:36:27 PDT
Created attachment 646104 [details] [diff] [review]
missed a gfx->restore() call in one spot
Comment 16 Ed Morley [:emorley] 2012-07-27 08:14:02 PDT
https://hg.mozilla.org/mozilla-central/rev/7c0a0c727388
Comment 17 Robert Longson 2012-07-27 09:08:05 PDT
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 2012-08-02 02:56:23 PDT
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 2012-08-03 11:34:40 PDT
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 2012-08-09 12:17:13 PDT
(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 2012-08-09 14:21:27 PDT
Have you checked out a nightly to confirm that you're happy this issue is fixed there Martin?
Comment 22 Martin Hejral 2012-08-09 17:05:56 PDT
(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 2012-08-10 00:18:19 PDT
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 2012-08-10 11:55:46 PDT
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 27 Paul Silaghi, QA [:pauly] 2012-08-14 07:36:15 PDT
(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 2012-08-14 07:43:00 PDT
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 2012-08-15 02:00:09 PDT
(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] 2012-08-17 02:08:16 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.