Closed Bug 463934 Opened 11 years ago Closed 11 years ago

text bounds do not account for position properly

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: longsonr, Assigned: longsonr)

References

()

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 5 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Flags: blocking1.9.1?
Attachment #347188 - Flags: superreview?(roc)
Attachment #347188 - Flags: review?(roc)
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?
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.
(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?
I seem to remember tor saying something about stroke-widths not being treated correctly in that case but I can't find the reference.
Ah, you're absolutely right. Grrr.
Attachment #347188 - Flags: superreview?(roc)
Attachment #347188 - Flags: superreview+
Attachment #347188 - Flags: review?(roc)
Attachment #347188 - Flags: review+
Would be nice to have a test for this bug and bug 460946.
Attached patch updated patch with tests (obsolete) — Splinter Review
Looks like tests were a good idea as there were some flaws with the original patch.
Attachment #347188 - Attachment is obsolete: true
Attachment #348476 - Flags: superreview?(roc)
Attachment #348476 - Flags: review?(roc)
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.
Flags: blocking1.9.1? → wanted1.9.1+
Duplicate of this bug: 465275
Attachment #348476 - Attachment is obsolete: true
Attachment #348522 - Flags: superreview?(roc)
Attachment #348522 - Flags: review?(roc)
Attachment #348476 - Flags: superreview?(roc)
Attachment #348476 - Flags: review?(roc)
Attachment #348522 - Flags: superreview?(roc)
Attachment #348522 - Flags: superreview+
Attachment #348522 - Flags: review?(roc)
Attachment #348522 - Flags: review+
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.
Attachment #348522 - Flags: approval1.9.1?
Attachment #348522 - Flags: approval1.9.1? → approval1.9.1b2?
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.
Attachment #348522 - Flags: approval1.9.1b2?
Attachment #348522 - Flags: approval1.9.1b2-
Attachment #348522 - Flags: approval1.9.1?
Attachment #348522 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 348522 [details] [diff] [review]
address review comment
[Checkin: Comment 15+18]

a191=beltzner
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
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
If you disable the individual failing isRounded tests I'll fix them once the tree reopens. Looks like some rounding differences on different platforms.
I'm going to hold off on doing anything until I see perf numbers.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 465222
Attached patch patch (obsolete) — Splinter Review
fix up mochitests to allow for rounding differences
Duplicate of this bug: 468803
Blocks: 468803
Attachment #351826 - Attachment is obsolete: true
Attachment #352433 - Flags: review?(roc)
Keywords: checkin-needed
Whiteboard: [needs-landing] updated patch only
Duplicate of this bug: 469327
Attachment #348522 - Attachment description: address review comment → address review comment [Checkin: Comment 15+18]
Comment on attachment 352433 [details] [diff] [review]
updated patch
[Backout: Comment 24]

http://hg.mozilla.org/mozilla-central/rev/e7aa9d60b511
Attachment #352433 - Attachment description: updated patch → updated patch [Checkin: Comment 23]
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs-landing] updated patch only → [c-n: 'updated patch' baking for 1.9.1]
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [c-n: 'updated patch' baking for 1.9.1]
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
}
Attachment #352433 - Attachment description: updated patch [Checkin: Comment 23] → updated patch [Backout: Comment 24]
(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
}
Attached patch new attempt (obsolete) — Splinter Review
Attachment #352433 - Attachment is obsolete: true
Any chance that someone with a Mac can test Robert's latest reftest attempt on that platform?
Uh, mochitest, not reftest.
Not just a Mac, linux too. I only have Windows.
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
Can you check this one out please?
Attachment #352941 - Attachment is obsolete: true
(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
Duplicate of this bug: 471337
Please file a new bug for the tests? Keeping a bug open just for tests is horribly confusing.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 479656
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.
(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.
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.