Open Bug 808288 Opened 7 years ago Updated 7 years ago

<canvas> text drawing does not handle missing glyphs properly

Categories

(Core :: Canvas: 2D, defect)

x86_64
All
defect
Not set

Tracking

()

REOPENED
mozilla19

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Depends on 1 open bug)

Details

(Whiteboard: [leave open])

Attachments

(5 files, 2 obsolete files)

In HTML content, when a character occurs for which no font can be found, we put a missing-glyph record in the textrun, which is rendered as a "hexbox" placeholder.

However, the canvas drawing code does not handle this, with the result that it treats the *character* code of the missing-glyph record as though it were a *glyph id* in the current font. This results in an incorrect glyph being rendered (quite likely the font's .notdef, which may be blank or a box, but it may result in a visible but completely wrong character appearing).

There are several possible fixes, listed here in likely order of complexity. Any one of these would be an improvement over the current behavior:

(1) simply don't render a glyph for missing-glyph records, so they remain blank in canvas;

(2) render glyph ID 0, the standard ".notdef" glyph slot, which will most often be either blank or a simple placeholder "box", though this is font-dependent;

(3) render a hexbox, like we do in HTML text.

The attached testcase uses the characters U+0528..052F, which are unassigned in Unicode (as of v6.2), and so are not expected to be found in any fonts; they should result in hexboxes in HTML content, but when drawn to <canvas>, they appear as unrelated, "arbitrary" glyphs depending on the particular fonts involved.
Note how the unassigned characters are rendered as completely unrelated glyphs in <canvas>, with different results in each of the fonts used. (Results are likely to vary across systems/versions, depending on the exact glyph palette of the fonts.)
This implements the simplest option - just leave the missing glyphs blank, advancing the x-position but not rendering anything.
Attachment #678022 - Flags: review?(bas)
This extends the preceding patch to render "missing" glyphs as glyph ID zero, which is defined as the .notdef glyph in truetype/opentype fonts. It could, in principle, be the wrong thing in other font technologies, but in practice it's a pretty safe bet. (I think we should do at least this, and preferably go the whole way to the hexbox option.)
Attachment #678023 - Flags: review?(bas)
And this replaces the use of .notdef (glyph 0) with code to call the gfxFontMissingGlyphs code that renders bitmap hexbox representations of the character code. This would make canvas text behave more like HTML text, and is potentially the most helpful to an author trying to diagnose problems with their page.
Attachment #678024 - Flags: review?(bas)
Attachment #678022 - Flags: review?(bas) → review+
Attachment #678023 - Flags: review?(bas) → review+
Comment on attachment 678024 [details] [diff] [review]
part 3 - draw missing-glyph hexboxes for <canvas> text, similar to missing glyphs in HTML text

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3040,5 @@
>            mTextRun->GetDetailedGlyphs(i);
>  
>          if (glyphs[i].IsMissing()) {
> +          float xpos;
> +          float advance = detailedGlyphs[0].mAdvance * devUnitsPerAppUnit;

In the previous patch we only took detailedGlyphs[0].mAdvance into account for LTR test, I'm guessing this change doesn't matter though.

@@ +3053,5 @@
> +          // we don't have to draw the hexbox for them
> +          if (advance > 0) {
> +            Matrix matrix = mCtx->mTarget->GetTransform();
> +            nsRefPtr<gfxContext> thebes;
> +            if (gfxPlatform::GetPlatform()->SupportsAzureContent()) {

This is a little tricky, potentially this could go wrong if we fell back to a Cairo context because of for example extremely large Canvas size. Cairo content is technically -not- supported, but SupportsAzureContent would return true as the browser uses D2D content.

I'm thinking Cairo content will be good enough to do DrawMissingGlyph though, so I'm not too worried, we should probably add a comment though, and keep an eye out.

@@ +3067,5 @@
> +            gfxFloat height = font->GetMetrics().maxAscent;
> +            gfxRect glyphRect(xpos, baselineOrigin.y - height,
> +                              advance, height);
> +            gfxFontMissingGlyphs::DrawMissingGlyph(thebes, glyphRect,
> +                                                   detailedGlyphs[0].mGlyphID);

I'd rather have seen us use Azure-code directly here, but I understand why that's a hard thing to do, this is pretty terrible though! But oh well.
Attachment #678024 - Flags: review?(bas) → review+
Yeah, I realize this isn't ideal, but I didn't want to completely reimplement gfxFontMissingGlyphs just for this. Eventually I expect we'll want to replace it and clean up the ugliness here.
Backed out for:
REFTEST TEST-UNEXPECTED-PASS | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/canvas/text-emoji.html | image comparison (!=), max difference: 255, number of differing pixels: 204
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5eec0a5b76ff

https://hg.mozilla.org/integration/mozilla-inbound/rev/a08d5817fe26
Ah yes - it makes perfect sense that test will no longer fail. Relanded, with the annotation in the reftest manifest updated accordingly:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58b4bd7b5065
https://hg.mozilla.org/mozilla-central/rev/58b4bd7b5065
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 837461
Partially backed out because of bug 837461.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 841335
Duplicate of this bug: 841491
(In reply to Jonathan Kew (:jfkthame) from bug 837461 comment #18)
> (It's not really a good solution, though, as users might not even realize
> that some of the text in their document isn't showing up.)

I agree. We should render something to inform the user that something went wrong.
It looks like this patch doesn't regress 837461. (Of course empty box is rendered. But we should fix the root cause of failing to load the font, instead of suppress the rendering.)
Attachment #717442 - Flags: review?(jfkthame)
Forgot to add a bug number in the comment.
Attachment #717442 - Attachment is obsolete: true
Attachment #717442 - Flags: review?(jfkthame)
Attachment #717443 - Flags: review?(jfkthame)
Comment on attachment 717443 [details] [diff] [review]
Reland part 2 & 3, but use .notdef glyph instead of hexbox when HWA is disabled

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

Alternatively, how about if we just reland part 2 for now? That should be "safe" in that it'll only draw the .notdef glyph, not hexboxes.

The problem with part 3 is that because of how pdf.js deals with font scaling via transforms on the context, it ends up drawing the hexbox at a hugely inflated size. I think we should try to address that by correctly handling the transforms involved, then we can re-land the hexbox support for all platforms, rather than having behavior that differs depending on the platform.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2365,5 @@
> +          }
> +          advanceSum += advance;
> +          // Do not draw hexbox when HWA is disabled to avoid bad
> +          // rendering by pdf.js (see bug 837461)
> +          if (!gfxPlatform::GetPlatform()->SupportsAzureContent()) {

Wouldn't this disable the hexbox on all non-Windows platforms, too? Is that actually necessary? (The comment doesn't match the code, as HWA may be enabled yet Azure Content disabled, AFAIK.)
Comment on attachment 717443 [details] [diff] [review]
Reland part 2 & 3, but use .notdef glyph instead of hexbox when HWA is disabled

OK, let's reland part 2.
Attachment #717443 - Flags: review?(jfkthame)
Attachment #717443 - Attachment is obsolete: true
Comment on attachment 678023 [details] [diff] [review]
part 2 - render the font's .notdef glyph for missing glyphs in <canvas>

This patch still applies cleanly.
Attachment #678023 - Flags: review?(jfkthame)
Comment on attachment 678023 [details] [diff] [review]
part 2 - render the font's .notdef glyph for missing glyphs in <canvas>

OK, I think it's fine for this to re-land.

It means that in cases where pdf.js fails to load a font and hence draws "missing" glyphs, it is likely to get a box rather than blank, which may make certain examples of problem PDFs look a bit worse (but it won't produce the huge ugly black blobs reported in bug 837461).

The real solution to that is for pdf.js to fix up the font resources so that they load successfully, not to hide the problem (and thus mislead the user into thinking they're seeing a complete document, when in fact parts are missing) by skipping those glyphs altogether.
Attachment #678023 - Flags: review?(jfkthame) → review+
Whiteboard: [leave open]
Depends on: 844643
You need to log in before you can comment on or make changes to this bug.