Canvas fillText / strokeText do not respect canvas filters

RESOLVED FIXED in Firefox 49

Status

()

Core
Canvas: 2D
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: mstange, Assigned: ethlin)

Tracking

({dev-doc-complete, regression, site-compat})

49 Branch
mozilla52
dev-doc-complete, regression, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49+ fixed, relnote-firefox 49+, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

a year ago
[Tracking Requested - why for this release]: regression in a new feature that we just shipped in 49 (canvas filters)

In the testcase, the text should be green, but it's red because the canvas filter is being ignored.

Bug 1274936 broke this for fillText (which shipped in 49) and then bug 1275693 also broke it for strokeText (which will ship in 50).

This breaks the interactive example that somebody added to the main documentation for canvas filters at https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/filter
(Reporter)

Updated

a year ago
status-firefox49: unaffected → affected
(Reporter)

Comment 1

a year ago
Created attachment 8793531 [details]
testcase
Has Regression Range: --- → yes
Has STR: --- → yes
Priority: -- → P3
Whiteboard: [gfx-noted]
Version: Trunk → 49 Branch
I guess the new text-painting code path that goes through gfxTextRun::Draw etc doesn't know about the AdjustedTarget stuff used for filter support. :(  Are you aiming to look into this, Markus, or do we need to find someone else to tackle it?
(Reporter)

Comment 3

a year ago
I was hoping that your patch for bug 1304353 might magically fix this, but it does not sound like it will.
If it does not respect AdjustedTarget, does that mean that canvas shadows are also broken for fillText/strokeText?
I wasn't really planning to look into this, but if you don't want to work on it I can take a look.
(In reply to Markus Stange [:mstange] from comment #3)
> I was hoping that your patch for bug 1304353 might magically fix this, but
> it does not sound like it will.

I tried your testcase here with that patch, but it remains red. So no magic fix this time, sadly.

> If it does not respect AdjustedTarget, does that mean that canvas shadows
> are also broken for fillText/strokeText?

Looks like it. (Hmm, we didn't have tests for that?)

> I wasn't really planning to look into this, but if you don't want to work on
> it I can take a look.

If you could, that would be awesome.... I have a couple other issues that I need to focus on at the moment, so probably wouldn't get to this for a week or two at least.
(Assignee)

Comment 5

a year ago
Created attachment 8794124 [details] [diff] [review]
use AdjustedDrawTarget

I pass AdjustedDrawTarget to gfxTextRun and the text is green, though I'm not sure if there is any side effect.
Attachment #8794124 - Flags: feedback?(mstange)
[Tracking Requested - why for this release]:
tracking-firefox49: --- → ?
(In reply to David Bolter [:davidb] from comment #6)
> [Tracking Requested - why for this release]:

Oops. The 'why' is so Liz sees this and possibly adds to the growing ridealong list for a possible point release. Not sure I'd advocate for that though...
(Reporter)

Comment 8

a year ago
(In reply to Ethan Lin[:ethlin] from comment #5)
> Created attachment 8794124 [details] [diff] [review]
> use AdjustedDrawTarget
> 
> I pass AdjustedDrawTarget to gfxTextRun and the text is green, though I'm
> not sure if there is any side effect.

This looks good. Does it cause us to apply globalAlpha twice, when combined with the fix for bug 1304353? If not, then this should be fine.
We also need a bunch of reftests. For example:
== fillText-with-color-rgb(0,32,0)-and-filter:brightness(4) fillText-with-color-rgb(0,128,0)-and-no-filter
== same for strokeText
!= fillText-with-shadow fillText-without-shadow
!= same for strokeText
And the above with globalAlpha 0.5.
Tracking 52+ since this is a regression in a new feature that we just shipped in 49 (canvas filters).
tracking-firefox52: ? → +
(Assignee)

Comment 10

a year ago
(In reply to Markus Stange [:mstange] from comment #8)
> (In reply to Ethan Lin[:ethlin] from comment #5)
> > Created attachment 8794124 [details] [diff] [review]
> > use AdjustedDrawTarget
> > 
> > I pass AdjustedDrawTarget to gfxTextRun and the text is green, though I'm
> > not sure if there is any side effect.
> 
> This looks good. Does it cause us to apply globalAlpha twice, when combined
> with the fix for bug 1304353? If not, then this should be fine.
> We also need a bunch of reftests. For example:
> == fillText-with-color-rgb(0,32,0)-and-filter:brightness(4)
> fillText-with-color-rgb(0,128,0)-and-no-filter
> == same for strokeText
> != fillText-with-shadow fillText-without-shadow
> != same for strokeText
> And the above with globalAlpha 0.5.

Okay, I will check with the patch in bug 1304353 and add reftests.
Assignee: nobody → ethlin
(Assignee)

Comment 11

a year ago
(In reply to Ethan Lin[:ethlin] from comment #10)
> (In reply to Markus Stange [:mstange] from comment #8)
> > This looks good. Does it cause us to apply globalAlpha twice, when combined
> > with the fix for bug 1304353? If not, then this should be fine.
> > We also need a bunch of reftests. For example:
> > == fillText-with-color-rgb(0,32,0)-and-filter:brightness(4)
> > fillText-with-color-rgb(0,128,0)-and-no-filter
> > == same for strokeText
> > != fillText-with-shadow fillText-without-shadow
> > != same for strokeText
> > And the above with globalAlpha 0.5.
> 
> Okay, I will check with the patch in bug 1304353 and add reftests.

After testing, it doesn't apply globalAlpha twice with the fix in bug 1304353. So I'll add more reftests.
(Assignee)

Comment 12

a year ago
Created attachment 8794451 [details] [diff] [review]
use AdjustedDrawTarget

Use AdjustedDrawTarget for drawing text. The previous patch caused a failure in  "dom/canvas/test/reftest/1177726-text-stroke-bounds.html".
The try result looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7b0b8e61ede

I'll add reftests in another patch, but I may need some time to prepare.
Attachment #8794124 - Attachment is obsolete: true
Attachment #8794124 - Flags: feedback?(mstange)
Attachment #8794451 - Flags: review?(mstange)
(Assignee)

Comment 13

a year ago
Created attachment 8794723 [details] [diff] [review]
Add reftest

Add reftest for fillText and strokeText. I need to add fuzzy for these tests. It looks like after filter, the stroke of text is slightly different.
Attachment #8794723 - Flags: review?(mstange)
(Reporter)

Updated

a year ago
Attachment #8794451 - Flags: review?(mstange) → review+
(Reporter)

Comment 14

a year ago
Comment on attachment 8794723 [details] [diff] [review]
Add reftest

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

::: dom/canvas/test/reftest/filters/reftest.list
@@ +17,5 @@
>  == units.html ref.html
>  == units-em.html ref.html
>  == units-ex.html ref.html
>  == units-off-screen.html ref.html
> +fuzzy(47,633) == fillText-with-filter-brightness.html fillText-with-filter-brightness-ref.html

That's a lot of fuzz. I tested a bit and found that with the opacity() filter, you don't need as much fuzz. Can you remove the hue-rotate and the brightness tests, and replace them with tests that use ctx.filter = "opacity(0.5)"? And instead of using globalAlpha in all tests, I think we should have two versions of all of them, with and without globalAlpha.
Attachment #8794723 - Flags: review?(mstange)
(Assignee)

Comment 15

a year ago
(In reply to Markus Stange [:mstange] from comment #14)
> Comment on attachment 8794723 [details] [diff] [review]
> Add reftest
> 
> Review of attachment 8794723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/test/reftest/filters/reftest.list
> @@ +17,5 @@
> >  == units.html ref.html
> >  == units-em.html ref.html
> >  == units-ex.html ref.html
> >  == units-off-screen.html ref.html
> > +fuzzy(47,633) == fillText-with-filter-brightness.html fillText-with-filter-brightness-ref.html
> 
> That's a lot of fuzz. I tested a bit and found that with the opacity()
> filter, you don't need as much fuzz. Can you remove the hue-rotate and the
> brightness tests, and replace them with tests that use ctx.filter =
> "opacity(0.5)"? And instead of using globalAlpha in all tests, I think we
> should have two versions of all of them, with and without globalAlpha.

I just tried opacity() filter, and for 'fillText', the result is much better, but for 'strokeText', I still have to set something like 'fuzzy(45,748)'.
(Assignee)

Comment 16

a year ago
Created attachment 8795057 [details] [diff] [review]
Add reftest

Upload my patch first. I'll see if there's other way to reduce the fuzzy number of strokeText. Markus, any idea?
Attachment #8794723 - Attachment is obsolete: true
Flags: needinfo?(mstange)
(Assignee)

Comment 17

a year ago
Created attachment 8795065 [details] [diff] [review]
Add reftest

I tried grayscale() filter and the result seems better. So I add grayscale test for fillText and strokeText. The fuzzy number may change after I send the patch try server.
Attachment #8795057 - Attachment is obsolete: true
Flags: needinfo?(mstange)
Attachment #8795065 - Flags: review?(mstange)
(Reporter)

Comment 18

a year ago
Comment on attachment 8795065 [details] [diff] [review]
Add reftest

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

That's a lot better!
Attachment #8795065 - Flags: review?(mstange) → review+
Tracked 50+ as it's a new regression since Fx49
tracking-firefox50: ? → +
tracking-firefox51: ? → +
Blocks: 1306027
(Assignee)

Comment 20

a year ago
Created attachment 8796004 [details] [diff] [review]
Add reftest (r=mstange)

According to try result, I should change the fuzz of 'strokeText-with-filter-grayscale-1.html' to (1,400).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20401f6a44d2
Attachment #8795065 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Attachment #8796004 - Flags: review+
(Assignee)

Comment 21

a year ago
Comment on attachment 8794451 [details] [diff] [review]
use AdjustedDrawTarget

Approval Request Comment
[Feature/regressing bug #]: Bug 1274936
[User impact if declined]: The filter and shadow of canvas doesn't work for fillText and strokeText.
[Describe test coverage new/current, TreeHerder]: Tested on locally and try server.
[Risks and why]: Low. Just use the original way to render.
[String/UUID change made/needed]: None.
Attachment #8794451 - Flags: approval-mozilla-beta?
Attachment #8794451 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

a year ago
Created attachment 8796106 [details] [diff] [review]
use AdjustedDrawTarget

Create patch for FF release.
(Assignee)

Comment 23

a year ago
Comment on attachment 8796106 [details] [diff] [review]
use AdjustedDrawTarget

Approval Request Comment
[Feature/regressing bug #]: Bug 1274936
[User impact if declined]: The filter and shadow of canvas doesn't work for fillText and strokeText.
[Describe test coverage new/current, TreeHerder]: Tested on locally and try server.
[Risks and why]: Low. Just use the original way to render.
[String/UUID change made/needed]: None.
Attachment #8796106 - Flags: approval-mozilla-release?
(In reply to Ethan Lin[:ethlin] from comment #22)
> Created attachment 8796106 [details] [diff] [review]
> use AdjustedDrawTarget
> 
> Create patch for FF release.

Are we considering this for a FF49.dot release? If so, we should probably reconsider taking bug 1304353 (currently wontfix'd for 49) as well, given how closely related they are.

Comment 25

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f205f28cbcf3
Use AdjustedDrawTarget to draw text. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfe71842bb7
Add reftest for fillText and strokeText with filter. r=mstange
Keywords: checkin-needed
(Assignee)

Comment 27

a year ago
In win8, the fuzz number is different. I'll update the the patch.
Flags: needinfo?(ethlin)
(Assignee)

Comment 28

a year ago
Created attachment 8796382 [details] [diff] [review]
Add reftest (r=mstange)

Fix win8 try server failure.
Attachment #8796004 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8796382 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 29

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac1c5fdaef5
Use AdjustedDrawTarget to draw text. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/1422dee77e60
Add reftest for fillText and strokeText with filter. r=mstange
Keywords: checkin-needed

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ac1c5fdaef5
https://hg.mozilla.org/mozilla-central/rev/1422dee77e60
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
I think that like bug 1304353, this regression from 49 can wait for a fix in 50. Sounds like a new development feature that most web sites will not yet be using.
status-firefox49: affected → wontfix
tracking-firefox49: ? → -
Comment on attachment 8796106 [details] [diff] [review]
use AdjustedDrawTarget

I don't think this is a dot release driver, and not used widely yet; let's aim to fix this in 50.
Attachment #8796106 - Flags: approval-mozilla-release? → approval-mozilla-release-
Comment on attachment 8794451 [details] [diff] [review]
use AdjustedDrawTarget

As mentioned in comment 0, if this bug breaks an MDN example, it's worth fixing soon, Aurora51+, Beta50+
Attachment #8794451 - Flags: approval-mozilla-beta?
Attachment #8794451 - Flags: approval-mozilla-beta+
Attachment #8794451 - Flags: approval-mozilla-aurora?
Attachment #8794451 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1306027
Duplicate of this bug: 1309254
[Tracking Requested - why for this release]: Bug 1304353 to ride 49.0.2. This can also be fixed given the risk is low, 2 dups reported, and an MDN example is also broken.
status-firefox49: wontfix → affected
tracking-firefox49: - → ?
Keywords: dev-doc-needed
See Also: → bug 1304353
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/specific-canvas-text-effects-are-not-rendered-properly/
Keywords: dev-doc-needed → dev-doc-complete, site-compat
(Reporter)

Comment 40

a year ago
Comment on attachment 8796106 [details] [diff] [review]
use AdjustedDrawTarget

I agree, we should reconsider release uplift here. This not only affects filters (which aren't used widely yet), it also affects canvas shadows, which have been around for a long time.
Liz: please reconsider uplifting the release patch.
Flags: needinfo?(lhenry)
Now that we do have a dot release planned it makes sense to take this for 49.0.2, especially as folks are chiming in to say it's already widely in use.
tracking-firefox49: ? → +
Flags: needinfo?(lhenry)
Comment on attachment 8796106 [details] [diff] [review]
use AdjustedDrawTarget

Fix a regression, taking this as a ridealong for 49.0.2.
Attachment #8796106 - Flags: approval-mozilla-release- → approval-mozilla-release+
Release Note Request (optional, but appreciated)
[Why is this notable]: Ridealong for 49.0.2
[Suggested wording]: Fix a Canvas filters graphics issue affecting HTML5 apps (Bug 1304539)
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → 49+
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.