Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide an option to decouple fontSize & canvasSize #19

Closed
jcppman opened this issue Mar 8, 2016 · 13 comments · Fixed by #20
Closed

provide an option to decouple fontSize & canvasSize #19

jcppman opened this issue Mar 8, 2016 · 13 comments · Fixed by #20
Labels
Milestone

Comments

@jcppman
Copy link
Contributor

jcppman commented Mar 8, 2016

Hi fellows,

First, thanks for this useful lib!

It would be very useful if an option like options.font.size or something is provided to use as font size while rendering text, as sometimes font-size and the actual dimensions of the rendered text on the canvas are quite different (and would be clipped if the actual size is bigger). please see the example here

so an optional option is really helpful as users can tweak it by themselve if meet such scenario.

I'd love to provide a PR if you like

Thanks

@jwarby
Copy link
Owner

jwarby commented Mar 8, 2016

Hi @jcppman, thanks for finding this issue and creating the JSFiddle!

It looks like this is naivety on my part when dealing with font sizing - I had a quick look earlier and hacked in something that seemed to work pretty well if the user defined the font size in ems rather than pxs. I'm fairly certain we can switch it around so that we can continue to use px and so that we don't need to add an additional option - I would much rather deal with this transparently so that the end user doesn't have to worry about fiddling around with the font size/canvas size.

I'll try and get something more concrete done this week and get back to you on it.

Thanks again!

@jcppman
Copy link
Contributor Author

jcppman commented Mar 8, 2016

Sounds awesome! Would you mind to leak more details you've found? Just curious :)

@jwarby
Copy link
Owner

jwarby commented Mar 9, 2016

Sure - I forked your original fiddle: https://jsfiddle.net/3v5c82xw/5/. In the fork, I pasted the plugin's code and added a function (getFontSize) to get the element's font size, which I borrowed and adapted from here: http://tzi.fr/js/convert-em-in-px. I multiplied the canvas size by the returned value from that function, then I changed the size option to use ems instead of pxs.

Having thought about it a little more, I'm not sure we will be able to fix this when the size option is specified in pxs, as I think we'd need to know the em ratio of the font that's being used... I'm a little fuzzy on font sizing and stuff TBH....

That said, perhaps the best solution will be to have the plugin support em/rem units for the size option, and then update the docs to recommend that users specify the size in those units to avoid this issue.

@jwarby jwarby added the bug label Mar 9, 2016
@jwarby jwarby added this to the v1.0.0. milestone Mar 9, 2016
@jcppman
Copy link
Contributor Author

jcppman commented Mar 9, 2016

Hmm, after digging I got something.

The current solution doesn't always work and here's the jsfiddle to demo the fail case: https://jsfiddle.net/5oybj1rh/3/ and please open the web console to see debug info.

The reason is, in current solution:

  • when using getFontSize() to get fontSize, what it get is the fontSize of the container, in this case .mat div, on my Chrome the default value is 16px.
  • we set options.size to 3em and after parsing it is 3
  • so canvasSize is set to 16px * 3 = 48 px
  • when we set context.font = '3em FontAwesome', what it refers to as 1em is the font size of his parent, in canvas it is the default value 10px Reference, so what we get is 30px FontAwesome
  • and 30px happens to be smaller than 48px so no clipping

however, clipping might happen if the font size of parent container is close to, equal or smaller than default canvas font size (10px).

the core issue is that there's no guarantee that when font-size is x, the actual dimensions of rendered character will be smaller than x, it all depends on font-icon designers.

I got an idea that might be able to solve this, please tell me what you think:

if there's no way to "predict" the actual size of rendered characters, fine, let's render it and "measure" it! Exactly like the way you get the unicode!

You can find the modified fiddle here: https://jsfiddle.net/pbLzhd55/4/

First I tried getComputedStyle but because the actual character (the :before) is display:inline so both width and height are "auto", and although his wrapper (the <i>) has width and height but the value is different to the actual dimensions (usually smaller, say if the character is 16 x 17 usually his wrapper is 16 x 16 or something).

so we need to use getBoundingClientRect, and since we can't get pseudo elements as an element and use getBoundingClientRect on it, I change the <i> to have display: inline and remove position: absolute to make it exactly big as the content inside, then wrap it with another position: absolute; wrapper to prevent it from affecting others.

now finally I got the exact size of the character!

obviously this method need to be improved a little bit because due to antialiasing or other reasons, when we draw a character with font-size: 18px on canvas the result is slightly different (you can see the canvas version in the fiddle) than a font-size: 18px character in DOM, it is blurred so might still be clipped a little bit.

this might be a solution: http://www.html5rocks.com/en/tutorials/canvas/hidpi/

@jwarby
Copy link
Owner

jwarby commented Mar 10, 2016

Awesome work @jcppman, thanks! Everything you've written above makes sense, and the demo looks good too. I got it into my head yesterday that we'd have to somehow know the size of an M character for the font (which Font Awesome and other icon fonts obviously doesn't have), but measuring the actual character we want to use is a much better idea.

I couldn't see any issue with blurring/clipping with cursor in the demo however. The canvas in the topbar does look blurred though. There's an existing issue I'm aware of, where adding an outline to the cursor looks awful in Chrome under *nix OSes. That cropped up with a Chrome update a while back - it always used to look fine, and still does (or at least did last time I checked) on Chrome for Windows.

I'll try and get your changes into a branch today and run the tests, and make sure all the various options still work as expected.

Thanks again for the hard work you've put into this issue!

@jcppman
Copy link
Contributor Author

jcppman commented Mar 10, 2016

I'm glad that it helps! 😈

The blurring & clipping issue might have something to do with screen resolution, and clipping is a little bit critical than blurring I'd say. Try some "naughtier" icons like "fa-reddit-alien" or "fa-arrows-v", on my side they're very "clipped"

@jwarby
Copy link
Owner

jwarby commented Mar 10, 2016

Those icons work fine for me with your changes - are they still clipped for you even with your changes?

@jcppman
Copy link
Contributor Author

jcppman commented Mar 10, 2016

Interesting...

Yeah they are clipped on my side:
clip-clip

My env:
Chrome 49 on OSX 10.10

[edit] And I just noticed that the alien doesn't smile anymore on my side hahaha, must be some antialiasing thing that eliminates the small transparent smiling line

so this could be put as a different issue i guess

@jcppman
Copy link
Contributor Author

jcppman commented Mar 10, 2016

I just tried it on a windows machine with normal resolution and it works perfectly. So it must be the canvas HiDPI supporting issue

FYI I found a solid solution, but only works on Chrome: combine window.devicePixelRate with -webkit-image-set: https://jsfiddle.net/pbLzhd55/8/, and here is the reference.

@jwarby
Copy link
Owner

jwarby commented Mar 21, 2016

@jcppman Looks Firefox aren't in any particular hurry to implement the image-set CSS, so I'm not sure where to go with this next. I'd love to hack around and try a few things but I don't have access to HiDPI screen :(

@jcppman
Copy link
Contributor Author

jcppman commented Mar 21, 2016

yeah I guess this one (HiDPI compatibility) is more trivial than the original clipping issue, let's wait for browsers to grow up a bit :)

@jwarby
Copy link
Owner

jwarby commented Mar 21, 2016

OK good plan - let's get this one fixed first, and get a separate issue raised for the HiDPI screens issue. Do you want to go ahead and create a pull request with your original changes implemented, or do you want me to implement it?

jcppman added a commit to jcppman/jquery-awesome-cursor that referenced this issue Mar 21, 2016
if the icon is too wide/tall, it wil be clipped. The new approach of
finding dimensions is measuring the actual rendered icon.

closes jwarby#19
@jcppman
Copy link
Contributor Author

jcppman commented Mar 21, 2016

Actually I do have a branch that contains the fix, but somehow I can't pass the test on both PhantomJs and real browsers, even without the fixing, on both my windows & osx machine.

I'd create the PR first anyway, and if you find that the way I did the fix is not proper, feel free to implement it by yourself & close it :)

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants