Merge pull request #6 from compositive/improve-test-suite

[lordofthelake]

Some background about why this pull request: I'm trying to build a library compatible with node-sketch-bridge, exposing the same API and behavior, without relying on macOS APIs, so that it can be run e.g. on a Linux server.

In order to do that, I needed to build a spec to adhere to. I'm planning of using node-sketch-bridge as "oracle", checking that my library behaves the same, but before codifying its behavior, with quirks and all, I wanted first to check that this is actually consistent with how Sketch behaves and I tried to shake out some edge cases.

I left comments in the form of TODOs in places where I think that the behavior is possibly inconsistent, but not necessarily wrong; I would welcome some comments on those to understand whether what I observed from the tests is expected behavior or not.

I've added FIXMEs and skipped tests, instead, where I encountered something that definitely qualifies as a bug. I would have tried to fix the problems myself, but my Objective-C is not so hot and before attempting any change in behavior I wanted to have some feedback.

[lordofthelake]

Another discovery: some behaviors change between macOS 10.15 (Catalina – running on GitHub CI) and macOS 10.14 (Mojave – my local machine). Probably the behavior is still different on macOS 10.13 (High Sierra), that is the earliest version that Sketch 61 supports.

The differences are around two areas:

• What is returned as "system font": the variants of .SFNS* differ among OS versions. Since, from my knowledge of Sketch, there is no way of selecting "the default system font", would it make sense to change the resolution for a missing or omitted font to something else?
• How PNGs are (re-)encoded: there is probably some difference in the default settings for the compression, etc. I haven't dug too much into the details. Not re-encoding files might help here? I don't have enough knowledge about Sketch internals to understand whether there is a constraint to represent everything as PNGs though.

This is making the test suite fail at the moment on GitHub CI. I would have loved to set up a matrix for different macOS versions, but they just support the latest one, so no dice there.

I find this troubling because it means that identical React components, when rendered through React-Sketchapp, will create different files on different OS versions. Possibly, given the different font resolution, the generated files might also be broken when moving from an earlier to a later macOS version?

[dabbott]

@lordofthelake thanks for the PR. This is very thorough and it'd be awesome if we were able to create a spec that matches Sketch.

At some point I had a couple tests for measured font dimensions and encoded PNGs, but I ended up removing them once I realized it might get time-consuming to figure out.

For macOS differences:

• System fonts: I like the idea of using a more consistent default font, since generating consistent components is a core use case for react-sketchapp. We already fall back to Helvetica if we can't figure out the system fonts, after all.
• PNG encoding: I decided not to dig into this either. IMO if the API produces a valid PNG then that's good enough, since I'm not sure if there's a strong use case for it being consistent at the byte level.

If we do change the font resolution in some way, we'll need to release a new major version of react-sketchapp I think.

@mathieudutour thoughts?

For bugs, e.g. the failing jpg, can we open a separate Github issue for each so it's a bit easier to track?

I think we might also want to split out the line height measurement into a separate issue, and merge in the tests that do work first. It sounds pretty daunting to get 100% compatible text measurements on Linux (I'm not even confident we can do it with the Mac APIs, at least not without getting the exact algo from Sketch) but definitely interested to see how it goes.

[mathieudutour]

Nice PR!

• Default font: React Native uses the system font as the default font. react-sketchapp aims to match RN behavior. I think there is a difference between omitted and missing font family:

  • omitted -> default font -> system font
  • missing -> fall back to font that we know exists (Helvetica) and print a warning

  I understand that it's annoying for tests since the system font can change but if we agree that it's the expected behavior, then the fact that tests are hard is a bit secondary.

• PNG encoding: IMO this API can be expressed in pure NodeJS (doing the request, and base64 the result). Just need to check if it's a local image or remote image because NSURL will work either way. I didn't do it for lack of time but it might be a better option than try to match the behavior on different platform. I'm fine with releasing a breaking version if there are small differences (encoding, etc.).

PS: Are you working with @nrydevopswatch?

[lordofthelake]

@dabbott Thanks for the feedback!

About the bigger problems:

• System fonts: I like the idea of using a more consistent default font, since generating consistent components is a core use case for react-sketchapp. We already fall back to Helvetica if we can't figure out the system fonts, after all.

That's what I was expecting to see in the tests, given the source code: if the font is the "system font", fall back to Helvetica, that seemed like a sane default. My question was: is the current behavior (returning .SFNS*) intended or is it a bug and it was supposed to return Helvetica?

I can run a test with the sketchImpl of react-sketchapp and see what happens there; if it returns Helvetica, it should probably be fixed here as a bug. Otherwise yes, I agree that it would be a breaking change for react-sketchapp and it would be a major version bump.

• PNG encoding: I decided not to dig into this either. IMO if the API produces a valid PNG then that's good enough, since I'm not sure if there's a strong use case for it being consistent at the byte level.

I would probably not try to fix the behavior of the re-encoding either, because I agree that it's pointless, as long as it's a valid image. But my question is: do we actually need to re-encode as PNG? Can't we just give Sketch the JPG/original PNG/GIF/whatever? It would certainly be a whole lot easier, it would get rid of the JPG bug "by design" and it would give consistent results cross-platform.

Is there something that I don't know about image handling in Sketch that makes it work only with PNGs?

For bugs, e.g. the failing jpg, can we open a separate Github issue for each so it's a bit easier to track?

Sure.

I think we might also want to split out the line-height measurement into a separate issue, and merge in the tests that do work first. It sounds pretty daunting to get 100% compatible text measurements on Linux (I'm not even confident we can do it with the Mac APIs, at least not without getting the exact algo from Sketch) but definitely interested to see how it goes.

As long as the line height is provided, so far I've had good results in node-sketch-bridge-multiplatform for the rest.

For the undefined behavior when a line-height is not set, I'm not sure it can be fixed either. I've spent quite a bit into this and Windows, Linux and macOS all use a different way of determining a default line-height. The same goes for different browsers.

The only thing I would worry about would be that the nodeImpl and the sketchImpl behave the same when a line-height is not provided. Otherwise, the layout will "jump" when re-rendered through a different method. If they don't, I think that a sensible thing to do would be to add a default multiplier of the font size, whatever it would be (1.19 - 1.5 are popular choices), and keep it consistent at least within the different drivers of react-sketchapp.

[mathieudutour]

For the undefined behavior when a line-height is not set, I'm not sure it can be fixed either. I've spent quite a bit into this and Windows, Linux and macOS all use a different way of determining a default line-height. The same goes for different browsers.

I thought that the default line-height is contained in the font data and that what macOS is using but that browsers use a multiple of the font size instead (because it's more consistent sometimes).

Regarding the differences between nodeImpl and sketchImpl, there shouldn't be any since it's the same code (written in obj-c instead of cocoascript).

[lordofthelake]

Nice PR!

• Default font: React Native uses the font system as the default font. react-sketchapp aims to match RN behavior. I think there is a difference between omitted and missing font family:

  • omitted -> default font -> system font
  • missing -> fall back to font that we know exists (Helvetica) and print a warning

  I understand that it's annoying for tests since the system font can change but if we agree that it's the expected behavior, then the fact that tests are hard is a bit secondary.

No, test-wise it's not a problem, I can just match it in a fuzzier way. My question is more about what will Sketch render if I get a file generated in Mojave and move it to Catalina. In theory, it shouldn't complain about missing fonts, because both have SF. But it might in this case, because the font name gets "resolved" to the "wrong" thing (for the current OS version) and committed to the .sketch file?

• PNG encoding: IMO this API can be expressed in pure NodeJS (doing the request, and base64 the result). Just need to check if it's a local image or remote image because NSURL will work either way. I didn't do it for lack of time but it might be a better option than try to match the behavior on different platform. I'm fine with releasing a breaking version if there are small differences (encoding, etc.).

So we just need to fetch the image and base64-encode and it's not a requirement to re-encode it to PNG? If that's the case, it makes things a whole lot easier (and the code more deterministic across platforms).

PS: Are you working with @nrydevopswatch?

No, although I've seen his issue. This is part of a bigger project, Compositive, and I had already written 2/3 of a multi-platform implementation for the nodeImpl a few months ago in a private fork of react-sketchapp. Now that the native bindings have been extracted, I'm trying to "do it right" and contribute upstream :)

[lordofthelake]

For the undefined behavior when a line-height is not set, I'm not sure it can be fixed either. I've spent quite a bit into this and Windows, Linux and macOS all use a different way of determining a default line-height. The same goes for different browsers.

I thought that the default line-height is contained in the font data and that what macOS is using but that browsers use a multiple of the font size instead (because it's more consistent sometimes).

The answer here is "it depends" :D

That was also my assumption, and then I started digging into the OpenType font tables and see how other implementations calculate the line-height. The majority of implementations use two metrics from the font that determine the size of the ascenders and the descenders, plus a third optional metric that is a "discretionary" measurement added by the font author and that is summed to the other two.

This seems to work consistently for the vast majority of fonts that I tested, except some of the system fonts, on which I suspect that Apple did something more "manual" and have some override tables that are not embedded in the font. For Helvetica, for example, the multipliers don't seem even to be consistent among font sizes, so it smells of manual tuning to me. This is incredibly undocumented, so that's a lot of guesswork from my side. Maybe there are some other metric tables embedded in the font that I couldn't access with FontForge and this could be solved deterministically, but I couldn't find any hint of such thing.

Sorry if it's a bit fuzzy, but I'm going from memory right now. I have the specs somewhere (because I had to write half of a typesetting system basically), but it's the work of almost a year ago and I don't have the links handy right now. I can find them again for you if you're interested though.

[mathieudutour]

But it might [complain about missing fonts] in this case, because the font name gets "resolved" to the wrong thing and committed to the .sketch file?

I'd say it's fine. The source is the react component and it's still ok. They just have to regenerate the sketch file on the new OS.

So we just need to fetch the image and base64-encode and it's not a requirement to re-encode it to PNG?

I think so. I can't remember exactly why it's re-encoded in png but it should be fine without re-encoding it.

I'm trying to "do it right" and contribute upstream

🎉

I started digging into the OpenType font tables

You are a lot more knowledgeable that I am then 😄
Happy to defer to your judgement, we just want to match as closely as possible what react native is doing, even if it means a breaking change in react-sketchapp (it's not a problem to publish a new version)

[lordofthelake]

I think so. I can't remember exactly why it's re-encoded in png but it should be fine without re-encoding it.

Alright. Then I'll go for a pure Node implementation. I have to write one anyway for the multiplatform version, so it might as well go back here as well. At this point wouldn't it make sense putting it back into react-sketchapp and leave only the parts that need native bindings here?

You are a lot more knowledgeable that I am then 😄

Not by choice 😅 I was not planning on becoming a typography expert, but then I had to figure out how to write a typesetter for this project and here we are 😅

Happy to defer to your judgement, we just want to match as closely as possible what react native is doing, even if it means a breaking change in react-sketchapp (it's not a problem to publish a new version)

Unluckily React Native typography is best described as a travesty, even compared to the browser, because it's a very thin layer on top of the OS rendering, and Android and iOS have quite different opinions in that regard, so aiming at consistency with RN is a bit of a moving target.

Probably leaving the ".AppleSystemFont" unresolved, and leaving the onus of using a "reset stylesheet" to the user if they want consistent rendering (like it's in browsers right now), seems like a better solution here? Is there any case where we have the necessity of resolving that font?

[mathieudutour]

At this point wouldn't it make sense putting it back into react-sketchapp and leave only the parts that need native bindings here?

Yep totally. I'd be nice to replace the sketch implementation as well but it might need to be a little be different than pure node (because http isn't available on sketch, we need to use fetch)

Is there any case where we have the necessity of resolving that font?

I think we need to when we have additional traits? Like bold or oblique?

[lordofthelake]

Yep totally. I'd be nice to replace the sketch implementation as well but it might need to be a little be different than pure node (because http isn't available on sketch, we need to use fetch)

We could use a fetch polyfill in node if we go for that. But then in Sketch we'll not have fs either right? So we would need the Sketch bindings to open a local file anyway.

Is there any case where we have the necessity of resolving that font?

I think we need to when we have additional traits? Like bold or oblique?

🤔I think this risks becoming even messier then.

Maybe an alternative approach could be then to use the system font for everything (missing, not specified, or explicitly set to "System") and explicitly saying "if you don't specify the font or the font is missing, it falls back to the system font, that is OS-dependent, and we make no assumptions about that"? This would at least be consistent with RN and the browsers. So it would be clear that the fallback is whatever the current system font is, and we don't get Helvetica in some cases and SF* in others.

[mathieudutour]

But then in Sketch we'll not have fs either right? So we would need the Sketch bindings to open a local file anyway.

You can use https://github.com/skpm/fs. We just need to be careful about how you require things so that they aren't required on the wrong platform (see https://github.com/airbnb/react-sketchapp/blob/master/src/jsonUtils/nodeImpl/requireSvgModel.ts)

if you don't specify the font or the font is missing, it falls back to the system font, that is OS-dependent, and we make no assumptions about that

I think that's sensible. I'm just not sure how RN is handling font weight with the system font.

[lordofthelake]

You can use https://github.com/skpm/fs. We just need to be careful about how you require things so that they aren't required on the wrong platform (see https://github.com/airbnb/react-sketchapp/blob/master/src/jsonUtils/nodeImpl/requireSvgModel.ts)

Alright, that sounds good. Then I will drop the makeImageDataFromURL bits from here and I'll move over to react-sketchapp to write an implementation there.

I think that's sensible. I'm just not sure how RN is handling font weight with the system font.

I don't do a lot of iOS, but my best guess there is that it works the same as macOS. For Android, afaik you just end up with different weights/styles of Roboto. I could do a quick POC with Expo and see what's the actual behavior.

[mathieudutour]

I don't do a lot of iOS, but my best guess there is that it works the same as macOS. For Android, afaik you just end up with different weights/styles of Roboto.

Right sorry I misunderstood. I thought you were suggesting to return .AppleSystemUIFont which would work when there isn't any trait but not when there are some.
Happy to just use the system font and tell people that it's system dependant (it's in the name after all)

[lordofthelake]

Ok, this all sounds good.

Then the plan here might be:

This PR:

• Drop all the tests that are failing

PR 1 (here):

1. I change the behavior here for the default font resolution so that it's fully platform-dependent and always falls back to the .SF family; document it, and rewrite the tests accordingly.
2. Document that the text boundaries without a user-set line-height are explicitly platform-dependent.

PR 2 (react-sketchapp):

• Apply the same changes for the font resolution

PR 3 (react-sketchapp):

• Write a pure implementation of makeImageDataFromURL

PR 4 (here):

• Drop everything around makeImageDataFromURL

Possibly PR 5 (on react-sketchapp):

• Give the possibility to the user of injecting their own bridge? It would solve in a clean way the multiplatform problem. For my needs it's not strictly necessary (I can use module aliasing if I need to), but I guess it would be a better general solution for everyone else?

And then release new breaking versions of everything?

[mathieudutour]

Give the possibility to the user of injecting their own bridge?

Tbh if node-sketch-bridge-multiplatform matches the behavior of node-sketch-bridge on macOS, I'm happy to drop node-sketch-bridge and just use node-sketch-bridge-multiplatform in react-sketchapp. And it wouldn't really be a bridge anymore 😄. It could potentially be merged with react-sketchapp directly (but I'd understand if you want to keep it a separate project).

The rest sounds like a good plan! Thanks for working on this!

[lordofthelake]

Tbh if node-sketch-bridge-multiplatform matches the behavior of node-sketch-bridge on macOS, I'm happy to drop node-sketch-bridge and just use node-sketch-bridge-multiplatform in react-sketchapp.

I'm not sure if I will be able to match 100% the behavior of node-sketch-bridge, especially for the areas that we are considering "undefined". By using the macOS version at least people would have the assurance that the rendering is consistent on their own machine regardless they do it through CLI or through a plugin.

My version comes with some more caveats. For sure the default line-height calculation will be different (I'll resort to something simpler using the OpenType metrics tables or just using a good ole' multiplier). And for the rest, it depends on how good my typesetter is. 😅

So I think it would be something good to be provided as an option, but I wouldn't make it the default, at least for macOS users (for others, maybe? On the principle that something is better than nothing?).

The rest sounds like a good plan! Thanks for working on this!

🙌

[lordofthelake]

I cleaned up the tests. Ready for another review!

[mathieudutour]

LGTM!

[dabbott]

Nice job getting this in guys! I'm not nearly as knowledgeable about Sketch/react-sketchapp behavior, but I can help with Obj-C issues if we run into any.