1304353 - HTML5 Canvas globalAlpha has no effect on fillText in Firefox.49

Status: RESOLVED FIXED

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

Description

[Marton Prorok]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160916101415

Steps to reproduce:

part of index.html file:
<canvas id="myCanvas" width="500" height="140"></canvas>
<script type="text/javascript" src="main.js"></script>

onload function in main.js file:
var myCanvas = document.getElementById("myCanvas");
var ctx = myCanvas.getContext("2d");
ctx.fillStyle = "black";
ctx.fillRect(0, 0, myCanvas.width, myCanvas.height);

ctx.fillStyle = "white";
ctx.font = "50px sans-serif";
ctx.textBaseline = "top";

var x = 10, y = 10, w = 480, h = 14;
ctx.fillRect(x, y, w, h);
ctx.fillText('globalAlpha = ' + ctx.globalAlpha, x, y);

ctx.globalAlpha = 0.5;
y = 80;
ctx.fillRect(10, y, w, h);
ctx.fillText('globalAlpha = ' + ctx.globalAlpha, x, y);

test project with screenshots:
http://www.ultistars.hu/globalalphatest


Actual results:

set globalAlpha to 0.5 has no effect on fillText in Firefox.49


Expected results:

set globalAlpha to 0.5 has the expected effect on fillText in Firefox.48.0.2

Comment 1

[Marton Prorok]

Attachment: index.html

Comment 2

[Marton Prorok]

Attachment: main.js

Comment 3

[Marton Prorok]

Attachment: Firefox_49.png

Comment 4

[Marton Prorok]

Attachment: Firefox_48.0.2.png

Comment 5

[Loic]

Attachment: index.html

Comment 6

[Loic]

Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=15359139459812abed51ce740a819624f1d8de2d&tochange=51b1f2343ad9544921e68859dfbe9d3c79b00951

onathan Kew — Bug 1274936 - When <canvas> fillText is using a simple color, draw via the gfxTextRun::Draw code path to get support for COLR and SVG-in-OT fonts. r=bas

Comment 7

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

Is this something we should consider for uplift to 49 (if we patch it) or can we wontfix for 49?

Comment 8

[Jonathan Kew [:jfkthame]]

I think I see what the problem is, and am testing a patch at the moment. Personally, I don't think the issue is likely to be widespread and serious enough to justify uplifting to 49. But the patch should be safe enough that we can take it for 50 and up, if all goes well.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Set up the DrawOptions appropriately when painting canvas text for FILL, not only for STROKE

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Initialize font's DrawOptions from the textrun's options when painting, so we don't drop any opacity that has already been set

Comment 11

[Jonathan Kew [:jfkthame]]

Looks like this doesn't break any existing tests, at least: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f9d53d7f9b0. We should also add a new reftest that would have caught the regression here.

Comment 12

[Marcia Knous [:marcia]]

Tracking 52+ for this site-compat regression.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - Add reftests for use of globalAlpha with canvas text drawing

Simple reftests for globalAlpha with fillText (fails before the patches here) and strokeText (already passes, but it seemed worth having tests for both APIs).

Comment 14

[Jonathan Kew [:jfkthame]]

Oops, I see the tests have tab characters in them. I'll de-tab them before landing to tidy things up.

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4c4312a8c639feb1be77c32e709625ce58ac1d
Bug 1304353 - part 1 - Set up the DrawOptions appropriately when painting canvas text for FILL, not only for STROKE. r=bas

https://hg.mozilla.org/integration/mozilla-inbound/rev/59a8a58754d58667c5484a2a4b8c889a640a2f89
Bug 1304353 - part 2 - Initialize font's DrawOptions from the textrun's options when painting, so we don't drop any opacity that has already been set. r=bas

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3751c3573a81e751079ed0a2ddbe1a29d0be3e
Bug 1304353 - part 3 - Add reftests for use of globalAlpha with canvas text drawing. r=bas

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4c4312a8c6
part 1 - Set up the DrawOptions appropriately when painting canvas text for FILL, not only for STROKE. r=bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a8a58754d5
part 2 - Initialize font's DrawOptions from the textrun's options when painting, so we don't drop any opacity that has already been set. r=bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3751c3573a
part 3 - Add reftests for use of globalAlpha with canvas text drawing. r=bas

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65566510613321ab3153c5e602b6fb7a42cbc5d1
Bug 1304353 followup, mark test as slightly fuzzy on Windows.

Comment 18

[Gerry Chang [:gchang]]

Track for 51+ as this is a site-compat regression.

Comment 19

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/7c4c4312a8c6
https://hg.mozilla.org/mozilla-central/rev/59a8a58754d5
https://hg.mozilla.org/mozilla-central/rev/3a3751c3573a
https://hg.mozilla.org/mozilla-central/rev/655665106133

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 8793648 [details] [diff] [review]
part 1 - Set up the DrawOptions appropriately when painting canvas text for FILL, not only for STROKE

Approval Request Comment (for parts 1 and 2 together, both are needed to fix the bug)

[Feature/regressing bug #]: 1274936

[User impact if declined]: some <canvas> elements render incorrectly (text is painted with a solid color when it should be transparent)

[Describe test coverage new/current, TreeHerder]: new reftest landed with the fix (patch 3)

[Risks and why]: low risk patch to correctly pass the canvas alpha value to text painting

[String/UUID change made/needed]: none

Comment 22

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

Wontfix for 49, looks likely we can take this in 50 though.

Comment 23

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

Hello Marton, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 24

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

Comment on attachment 8793648 [details] [diff] [review]
part 1 - Set up the DrawOptions appropriately when painting canvas text for FILL, not only for STROKE

Fixes a recent regression, Aurora51+, Beta50+

Comment 25

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

I don't feel the need to track now that it's fixed.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/130a83f1c885
https://hg.mozilla.org/releases/mozilla-aurora/rev/c0c615ba525b

Comment 27

[Carsten Book [:Tomcat]]

landed also the tests in

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/246a46a11eb8
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/816341411190

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/edb415cd33b8
https://hg.mozilla.org/releases/mozilla-beta/rev/5ef5aef99bf1
https://hg.mozilla.org/releases/mozilla-beta/rev/4956e9c8602b
https://hg.mozilla.org/releases/mozilla-beta/rev/835bdb83d005

Comment 30

[Loic]

Jonathan, bug 1305710 is about the same issue but with ctx.globalCompositeOperation = "destination-out" which is fixed with your patch in Nightly.

Do we need to add a test about globalCompositeOperation to track future regressions too?

Comment 31

[Jonathan Kew [:jfkthame]]

Sure, why not. More tests == more fun for everyone. :)

Comment 32

[Jonathan Kew [:jfkthame]]

Attachment: followup, add a reftest using globalCompositeOperation with canvas text painting

Here's an extra test that uses globalCompositeOperation, and fails badly in a build before this bug was fixed. I suspect it'll need a bit of fuzzing for antialiasing around the text, so I've pushed it to tryserver and will add annotations depending on what that reports. https://treeherder.mozilla.org/#/jobs?repo=try&revision=16d4e8f239d9.

Comment 33

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e791148312529e338c6958267406de69d198b66d
Bug 1304353 followup, add a reftest using globalCompositeOperation with canvas text painting. r=bas

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 36

[Kohei Yoshino]

Posted the site compatibility doc given several duplicates. https://www.fxsitecompat.com/en-CA/docs/2016/canvas-2d-globalalpha-and-globalcompositeoperation-have-no-effect-on-filltext/

Comment 38

[Marton Prorok]

Hello Ritu, yes, I can verify that this issue is fixed as expected on a latest Nightly build.
I think this is a major bug, because too many expensive html5 apps and games brokes because of it.
Thanks!

Comment 39

[adrian]

Glad to hear this is fixed but this bug has caused us major issues in our app as we use globalCompositeOperation for efficient glyph caching in webgl (drawing each one into each colour component).  Is it confirmed to be in the next major release? this is affecting out customers.

Thanks

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to adrian from comment #39)
> Glad to hear this is fixed but this bug has caused us major issues in our
> app as we use globalCompositeOperation for efficient glyph caching in webgl
> (drawing each one into each colour component).  Is it confirmed to be in the
> next major release? this is affecting out customers

You can test with the latest Firefox Beta[1] to check whether the upcoming release is on track to fix your issues. (As far as I know, it will; but there's no substitute for verifying with your actual use-case!)

[1] https://www.mozilla.org/en-US/firefox/channel/desktop/#beta

Comment 42

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

Jonathan, since we now have several duplicates reported, do you think this should be included in a dot release?   The duplicates weren't marked as affected for 49 or tagged as regressions, but looks like they should have been. 

Kohei, can you help if you have examples of games and html5 apps broken? I don't have a great way to assess user impact, other than seeing there are some duplicate bugs, and some things folks are saying, for example in comment 38.

Comment 43

[Kohei Yoshino]

Yeah, we haven't seen specific cases here, but my quick GitHub search finds 500K+ code including one from the popular Qt framework:

https://github.com/search?q=globalAlpha+OR+globalCompositeOperation&type=Code

I'm forwarding the question to Marton. Do you have any examples of broken apps or games?

Comment 44

[Kohei Yoshino]

Ah, well, some results may be not related to fillText. Try this:

https://github.com/search?q=globalAlpha+fillText&type=Code
https://github.com/search?q=globalCompositeOperation+fillText&type=Code

Comment 45

[Jonathan Kew [:jfkthame]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #42)
> Jonathan, since we now have several duplicates reported, do you think this
> should be included in a dot release?   The duplicates weren't marked as
> affected for 49 or tagged as regressions, but looks like they should have
> been.

I'm not sure I have a good sense of how serious the regressions are for sites that are affected -- I'd have thought that in many cases it's more cosmetic than a critical usability issue, but perhaps it's worse for some. (I haven't been seeing affected sites personally, but that's probably a function of the kinds of sites I tend to visit -- or not!)

Anyway, IMO the fix here is sufficiently contained and well-understood (and has been on beta and up for some time now) that we could include it in a dot release without significant risk. I wouldn't have thought it is critical enough to drive a dot release in its own right, but if we're doing one for other reasons I'd be happy to see this as a ride-along.

Comment 46

[Jonathan Kew [:jfkthame]]

I realized that the patch here needs minor rebasing in order to apply on mozilla-release (because of bug 1275693, which landed on mozilla-50). The actual fix required remains the same, though.

I'm doing a test build of Fx49 locally, and will post the rebased patch for possible approval later today after confirming that it works as expected.

Comment 47

[Jonathan Kew [:jfkthame]]

Attachment: [rebased for mozilla-49] Set up the DrawOptions appropriately when painting canvas text, and initialize font's DrawOptions from the textrun's options when painting, so we don't drop any opacity that has already been set

This is the combined patch (parts 1 and 2) rebased to apply on mozilla-release. It was necessary to incorporate the addition of the drawOpts field in TextRunDrawParams from bug 1275693 pt 2, as the patch here depends on that field, but it's still a straightforward fix. I've confirmed locally that this fixes the reftests here (which would fail with current release code), and have also pushed a try run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=37460c2486a733fac075e23f86c2922f9557570a) to double-check, although I'm not sure how well Try handles mozilla-release at the moment; it's possible other issues will show up there.

Comment 48

[adrian]

Attachment: Bug affect in "KeyLines"

Here is what our product "KeyLines" looks like in Firefox 49

Comment 49

[Jonathan Kew [:jfkthame]]

Comment on attachment 8801038 [details] [diff] [review]
[rebased for mozilla-49] Set up the DrawOptions appropriately when painting canvas text, and initialize font's DrawOptions from the textrun's options when painting, so we don't drop any opacity that has already been set

Approval Request Comment
(for consideration as a possible dot-release ridealong, see comment 42 and following)

[Feature/regressing bug #]: 1274936

[User impact if declined]: bad rendering for <canvas> text when global opacity or composition operators are used

[Describe test coverage new/current, TreeHerder]: reftests added in this bug

[Risks and why]: low-risk patch to pass the relevant properties from canvas through to the textrun drawing code

[String/UUID change made/needed]: none

Comment 50

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #47)
> have also pushed a try run
> (https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=37460c2486a733fac075e23f86c2922f9557570a) to
> double-check

Turns out that tryserver says we'll need to adjust the fuzz annotation a bit for Windows if we land the tests on release, but that's OK; it's not a real/worrying failure.

Comment 51

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

Comment on attachment 8801038 [details] [diff] [review]
[rebased for mozilla-49] Set up the DrawOptions appropriately when painting canvas text, and initialize font's DrawOptions from the textrun's options when painting, so we don't drop any opacity that has already been set

Let's take this as a ridealong for 49.0.2, since it seems to have a broad impact on game and html5 sites.  We will plan to release next week.

Comment 52

[Jonathan Kew [:jfkthame]]

Attachment: [for mozilla-49] Add reftests for use of globalAlpha and globalCompositeOperation with canvas text drawing

Patch with all the reftests here, folded together and with fuzz annotation updated for mozilla-release.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/775ab918e224
https://hg.mozilla.org/releases/mozilla-release/rev/f194bf906233