Last Comment Bug 698652 - Minimum font size and canvas don't get along very well
: Minimum font size and canvas don't get along very well
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on:
Blocks: 722569
  Show dependency treegraph
 
Reported: 2011-10-31 18:00 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-01-30 16:47 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Ignore the user's minimum font size when drawing to canvas (3.31 KB, patch)
2011-11-01 01:01 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
dbaron: review+
Details | Diff | Splinter Review
Test that minimum font size is ignored in canvas (2.83 KB, patch)
2012-01-14 19:30 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
dbaron: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-31 18:00:27 PDT
Problem: in pdf.js, we render PDFs to <canvas>.  PDFs quite commonly have most text explicitly positioned; for example, latex implements kerning by offsetting blocks of text.  This obviously leads to some serious borkenness, see https://github.com/andreasgal/pdf.js/issues/187 for more details.

I'm going to argue that minimum font size doesn't make sense for <canvas>.  Unlike HTML, there's no notion of reflow for canvas drawing, so when the UA doesn't do what the <canvas> drawer says, the result can just be totally unreadable, like what's happening in pdf.js.  There are also use cases for drawing text offscreen below the user's minimum font size, but never displaying the text.

In parallel, it would be nice to expose the minimum font size to web content, somehow.  In pdf.js, with a bit of work we can set up the zoom level so that the smallest font is the minimum font size.

I just pinged roc on IRC and he proposed the same solution, and also mentioned that we should turn off minimum font size for CSS transforms too.  I think we might want to do that for SVG as well.  Can be separate bugs I guess.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-31 18:09:11 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> In parallel, it would be nice to expose the minimum font size to web
> content, somehow.  In pdf.js, with a bit of work we can set up the zoom
> level so that the smallest font is the minimum font size.

The minimum font sizes are language and family-specific so that's not easy to do. At least treat it as a separate issue.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-31 18:12:36 PDT
Agreed.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-10-31 21:54:15 PDT
I think this is a one-liner.  See the difference between nsStyleFont::mSize and nsStyleFont::mFont::size as documented in nsStyleStruct.h and implemented in nsRuleNode::SetFont.  To fix, change nsCanvasRenderingContext2D::SetFont to use fontStyle->mFont.mSize instead of fontStyle->mFont.size.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-10-31 21:55:12 PDT
Er, should have written: ... to use fontStyle->mSize instead of fontStyle->mFont.size.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-01 01:01:26 PDT
Created attachment 570942 [details] [diff] [review]
Ignore the user's minimum font size when drawing to canvas

Thanks!  I wasn't sure whether I need to continue un-zooming the mSize value, but nsStyleStruct.h suggests the only difference between the two values is due to the minimum font size, so I assume that I also need to un-zoom mSize.

dbaron, how would you recommend testing this patch?  My first inclination is to write a reftest, but that would require flipping a pref (and so would need SpecialPowers or UniversalXPConnect).
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-11-01 07:41:19 PDT
See also bug 290044 for SVG.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-26 18:50:27 PST
Review ping.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-11-27 11:21:36 PST
Comment on attachment 570942 [details] [diff] [review]
Ignore the user's minimum font size when drawing to canvas

r=dbaron.  I'd suggest changing "natural size" to "computed size".
Comment 9 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-11-27 11:23:36 PST
I'd suggest using WindowSnapshot.js for tests.  Have a look at some of the existing tests that use it.  (It's basically reftest-inside-mochitest.)
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-14 19:30:15 PST
Created attachment 588704 [details] [diff] [review]
Test that minimum font size is ignored in canvas

Ugh, lost track of this patch :(.

The reftest harness grew pref() since this bug was filed, which is just what I needed.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-14 19:34:54 PST
See also: https://github.com/mozilla/pdf.js/issues/187 (because bugzilla won't let me "See also" github URLs)
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-24 23:11:01 PST
Hi dbaron, any ETA on the review of the test?  It's pretty small, +31 lines.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-01-27 13:40:54 PST
Comment on attachment 588704 [details] [diff] [review]
Test that minimum font size is ignored in canvas

change <html> to <html lang="en"> in both test and reference, and r=dbaron.

(Otherwise changing only the x-western pref shouldn't work when running tests on non-Western systems.)
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-27 22:02:06 PST
Pushed with <html lang="en">

https://hg.mozilla.org/integration/mozilla-inbound/rev/c16a73f0f3a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ea6b7164bc

Note You need to log in before you can comment on or make changes to this bug.