[Text] Fix <Text> letterSpacing right side padding

[robertjpayne]

Letter spacing should not be applied to the last letter of any NSAttributedString as doing so pad's the last letter on the right hand side which results in slightly misaligned text.

[ide]

LGTM though the Apple docs seem to imply that the kerning value applies between characters which I read to mean not at the end of the string.

NSKernAttributeName
The value of this attribute is an NSNumber object containing a floating-point value. This value specifies the number of points by which to adjust kern-pair characters. Kerning prevents unwanted space from occurring between specific characters and depends on the font. The value 0 means kerning is disabled. The default value for this attribute is 0.

Adding the reviewers on the original diff #482
cc @nicklockwood @a2

[vkurchatkin]

@robertjpayne can you make a screenshot which demonstrates the problem?

[robertjpayne]

@ide @vkurchatkin please see attached screenshots of the issue. The last trailing issue is also having the kern added thus resulting in a bit extra width.

It was causing centre alignment to look just every so slightly off.

The background color here is to illustrate the issue.

Before:

After:

[vkurchatkin]

Can you also post the code?

[robertjpayne]

@vkurchatkin I can't really post the whole thing, what bit are you after? Just a working sample?

[robertjpayne]

@vkurchatkin run this index.ios.js example, it's very easy to reproduce and see the issue the larger letter spacing is.

/**
 * Sample React Native App
 * https://github.com/facebook/react-native
 */
'use strict';

var React = require('react-native');
var {
  AppRegistry,
  StyleSheet,
  Text,
  View,
} = React;

var TextLetterSpacing = React.createClass({
  render: function() {
    return (
      <View style={styles.container}>
        <Text style={styles.welcome}>
          WELCOME
        </Text>
        <Text style={styles.instructions}>
          To get started, edit index.ios.js
        </Text>
        <Text style={styles.instructions}>
          Press Cmd+R to reload,{'\n'}
          Cmd+D or shake for dev menu
        </Text>
      </View>
    );
  }
});

var styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  welcome: {
    fontSize: 20,
    textAlign: 'center',
    margin: 10,
    letterSpacing: 30,
    backgroundColor: '#FF0000',
  },
  instructions: {
    textAlign: 'center',
    color: '#333333',
    marginBottom: 5,
  },
});

AppRegistry.registerComponent('TextLetterSpacing', () => TextLetterSpacing);

[robertjpayne]

@ide @nicklockwood know you two are super busy but just want to bump this one to see thoughts

[nicklockwood]

@robertjpayne sorry for not getting back sooner. Looking at your "ACTIVE CHALLENGES" example above, it seems like the "ACTIVE" is still off-center - i.e. this fix doesn't work for wrapped lines, only the last line in the string, which seems a bit unsatisfactory.

Is there another solution we could try, such as offsetting the whole rendered string by half the width of one character space?

[nicklockwood]

I noticed this clause in the code:

if (shadowText.effectiveLetterSpacing < 0) {
  result.dimensions[CSS_WIDTH] -= shadowText.effectiveLetterSpacing;
}

If I understand correctly, this is intended to compensate for a bug where a negative letterspacing causes the width to be too short by one letter space. But I think this is actually the same bug you're dealing with. If the if statement was removed, so that the code was just

result.dimensions[CSS_WIDTH] -= shadowText.effectiveLetterSpacing;

I think it might remove the extra space at the end of the string. Wanna try that?

[vkurchatkin]

@nicklockwood actually current behaviour with positive letter spacing matches web: http://jsfiddle.net/tuo37tq9/

The behaviour with negative letter spacing would be different because of this workaround: "dangling" part of the string is not clipped on the web, but doesn't contribute to width of the block:

http://jsfiddle.net/9absfuu3/

[robertjpayne]

@nicklockwood actually good point too I didn't catch that the first line isn't fixed.

And @vkurchatkin wasn't aware this is behaviour of the web as well.

Closing this because it's probably not easy to fix and matches the web behaviour anyways.

[nicklockwood]

I'm not sure we should be slavishly matching web behaviour even if the web behaviour is wrong. It seems to me pretty clear cut that having letter spacing work the way that it does currently is not desirable, and if we can fix it then I think maybe we should. @vjeux, what are your thoughts on this?

[vkurchatkin]

@nicklockwood that's what @vjeux told me to do. to be honest, most of web behaviour related to layout is wrong or weird :-)

[nicklockwood]

@vkurchatkin I agree that it should work the same in terms of the actual spacing between characters, but having spacing after the last character seems broken. I cannot imagine a situation where you would want it to work that way - it's something you'd have to work around on the web rather than something you'd seek to emulate.

Also, one of the reasons we want stuff to work the same as on the web is so that we can make a web-based polyfill for React Native. But since the fix here is probably just adding or subtracting a known value from the frame width, there's no reason we couldn't make the web work the same way if we wanted to, so I don't think that's a valid reason not to fix this problem.

[robertjpayne]

@nicklockwood so the solution would probably be altering the frame based on the known kerning size. AFAIK kern is a fixed point value and independent of the font itself.

The hard thing while experimenting with it is that if you shrink the frame size you'll actually get truncated characters. So we'll need to solely offset the frame's origin.

[robertjpayne]

@nicklockwood the other really hard part -- what if we only have kern applied to part of the text? This is where it gets really tricky.

[vkurchatkin]

I think what @nicklockwood proposed is enough and actually makes perfect sense

[vjeux]

Sometimes I love the web but sometimes... Seriously, the way letter-spacing is implemented in CSS makes absolutely no sense :(

In order to get what would be considered the sane behavior, we can use a negative margin trick:

<span style="background-color: red; letter-spacing: 10px; color: white;">
  Hello<span style="margin-left: -10px; width: 0;"></span>
</span>

If we implement letterSpacing CSS attribute, we should implement it with the same quirks as CSS otherwise it's going to be super confusing and lead to annoying to debug problems.

However, I'm not opposed to not implementing letterSpacing but implementing the right behavior using a different name like letterKerning or something. And, when we do the web version, we can polyfill letterKerning with the negative margin trick.

[ghost]

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.