Closed
Bug 698652
Opened 13 years ago
Closed 12 years ago
Minimum font size and canvas don't get along very well
Categories
(Core :: Graphics: Canvas2D, defect)
Core
Graphics: Canvas2D
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: cjones, Assigned: cjones)
References
Details
Attachments
(2 files)
3.31 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
2.83 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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.
(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.
Assignee | ||
Comment 2•13 years ago
|
||
Agreed.
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.
Er, should have written: ... to use fontStyle->mSize instead of fontStyle->mFont.size.
Assignee | ||
Comment 5•13 years ago
|
||
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).
Assignee: nobody → jones.chris.g
Attachment #570942 -
Flags: review?(dbaron)
Comment 6•13 years ago
|
||
See also bug 290044 for SVG.
Assignee | ||
Comment 7•13 years ago
|
||
Review ping.
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".
Attachment #570942 -
Flags: review?(dbaron) → review+
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.)
Assignee | ||
Comment 10•12 years ago
|
||
Ugh, lost track of this patch :(. The reftest harness grew pref() since this bug was filed, which is just what I needed.
Attachment #588704 -
Flags: review?(dbaron)
Assignee | ||
Comment 11•12 years ago
|
||
See also: https://github.com/mozilla/pdf.js/issues/187 (because bugzilla won't let me "See also" github URLs)
Assignee | ||
Comment 12•12 years ago
|
||
Hi dbaron, any ETA on the review of the test? It's pretty small, +31 lines.
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.)
Attachment #588704 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 14•12 years ago
|
||
Pushed with <html lang="en"> https://hg.mozilla.org/integration/mozilla-inbound/rev/c16a73f0f3a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ea6b7164bc
Whiteboard: [inbound]
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c8ea6b7164bc https://hg.mozilla.org/mozilla-central/rev/c16a73f0f3a1
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•