Closed Bug 1304353 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Canvas: 2D, defect, P3)

49 Branch
x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 - fixed
firefox50 - fixed
firefox51 + fixed
firefox52 + fixed

People

(Reporter: pmarton80, Assigned: jfkthame)

References

()

Details

(4 keywords, Whiteboard: [gfx-noted])

Attachments

(11 files, 1 obsolete file)

552 bytes, application/x-javascript
Details
62.47 KB, image/png
Details
63.52 KB, image/png
Details
317 bytes, text/html
Details
1.73 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.14 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
5.15 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.66 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
4.99 KB, patch
Details | Diff | Splinter Review
43.39 KB, image/png
Details
7.11 KB, patch
Details | Diff | Splinter Review
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
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Attached file index.html (obsolete) —
Attached file main.js
Attached image Firefox_49.png
Attached image Firefox_48.0.2.png
Severity: normal → major
Attached file index.html
Attachment #8793276 - Attachment is obsolete: true
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
Blocks: 1274936
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Flags: needinfo?(jfkthame)
Keywords: regression, testcase
Keywords: site-compat
Flags: in-testsuite?
Is this something we should consider for uplift to 49 (if we patch it) or can we wontfix for 49?
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.
Flags: needinfo?(jfkthame)
Severity: major → normal
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Tracking 52+ for this site-compat regression.
Attachment #8793648 - Flags: review?(bas) → review+
Attachment #8793649 - Flags: review?(bas) → review+
Simple reftests for globalAlpha with fillText (fails before the patches here) and strokeText (already passes, but it seemed worth having tests for both APIs).
Attachment #8793905 - Flags: review?(bas)
Oops, I see the tests have tab characters in them. I'll de-tab them before landing to tidy things up.
Attachment #8793905 - Flags: review?(bas) → review+
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
Pushed by jkew@mozilla.com:
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
Track for 51+ as this is a site-compat regression.
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
Attachment #8793648 - Flags: approval-mozilla-beta?
Attachment #8793648 - Flags: approval-mozilla-aurora?
Flags: needinfo?(milan)
Duplicate of this bug: 1305447
Wontfix for 49, looks likely we can take this in 50 though.
Hello Marton, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(pmarton80)
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+
Attachment #8793648 - Flags: approval-mozilla-beta?
Attachment #8793648 - Flags: approval-mozilla-beta+
Attachment #8793648 - Flags: approval-mozilla-aurora?
Attachment #8793648 - Flags: approval-mozilla-aurora+
I don't feel the need to track now that it's fixed.
Duplicate of this bug: 1305710
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?
Flags: needinfo?(jfkthame)
Sure, why not. More tests == more fun for everyone. :)
Flags: needinfo?(jfkthame)
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.
Attachment #8795397 - Flags: review?(bas)
Attachment #8795397 - Flags: review?(bas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e791148312529e338c6958267406de69d198b66d
Bug 1304353 followup, add a reftest using globalCompositeOperation with canvas text painting. r=bas
Duplicate of this bug: 1306688
Duplicate of this bug: 1308540
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!
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
(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
Flags: needinfo?(pmarton80)
Duplicate of this bug: 1309254
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.
Flags: needinfo?(kohei.yoshino)
Flags: needinfo?(jfkthame)
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?
Flags: needinfo?(kohei.yoshino) → needinfo?(pmarton80)
(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.
Flags: needinfo?(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.
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.
Here is what our product "KeyLines" looks like in Firefox 49
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
Attachment #8801038 - Flags: approval-mozilla-release?
(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 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.
Attachment #8801038 - Flags: approval-mozilla-release? → approval-mozilla-release+
Patch with all the reftests here, folded together and with fuzz annotation updated for mozilla-release.
Attachment #8801228 - Attachment description: Add reftests for use of globalAlpha and globalCompositeOperation with canvas text drawing → [for mozilla-49] Add reftests for use of globalAlpha and globalCompositeOperation with canvas text drawing
See Also: → 1304539
Flags: needinfo?(pmarton80)
You need to log in before you can comment on or make changes to this bug.