Stroke text with shadow on canvas is cut off

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: smaren, Assigned: milan)

Tracking

({regression, reproducible})

18 Branch
mozilla42
Unspecified
All
regression, reproducible
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Component: Untriaged → Canvas: 2D
Product: Firefox → Core

Comment 1

3 years ago
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
Blocks: 591358
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, reproducible
Version: 38 Branch → 18 Branch
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.
OS: Unspecified → All
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.
Assignee: nobody → milan
Created attachment 8630592 [details] [diff] [review]
Inflate the bounds based on stroke options before creating adjusted target.  r=jmuizelaar
Attachment #8630592 - Flags: review?(jmuizelaar)
Add a reftest.
Created attachment 8630635 [details] [diff] [review]
Inflate the bounds based on stroke options before creating adjusted target.  r=jmuizelaar

Added a reftest.
Attachment #8630592 - Attachment is obsolete: true
Attachment #8630592 - Flags: review?(jmuizelaar)
Attachment #8630635 - Flags: review?(jmuizelaar)
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.
Attachment #8630635 - Flags: review?(jmuizelaar) → review+
(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.
(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.
Created attachment 8631053 [details] [diff] [review]
Inflate the bounds based on stroke options before creating adjusted target.  Carry r=jmuizelaar

Moved the test to dom/canvas instead.
Attachment #8630635 - Attachment is obsolete: true
Attachment #8631053 - Flags: review+
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11512920&repo=mozilla-inbound
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan)
Android seemed to have "half" of this bug to start - the left and bottom were "clipped", but the top...
Created attachment 8632149 [details] [diff] [review]
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
Attachment #8631053 - Attachment is obsolete: true
Attachment #8632149 - Flags: review+
Yup, the test is passing now.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dd9be6d98cdc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.