1304539 - Canvas fillText / strokeText do not respect canvas filters

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P3)

Description

[Markus Stange [:mstange]]

[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

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 2

[Jonathan Kew [:jfkthame]]

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?

Comment 3

[Markus Stange [:mstange]]

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.

Comment 4

[Jonathan Kew [:jfkthame]]

(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.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: use AdjustedDrawTarget

I pass AdjustedDrawTarget to gfxTextRun and the text is green, though I'm not sure if there is any side effect.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

[Tracking Requested - why for this release]:

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(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...

Comment 8

[Markus Stange [:mstange]]

(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.

Comment 9

[Marcia Knous [:marcia]]

Tracking 52+ since this is a regression in a new feature that we just shipped in 49 (canvas filters).

Comment 10

[Ethan Lin[:ethlin]]

(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.

Comment 11

[Ethan Lin[:ethlin]]

(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.

Comment 12

[Ethan Lin[:ethlin]]

Attachment: 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.

Comment 13

[Ethan Lin[:ethlin]]

Attachment: 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.

Comment 14

[Markus Stange [:mstange]]

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.

Comment 15

[Ethan Lin[:ethlin]]

(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)'.

Comment 16

[Ethan Lin[:ethlin]]

Attachment: Add reftest

Upload my patch first. I'll see if there's other way to reduce the fuzzy number of strokeText. Markus, any idea?

Comment 17

[Ethan Lin[:ethlin]]

Attachment: 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.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8795065 [details] [diff] [review]
Add reftest

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

That's a lot better!

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked 50+ as it's a new regression since Fx49

Comment 20

[Ethan Lin[:ethlin]]

Attachment: 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

Comment 21

[Ethan Lin[:ethlin]]

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.

Comment 22

[Ethan Lin[:ethlin]]

Attachment: use AdjustedDrawTarget

Create patch for FF release.

Comment 23

[Ethan Lin[:ethlin]]

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.

Comment 24

[Jonathan Kew [:jfkthame]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

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

I had to back this out for Windows reftest failures https://treeherder.mozilla.org/logviewer.html#?job_id=36733785&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/37580f7e092f

Comment 27

[Ethan Lin[:ethlin]]

In win8, the fuzz number is different. I'll update the the patch.

Comment 28

[Ethan Lin[:ethlin]]

Attachment: Add reftest (r=mstange)

Fix win8 try server failure.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ac1c5fdaef5
https://hg.mozilla.org/mozilla-central/rev/1422dee77e60

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/45c3cb513380
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c06ade5c9d3

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4103b6b482e3
https://hg.mozilla.org/releases/mozilla-beta/rev/2c8342768f77

Comment 38

[Kohei Yoshino]

[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.

Comment 39

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/specific-canvas-text-effects-are-not-rendered-properly/

Comment 40

[Markus Stange [:mstange]]

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.

Comment 41

[Kohei Yoshino]

Liz: please reconsider uplifting the release patch.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8796106 [details] [diff] [review]
use AdjustedDrawTarget

Fix a regression, taking this as a ridealong for 49.0.2.

Comment 44

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

https://hg.mozilla.org/releases/mozilla-release/rev/2c8f1510d293b272d4a2638b3c55a77beea2f66d

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

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)]: