Closed Bug 777476 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: SVG, defect)

14 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox15 + verified
firefox16 + fixed

People

(Reporter: martin.hejral, Assigned: longsonr)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image 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 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>
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!!
Would you be willing to find a regression range?
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Pardon, right now, after additional test on Windows XP, it seems that FF13 was still OK, but FF14 already fails...
(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...
Around February/March this year then.
(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
Sorry, I mixed "Before" and "After", just switch them.
Assignee: nobody → longsonr
Version: 13 Branch → 14 Branch
Attached patch patch (obsolete) — Splinter Review
Attachment #646083 - Flags: review?(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.
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 on attachment 646083 [details] [diff] [review]
patch

I see. Thanks for the explanation.
Attachment #646083 - Flags: review?(jwatt) → review+
Attachment #646083 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0a0c727388
Flags: in-testsuite+
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/7c0a0c727388
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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 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
Attachment #646104 - Flags: approval-mozilla-beta?
Attachment #646104 - Flags: approval-mozilla-aurora?
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.
(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...
Have you checked out a nightly to confirm that you're happy this issue is fixed there Martin?
(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?
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 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)
Attachment #646104 - Flags: approval-mozilla-beta?
Attachment #646104 - Flags: approval-mozilla-beta+
Attachment #646104 - Flags: approval-mozilla-aurora?
Attachment #646104 - Flags: approval-mozilla-aurora+
(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?
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.
(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>
Keywords: verifyme
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.
Keywords: verifyme
QA Contact: paul.silaghi
You need to log in before you can comment on or make changes to this bug.