463934 - text bounds do not account for position properly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

Need to ensure that no global matrix is applied when calculating the drawing size of the bounds and also that the x and y positions of the start of the glyph fragment are included correctly.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wouldn't nsSVGGlyphFrame::UpdateCoveredRegion work just as well (or even better) if instead of SetMatrixPropagation(PR_FALSE), we just removed the following code that does GetGlobalTransform/TransformBounds?

Comment 2

[Robert Longson [:longsonr]]

You would be back to the rotation problem I just fixed in bug 460946 unless I'm missing something from your suggestion.

If the global transform has a rotation then cairo returns the bounding box transformed to the user co-ordinate system. So you get a box round a roatated square. We then have to transform it again to get to the device co-ordinate system and the box becomes too big (a box round a rotated square which is itself round a rotated square).

To work out the covered region you have to determine the bounds of the untransformed object and then transform the bounds afterwards. Unfortunately 
nsSVGGlyphFrame::EnsureTextRun has a GetGlobalTransform embedded in it and I need to make that return an identity matrix.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #2)
> If the global transform has a rotation then cairo returns the bounding box
> transformed to the user co-ordinate system. So you get a box round a roatated
> square. We then have to transform it again to get to the device co-ordinate
> system and the box becomes too big (a box round a rotated square which is
> itself round a rotated square).

I think we could just go ahead and construct the path with the regular global transform, but set the tmpCtx CTM to the identity matrix before we extract the extents. Right?

Comment 4

[Robert Longson [:longsonr]]

I seem to remember tor saying something about stroke-widths not being treated correctly in that case but I can't find the reference.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, you're absolutely right. Grrr.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

So be it.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Would be nice to have a test for this bug and bug 460946.

Comment 8

[Robert Longson [:longsonr]]

Attachment: updated patch with tests

Looks like tests were a good idea as there were some flaws with the original patch.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Your tests are too strict. There's no reason why getBoundingClientRect has to return integers for coordinates. Maybe you should create a helper isRounded(a, b, message) which rounds both values before comparing them.

Comment 11

[Robert Longson [:longsonr]]

Attachment: address review comment [Checkin: Comment 15+18]

Comment 12

[Robert Longson [:longsonr]]

Comment on attachment 348522 [details] [diff] [review]
address review comment
[Checkin: Comment 15+18]

This is fairly noticeable when SVG text is scrolled into view (bug 465275). I would expect it to fix bug 465222 too. It includes tests for all of the code so should be low risk.

Comment 13

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

Comment on attachment 348522 [details] [diff] [review]
address review comment
[Checkin: Comment 15+18]

We're closing down for beta 2 at the moment, so we'll have to take this after branch. Find me on IRC if you disagree vehemently.

Comment 14

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

Comment on attachment 348522 [details] [diff] [review]
address review comment
[Checkin: Comment 15+18]

a191=beltzner

Comment 15

[Shawn Wilsher :sdwilsh]

I landed this to see if it fixes bug 465222.  If it doesn't, I'm going to back it out.

http://hg.mozilla.org/mozilla-central/rev/a8f64c6df6b8

Comment 16

[Robert Longson [:longsonr]]

If you disable the individual failing isRounded tests I'll fix them once the tree reopens. Looks like some rounding differences on different platforms.

Comment 17

[Shawn Wilsher :sdwilsh]

I'm going to hold off on doing anything until I see perf numbers.

Comment 18

[Shawn Wilsher :sdwilsh]

I'm reopening this bug to track the fixes for the test, but the code stayed in.
http://hg.mozilla.org/mozilla-central/rev/1648004a4b57

Comment 19

[Robert Longson [:longsonr]]

Attachment: patch

fix up mochitests to allow for rounding differences

Comment 21

[Robert Longson [:longsonr]]

Attachment: updated patch [Backout: Comment 24]

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 352433 [details] [diff] [review]
updated patch
[Backout: Comment 24]

http://hg.mozilla.org/mozilla-central/rev/e7aa9d60b511

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 352433 [details] [diff] [review]
updated patch
[Backout: Comment 24]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/5fb12dc5d98a
http://hg.mozilla.org/mozilla-central/rev/670cb792e94a

See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229185598.1229188228.1583.gz
Linux mozilla-central moz2-linux-slave07 dep unit test on 2008/12/13 08:26:38
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229185598.1229188278.1706.gz
Linux mozilla-central moz2-linux-slave08 dep unit test on 2008/12/13 08:26:38

*** 28124 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text1.getBoundingClientRect().top is 8 should be approximately 6
*** 28126 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text1.getBoundingClientRect().height is 21 should be approximately 24
*** 28128 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text2.getBoundingClientRect().top is 8 should be approximately 6
*** 28130 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text2.getBoundingClientRect().height is 21 should be approximately 24
*** 28133 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text3.getBoundingClientRect().width is 41 should be approximately 44
*** 28134 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text3.getBoundingClientRect().height is 41 should be approximately 43
*** 28136 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text4.getBoundingClientRect().top is 128 should be approximately 123
*** 28137 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text4.getBoundingClientRect().width is 81 should be approximately 86
*** 28138 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text4.getBoundingClientRect().height is 81 should be approximately 85
*** 28172 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text1a.getBoundingClientRect().top is 8 should be approximately 6
*** 28174 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text1a.getBoundingClientRect().height is 22 should be approximately 24
*** 28176 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text2a.getBoundingClientRect().top is 8 should be approximately 6
*** 28178 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text2a.getBoundingClientRect().height is 25 should be approximately 28
}

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)

MacOSX failed one case only:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229185598.1229189936.4919.gz
MacOSX Darwin 9.2.2 mozilla-central moz2-darwin8-slave01 dep unit test on 2008/12/13 08:26:38

*** 28175 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bounds.html | text2a.getBoundingClientRect().height is 25 should be approximately 28
}

Windows succeeded:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229185598.1229188820.2822.gz
WINNT 5.2 mozilla-central moz2-win32-slave07 dep unit test on 2008/12/13 08:26:38
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229185598.1229188930.3081.gz
WINNT 5.2 mozilla-central moz2-win32-slave08 dep unit test on 2008/12/13 08:26:38
}

Comment 26

[Robert Longson [:longsonr]]

Attachment: new attempt

Comment 27

[Jonathan Watt [:jwatt]]

Any chance that someone with a Mac can test Robert's latest reftest attempt on that platform?

Comment 28

[Jonathan Watt [:jwatt]]

Uh, mochitest, not reftest.

Comment 29

[Robert Longson [:longsonr]]

Not just a Mac, linux too. I only have Windows.

Comment 30

[Martijn Wargers (dead)]

Sorry for the delay. I just tested the mochitest on the Mac, I get these 2 failures:
Passed: 54
Failed: 2
Todo: 0

not ok - text3.getBoundingClientRect().width is 42 should be approximately 40
not ok - text2a.getBoundingClientRect().height is 28 should be approximately 25

Comment 31

[Robert Longson [:longsonr]]

Attachment: Yet another attempt

Can you check this one out please?

Comment 32

[Martijn Wargers (dead)]

(In reply to comment #31)
> Created an attachment (id=353520) [details]
> Yet another attempt
> 
> Can you check this one out please?

With that one, I get 3 failures:
not ok - text3.getBoundingClientRect().width is 42 should be approximately 40
not ok - text4.getBoundingClientRect().top is 128 should be approximately 130
not ok - text2a.getBoundingClientRect().height is 28 should be approximately 23

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Please file a new bug for the tests? Keeping a bug open just for tests is horribly confusing.

Comment 36

[developer]

Attachment: xhtml file demonstrating paint problem with SVG text

This xhtml file demonstrates a paint problem with SVG text on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7 (.NET CLR 3.5.30729).

Click on red square to hide text and junk is left behind. 

Sometimes the text is removed correctly but then the filter effect gets clipped, though this behaviour is unpredictable. The former happens regularly.

Comment 37

[Robert Longson [:longsonr]]

(In reply to comment #36)

The Firefox 3.0.x series only takes critical security fixes and SVG rendering glitches don't count I'm afraid. But the good news is that Firefox 3.5 is just around the corner so...

Please try a trunk nightly or a Firefox 3.5 nightly or even a Firefox 3.5 Beta, and if you have problems with those raise a NEW bug rather than commenting further in this one.