Drawing text to a <canvas> overlaps incorrectly (Windows, probably Azure)

VERIFIED FIXED in Firefox 8

Status

()

Core
Graphics
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: bsmedberg, Assigned: jfkthame)

Tracking

({regression})

unspecified
mozilla8
x86_64
Windows 7
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox7 affected, firefox8 fixed, firefox9 fixed, firefox10 fixed)

Details

(Whiteboard: [qa!])

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
This got found/reported to me via the aero-window-title extension: painting the following text to a canvas will overlap the text incorrectly (although .measureText still reports the correct bounds!):

【紅魔郷 ボーカル】Foreground Eclipse - Iris 

Testcase will be attached in a moment: this works correctly on non-Windows and on Windows with acceleration disabled, so I suspect Azure.
(Reporter)

Updated

6 years ago
Keywords: regression
(Reporter)

Comment 1

6 years ago
Created attachment 566223 [details]
Testcase HTML
(Assignee)

Comment 2

6 years ago
Looks to me like it's not handling the fact that font fallback kicks in and results in the string being rendered as several separate glyph runs, because the Japanese characters aren't supported in the default font. It appears that each glyph run is being drawn at the same starting position, instead of each glyph run being drawn from the ending position of the previous one.
(Assignee)

Comment 3

6 years ago
Created attachment 566258 [details] [diff] [review]
patch, don't overlap successive glyph runs

So a simple fix is to move the initialization of advanceSum out of the glyph-run loop, so that it accumulates across multiple runs instead of resetting for each.
Attachment #566258 - Flags: review?(bas.schouten)
Comment on attachment 566258 [details] [diff] [review]
patch, don't overlap successive glyph runs

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

This looks identical to what I was thinking of, so I'm all for it. I should've known the advanceSum wasn't per run since simple glyphs don't have another way of passing X offset.
Attachment #566258 - Flags: review?(bas.schouten) → review+
(Reporter)

Comment 5

6 years ago
Comment on attachment 566258 [details] [diff] [review]
patch, don't overlap successive glyph runs

Can this be reftested?
(Assignee)

Comment 6

6 years ago
Created attachment 566321 [details] [diff] [review]
reftest, check that Chinese and Latin runs aren't overprinted

We can at least do a != test to check that this specific bug is fixed.

A test that the rendering of multiple glyph runs within a single canvas text call is exactly _correct_, not just that it isn't _wrong_ in this particular way, seems like it would be trickier to do in a robust and portable way, as we're depending on font fallback behavior here in order to get multiple runs.
Attachment #566321 - Flags: review?(bas.schouten)
(Reporter)

Comment 7

6 years ago
Should drawing text to a canvas and having a <span> with the same text produce identical results?
(Assignee)

Comment 8

6 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Should drawing text to a canvas and having a <span> with the same text
> produce identical results?

Although in theory that sounds plausible, I'd be pretty surprised if that was reliably true, right down to the subpixel level - especially when we're rendering the canvas via Azure and the <span> via thebes/cairo.
(Assignee)

Comment 9

6 years ago
Pushed the patch to -inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5152a05d616

(Reftest not landed yet, awaiting Bas's review.)
(Assignee)

Updated

6 years ago
status-firefox10: --- → affected
status-firefox7: --- → affected
status-firefox8: --- → affected
status-firefox9: --- → affected
Target Milestone: --- → mozilla10
(Assignee)

Updated

6 years ago
Blocks: 651858
(Assignee)

Comment 10

6 years ago
Comment on attachment 566258 [details] [diff] [review]
patch, don't overlap successive glyph runs

Requesting approval for Aurora and Beta. This is a regression that is liable to show up any time font fallback occurs with <canvas> text strings - which can easily occur in multilingual situations, even if it's not common in simple English-only demos.

The patch is very safe - localized to the Azure canvas DrawText function, and a simple and well-understood fix - so I don't think there's any significant risk involved in taking this for FF8 and FF9.
Attachment #566258 - Flags: approval-mozilla-beta?
Attachment #566258 - Flags: approval-mozilla-aurora?
Attachment #566321 - Flags: review?(bas.schouten) → review+
(Assignee)

Comment 11

6 years ago
Pushed reftest to -inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92db762bf8d6
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/b5152a05d616
https://hg.mozilla.org/mozilla-central/rev/92db762bf8d6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Assignee: nobody → jfkthame
status-firefox10: affected → fixed

Updated

6 years ago
Attachment #566258 - Flags: approval-mozilla-beta?
Attachment #566258 - Flags: approval-mozilla-beta+
Attachment #566258 - Flags: approval-mozilla-aurora?
Attachment #566258 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b2f3b19ebe42
https://hg.mozilla.org/releases/mozilla-beta/rev/522217082f0d
status-firefox8: affected → fixed
status-firefox9: affected → fixed
Target Milestone: mozilla10 → mozilla8

Comment 14

6 years ago
I tested to see if the issue is still reproducible after the landing of the fix; I got the following results:
VERFIED FIXED:
 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0 (Beta)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a2) Gecko/20111019 Firefox/9.0a2 (Aurora)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111020 Firefox/10.0a1 (Nightly)

Since all the issue is fixed on all branches setting resolution to Verified and whiteboard entry to qa!
Status: RESOLVED → VERIFIED
Whiteboard: [qa!]
You need to log in before you can comment on or make changes to this bug.