1177726 - Stroke text with shadow on canvas is cut off

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[smaren]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150511103818

Steps to reproduce:

1: apply shadow with a small shadowBlur (http://jsfiddle.net/nnqc8/344/)
2: call stroke text



Actual results:

The text is cut off on all sides.


Expected results:

The stroked text should look as if there was no shadow.

http://jsfiddle.net/nnqc8/345/ - identical setup but without shadow color set.

Comment 1

[Alice0775 White]

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=07f04ff0b782&tochange=71d1333ca77f

Regressed by:71d1333ca77f	Anthony Jones — Bug 591358 - Part 3: Lazy creation of the draw target in order to save memory and improve performance. r=roc

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

If it was regressed by that, it's too long ago, and sort of incidental.  Seems like we just don't account for the shadow in CanvasRenderingContext2D::DrawOrMeasureText; either nsBidiPresUtils::ProcessText needs to know about it, or we need to adjust the values it gives us back.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

I take that back; increasing the bounding box returned by nsBidiPresUtils::ProcessText does make the problem go away, but I don't think that's the actual problem.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Inflate the bounds based on stroke options before creating adjusted target. r=jmuizelaar

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Add a reftest.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Inflate the bounds based on stroke options before creating adjusted target. r=jmuizelaar

Added a reftest.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8630635 [details] [diff] [review]
Inflate the bounds based on stroke options before creating adjusted target.  r=jmuizelaar

Review of attachment 8630635 [details] [diff] [review]:
-----------------------------------------------------------------

Call the test 1177726-text-stroke-bounds.html or something like that which gives an indication of what it's testing.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> ...
> Call the test 1177726-text-stroke-bounds.html or something like that which
> gives an indication of what it's testing.

All the other tests are just bug numbers, which is why I named these the way I did.  Hmm.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > ...
> > Call the test 1177726-text-stroke-bounds.html or something like that which
> > gives an indication of what it's testing.
> 
> All the other tests are just bug numbers, which is why I named these the way
> I did.  Hmm.

Yeah, that doens't make it a good practice. dom/canvas/test/reftest is also probably a better place to put it.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Already had try running with the old code, will rename/move the reftest and land

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3def327b3bc0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8771c4935ca

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Inflate the bounds based on stroke options before creating adjusted target. Carry r=jmuizelaar

Moved the test to dom/canvas instead.

Comment 12

[Pulsebot]

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

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11512920&repo=mozilla-inbound

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d17503902bd

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Good thing we had a test; this change didn't actually break anything, but the test failure is showing that I didn't fix the bug on all platforms.  +1 for automated tests and Jeff reminding me to put one in.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Android seemed to have "half" of this bug to start - the left and bottom were "clipped", but the top...

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Inflate the bounds based on stroke options before creating adjusted target. Carry r=jmuizelaar

I made an assumption about the test failure based on the Android results being different before the fix - looking at the actual result of the failed test, it turns out it's just aliasing errors away from the borders, likely the white shadow, so, the fix actually works.  Fuzz those few pixels away.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73ecd58fa2df

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Yup, the test is passing now.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9be6d98cdc

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dd9be6d98cdc