Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Support for Basic Latin Ligatures (\uFB00 to \uFB06). Partially resolves #38 #233

Merged
merged 2 commits into from

2 participants

@mdumic

Added support for Basic Latin Ligatures (Code points \uFB00 to \uFB06, as per Unicode 6.1 / Latin Ligatures)

The core cufon.js does substitution of letter groups to Unicode ligatures if the font file defines glyphs at those code points.

I left the generator unchanged as it already includes the ligature range if option "all" is checked. Though, it could be improved by adding specific range just for ligatures.

I tested the feature with "PT Sans" (2 glyphs) and "Anivers" (5 glyphs) but I'm not sure these should be included into the project so I left the examples out as Vegur does not have glyphs for ligatures.

@sorccu
Owner

I like it, but there are a few things I would change:

Would like to have:

  1. Please no deleting / changing empty lines, I'm not saying the original is perfect either, but those changes unnecessarily complicate the patch and are irrelevant anyway.
  2. Please no whitespace between "function" and "(". Again, I'm not saying that either style is wrong, this is just to keep the coding style consistent.
  3. Remove the semicolon after the if () {} inside the for. To keep the coding style consistent I would perhaps add {} to the for because it's not a one-liner.
  4. Add a semicolon after the last function in the return, this time it's an expression so a semicolon would be nice to have.
  5. I see 4-wide and 2-wide indents... could be just GitHub formatting, but make sure that all indents are tabs. Again, for consistency.

Must have:

  1. Move latinLigatures inside Font (or possibly even inside applyLigatures to keep almost all the changes in one place). Yes, it will be evaluated once for each Font instance, but it won't make any real difference because the instances are so few.
  2. Text nodes, on the other hand, can be quite numerous and function calls are relatively expensive. Ligatures are also quite rare (well, mainly because the generator filters them out) so this is an unnecessary penalty. Property access is quite a bit faster, so I would prefer something like:

    Cufon.CSS.textTransform(font.applyLigatures ? font.applyLigatures(text) : text, style).split('');
    

    which I presume to be somewhat faster (although benchmarks would be nice as well).

  3. There must be a JS option to disable ligatures altogether. Some of the font ranges in the generator (namely Latin-1 Supplement) include ligatures. They are filtered out for now, but maybe not in the future. I am not sure if the option should be enabled or disabled by default, but I have a feeling that having it enabled by default could lead to some users getting surprised. Unfortunately, not many people even know what ligatures are and would not know what to look for to disable the feature.

Can't be helped:

  1. Having to change both renderers sucks, but it is indeed the way it has to be for now.

I will pull this if you fix these issues :)

Thank you for your comments. I'll make necessasy fixes. I have just few questions/comments:

  1. Two/four wide indents: I use tabs for indents. One tab as normal block indent and two tabs to indicate continuation of previous line, like this:

    var x = condition
            ? trueValue
            : falseValue;

    Is this ok?

  2. I would put intermediate function between Cufon.CSS.textTransform and font.applyLigatures, something like:

    function preprocessText (text, font, style) {
        if (font.applyLigatures)
            text = font.applyLigatures(text)
    
        return Cufon.CSS.textTransform(text)
    }

    and then invoke it:

    var chars = preprocessText(text, font, style).split('');

    There is already dependency on external transformer in both renderers. I'm just replacing it with one more generic. That way we dont need to change both renderers (later).

    Then we can even do:

    var preprocessText = useLigatures // global option, true by default
            ? function (text, font, style) {
                    if (font.applyLigatures)
                        text = font.applyLigatures(text)
    
                    return Cufon.CSS.textTransform(text)
                }
            : Cufon.CSS.textTransform

    to avoid having to recheck if ligatures are disabled globally.

    (By the way, why do you use split when you can do String.charAt(), it is just as good in while loop? I could clean this up in different pull request if you want)

  3. I thought about option for disabling ligatures too, but when I saw they are filtered out in generator unless you check "all" (which is rarely the case, I believe) I decided they should be used if the glyphs are there.
    I'll add an option: useLigatures = true, as the default.

These are my first steps with Github, so I need some guidance on how to proceed? Should I make make commit on top of this one and resubmit pull request or should I scrap this and make new one? What are the proper steps?

Owner

Hi,

  1. For consistency I would only use one tab this time. Consistency makes the source easier to edit by other people as well.
  2. I like it, but the availability of the function may be an issue since the renderers only share the global context. We may have to create api.preprocessText within the Cufon core if we want to avoid further refactoring. One solution would be to simply wrap everything inside an anonymous function and place the function there, however I would not accept it because I don't want to anything to be out of place.

    Haven't really thought about the split vs charAt issue. I suppose the original reason was that it was easier to work with. I would imagine that it's slower because of the additional function calls, though. Is that what you had in mind? Seems to be about 17% slower in my Chrome.

  3. Good, but I may just have had a slightly better idea. First of all, let's rename the option to "ligatures". Then, let's make the default value of that option the object which latinLigatures currently holds, and get rid of latinLigatures altogether. This will allow people to add their own ligatures should the default ligature list prove insufficient. To disable ligatures, one would set the ligatures option to null. This does mean that the ligature regexp would have to be recreated each time a new Cufon.replace call is made, but it may be cacheable the same way for example textShadow and other options are (in Cufon.replace, although the caching is starting to take up quite a bit of space there...). So yeah, it's slower but I much prefer the idea of flexibility.

Thoughts?

We're getting closer. :) I'm OK with 1 and 3 (discretionary ligatures are next on my list and this was the way I hoped we could do it).
I'm not sure how to do 2, however. Your points are valid, but I don't see good solution:

  • we could make BaseEngine (and put common stuff in and extend from that) but I don't see anything else common to both engines.
  • we could make api.registerEngine inject util functions but that would be overkill as well.
  • we could make transformer function an option but there is already modifyText, which, unfortunately is not appropriate for the use here.
  • or we could just leave as is for now: Cufon.CSS.textTransform(font.applyLigatures ? font.applyLigatures(text) : text, style).split('');.

Is there other way? What do you suggest?

By the way I tried accessing chars with indexer (string[]) as well. This is way faster in IE but I wouldn't change to that though.

You didn't give me any comments on how to submit new patch?

@sorccu
Owner

Hi,

You should be able to update the pull request by simply pushing more commits to the same branch you initiated the pull request from (mdumic:ligatures).

@mdumic mdumic Refactored support for ligatures to support configuration option (ope…
…ning the way for discretionary ligatures). Also, fixed coding style issues with in commit.
91a7c02
@mdumic

Just a note about the commit:

I realized the Font and options are in many-to-many relation (due to the "autoDetect" option), so neither can simply store the other. That made the caching within Font.applyLigatures a little more complicated than I wanted, but I think it's pretty good overall.

@sorccu sorccu merged commit 94db085 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 12, 2012
  1. @mdumic
Commits on Mar 8, 2012
  1. @mdumic

    Refactored support for ligatures to support configuration option (ope…

    mdumic authored
    …ning the way for discretionary ligatures). Also, fixed coding style issues with in commit.
This page is out of date. Refresh to see the latest.
Showing with 51 additions and 4 deletions.
  1. +51 −4 js/cufon.js
View
55 js/cufon.js
@@ -344,7 +344,7 @@ var Cufon = (function() {
function Font(data) {
- var face = this.face = data.face, wordSeparators = {
+ var face = this.face = data.face, ligatureCache = [], wordSeparators = {
'\u0020': 1,
'\u00a0': 1,
'\u3000': 1
@@ -416,6 +416,43 @@ var Cufon = (function() {
return jumps;
};
+ this.applyLigatures = function(text, ligatures) {
+ // find cached ligature configuration for this font
+ for (var i=0, ligatureConfig; i<ligatureCache.length && !ligatureConfig; i++)
+ if (ligatureCache[i].ligatures === ligatures)
+ ligatureConfig = ligatureCache[i];
+
+ // if there is none, it needs to be created and cached
+ if (!ligatureConfig) {
+ // identify letter groups to prepare regular expression that matches these
+ var letterGroups = [];
+ for (var letterGroup in ligatures) {
+ if (this.glyphs[ligatures[letterGroup]]) {
+ letterGroups.push(letterGroup);
+ }
+ }
+
+ // sort by longer groups first, then alphabetically (to aid caching by this key)
+ var regexpText = letterGroups.sort(function(a, b) {
+ return b.length - a.length || a > b;
+ }).join('|');
+
+ ligatureCache.push(ligatureConfig = {
+ ligatures: ligatures,
+ // create regular expression for matching desired ligatures that are present in the font
+ regexp: regexpText.length > 0
+ ? regexpCache[regexpText] || (regexpCache[regexpText] = new RegExp(regexpText, 'g'))
+ : null
+ });
+ }
+
+ // return applied ligatures or original text if none exist for given configuration
+ return ligatureConfig.regexp
+ ? text.replace(ligatureConfig.regexp, function(match) {
+ return ligatures[match] || match;
+ })
+ : text;
+ };
}
function FontFamily() {
@@ -813,6 +850,7 @@ var Cufon = (function() {
var C_SHY_DISABLED = 'cufon-shy-disabled';
var C_VIEWPORT_RESIZING = 'cufon-viewport-resizing';
+ var regexpCache = {};
var sharedStorage = new Storage();
var hoverHandler = new HoverHandler();
var replaceHistory = new ReplaceHistory();
@@ -878,7 +916,16 @@ var Cufon = (function() {
ul: 1
},
textShadow: 'none',
- trim: 'advanced'
+ trim: 'advanced',
+ ligatures: {
+ 'ff': '\ufb00',
+ 'fi': '\ufb01',
+ 'fl': '\ufb02',
+ 'ffi': '\ufb03',
+ 'ffl': '\ufb04',
+ '\u017ft': '\ufb05',
+ 'st': '\ufb06'
+ }
};
var separators = {
@@ -1098,7 +1145,7 @@ Cufon.registerEngine('vml', (function() {
wStyle.height = size.convert(font.height) + 'px';
var color = style.get('color');
- var chars = Cufon.CSS.textTransform(text, style).split('');
+ var chars = Cufon.CSS.textTransform(options.ligatures ? font.applyLigatures(text, options.ligatures) : text, style).split('');
var jumps = font.spacing(chars,
getSpacingValue(el, style, size, 'letterSpacing'),
@@ -1317,7 +1364,7 @@ Cufon.registerEngine('canvas', (function() {
}
}
- var chars = Cufon.CSS.textTransform(text, style).split('');
+ var chars = Cufon.CSS.textTransform(options.ligatures ? font.applyLigatures(text, options.ligatures) : text, style).split('');
var jumps = font.spacing(chars,
~~size.convertFrom(parseFloat(style.get('letterSpacing')) || 0),
Something went wrong with that request. Please try again.