Skip to content
This repository has been archived by the owner on Jan 15, 2019. It is now read-only.

Add a font family overwrite for nonbreaking spaces #463

Merged
merged 7 commits into from
Nov 9, 2018

Conversation

kjellr
Copy link
Collaborator

@kjellr kjellr commented Nov 2, 2018

Attempts to address #187: A weird bug in Chrome where Hoefler text &nbsp characters are rendered as wider than a standard space in the editor.

Props to @iseulde 🙌

Attempts to address a weird bug in Chrome where Hoefler text `&nbsp` characters are rendered as wider than a standard space.
@kjellr kjellr added the bug Something isn't working label Nov 2, 2018
@kjellr kjellr requested a review from jasmussen November 2, 2018 13:44
@jasmussen
Copy link
Collaborator

Holy guacamole, this hack is amazing! I'm mobile at the moment but this is an amazing amazing trick and I can't see why it shouldn't work. (Only thing would be to check that the width of this nbsp character set in Georgia is close enough in width to Hoefler that it looks the same)

But wow I just want to give a silent emoji round of applause for this code, it's delicious! 👏👏

@ellatrix
Copy link
Member

ellatrix commented Nov 2, 2018

One thing to be aware of: any non breaking space insertions that get saved accidentally, or any intentional insertions by the user, will also render too wide on the front-end, so this fix might be needed there too.

@kjellr
Copy link
Collaborator Author

kjellr commented Nov 2, 2018

Only thing would be to check that the width of this nbsp character set in Georgia is close enough in width to Hoefler that it looks the same

According to my test here, both render at ~15px wide (using our default font size), so I think we're good there: https://codepen.io/kjellr/pen/qQWgNp

Our fallback fonts render slightly slimmer spaces (a couple pixels), so this may cause some very slight issues there. I didn't really notice while testing. However, I'll wait for someone else to confirm too though.

One thing to be aware of: any non breaking space insertions that get saved accidentally, or any intentional insertions by the user, will also render too wide on the front-end, so this fix might be needed there too.

That's a good point. Spaces in the front-end will likely be less noticeable (the only reason this was noticed in the editor as because of the cursor jumping while typing. 😄). But once we're sure this definitely works for the editor, we can look at rolling it out there too.

@jasmussen
Copy link
Collaborator

To clarify, I don't feel the width has to be perfect, if it's anything like I think it is (still mobile) then it's a substantial improvement.

The bar to pass is to ensure the writing flow feels right and uninterrupted.

Copy link
Collaborator

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a vast improvement. We should absolutely ship this.

Quick one: maybe it should be called NonBreakingSpaceOverride (override instead of overwrite)?

Also, I need a sanity check on this one. Does the height of the caret change ever so slightly on the space, or am I seeing things? To test, write a sentence, then wait with the caret at the end of the last letter, wait until it's visible, then press space and see if the height changes. It appears as if it becomes 1px taller. If it is, it's fine/acceptable but worth looking at a Georgia alternative that better mimics the height, separately.

But overall, this seems like a superbly clever hack that fixes the issue! Ship it!

@kjellr
Copy link
Collaborator Author

kjellr commented Nov 2, 2018

uick one: maybe it should be called NonBreakingSpaceOverride (override instead of overwrite)?

Good suggestion. Updated. 👍

Also, I need a sanity check on this one. Does the height of the caret change ever so slightly on the space, or am I seeing things? To test, write a sentence, then wait with the caret at the end of the last letter, wait until it's visible, then press space and see if the height changes. It appears as if it becomes 1px taller. If it is, it's fine/acceptable but worth looking at a Georgia alternative that better mimics the height, separately.

I added some guides to my screen to check this, and it doesn't appear to jump for me at least:

typing

In general, I tested this PR across a few different browsers in Windows, Mac, and iOS. Seems fine in each one. The only small issue I noticed is that while typing using Hoefler Text, descenders get cut off a little bit:

screen shot 2018-11-02 at 4 03 46 pm

This is resolved once you click out of the field, and is probably related to the line-height. I'll take a closer look on Monday and see if I can sort it out before merging this. 👍

@allancole
Copy link
Collaborator

Wow. Such a clever fix!!! 👍

@allancole allancole added this to the RC1 milestone Nov 2, 2018
@kjellr
Copy link
Collaborator Author

kjellr commented Nov 6, 2018

The only small issue I noticed is that while typing using Hoefler Text, descenders get cut off a little bit:

This is resolved once you click out of the field, and is probably related to the line-height. I'll take a closer look on Monday and see if I can sort it out before merging this. 👍

I don't think it's a line height issue after all: the line height hasn't actually changed — it's 1.8 by default here, whether or not we're defining it where we are in this PR. Commenting our our line height change doesn't have any effect on the issue.

Instead, my hypothesis is that this has something to do with the fact that Hoefler Text's descenders dip below those of Georgia:

screen shot 2018-11-05 at 8 30 45 pm

I checked some other potential font options (Times New Roman, Baskerville, etc.), but they all have the same issue — and their descenders all don't dip below the level that Hoefler Text's do. 😞

In general, still need to dig a little to figure that out... I'm going to focus on other work for a bit, but hope to circle back to this soon.

@jasmussen
Copy link
Collaborator

I think it's an acceptable trade off, but would definitely be nice if we could find a font alternative that had sufficiently long descenders.

Plan b we could look at a chrome only hack.

@ellatrix
Copy link
Member

ellatrix commented Nov 6, 2018

One idea to fix the descenders: create a font with just one character for the non breaking space, either taken from Hoefler directly (just take the normal space) or something handcrafted, then base64 encode it (or not) and add it as the source instead of Georgia. I think this is possible. All depends on how long you want to work on this fix of course. Thinking if there might be other solutions...

@ellatrix
Copy link
Member

ellatrix commented Nov 6, 2018

I don't see this issue in my example btw:

screen shot 2018-11-06 at 09 48 00

https://codepen.io/iseulde/pen/qQWJRo

@jasmussen
Copy link
Collaborator

I think it might be good to ship this as is and then look at additional improvements separately? If need be, I have a license to a font building tool, and can probably create a Hoefler/Georgia hybrid that only contains a space.

@kjellr
Copy link
Collaborator Author

kjellr commented Nov 6, 2018

I don't see this issue in my example btw:

Weird — I do see it in Safari 12.0, but it's only noticeable after bumping up the font size a bit, and only when you're typing (not on the text that's there originally):

https://codepen.io/kjellr/pen/pQjjQR

I think it might be good to ship this as is and then look at additional improvements separately?

My only hesitation is that while this fixes an issue in a single browser (Chrome), on a single operating system (Mac)... it also introduces this clipped text issue everywhere 😕. I'm not 100% sure that's a great tradeoff yet.

@jasmussen
Copy link
Collaborator

Oh I see it with the j's now. Taking a look.

@jasmussen
Copy link
Collaborator

webfontkit-20181107-022225.zip

☝️ This is a version of the Hoefler font, with the Georgia space character transplanted, and nothing else. Basically Ella's suggestion here #463 (comment)

Can anyone try and see if this fixes the issue?

@kjellr
Copy link
Collaborator Author

kjellr commented Nov 7, 2018

Can anyone try and see if this fixes the issue?

Thanks! I pushed a commit that includes this fix. Seems to work during my testing. 👍 Could use another set of eyes to double check though.

@ellatrix
Copy link
Member

ellatrix commented Nov 7, 2018

☝️ This is a version of the Hoefler font, with the Georgia space character transplanted, and nothing else. Basically Ella's suggestion here #463 (comment)

You could also transplant Hoefler's own space into the non breaking space's spot, which may be even cleaner. I wouldn't affect the space's ability to not break the line, the font is purely for rendering the character itself.

@jasmussen
Copy link
Collaborator

You could also transplant Hoefler's own space into the non breaking space's spot, which may be even cleaner. I wouldn't affect the space's ability to not break the line, the font is purely for rendering the character itself.

But isn't the non breaking space spot the same as the space spot? This is getting into advanced territory for me.

I think the following zip has a base64 encoded version, btw:

webfontkit-20181107-131226.zip

To save us an extra request.
@kjellr
Copy link
Collaborator Author

kjellr commented Nov 7, 2018

Added the base64 encoded version. Seems to work great. 👍 Happy to go ahead and merge, but I'll wait to get clarification on this part above:

You could also transplant Hoefler's own space into the non breaking space's spot, which may be even cleaner. I wouldn't affect the space's ability to not break the line, the font is purely for rendering the character itself.

But isn't the non breaking space spot the same as the space spot? This is getting into advanced territory for me.

@jasmussen
Copy link
Collaborator

We should get this in as soon as possible, the current behavior in master is highly disruptive to the writing flow. Just tested this again and I think it's good to ship and iterate on.

@allancole
Copy link
Collaborator

Yup, this seems to do the trick for me 👍. Gonna go ahead and merge this…

You could also transplant Hoefler's own space into the non breaking space's spot, which may be even cleaner. I wouldn't affect the space's ability to not break the line, the font is purely for rendering the character itself.

@iseulde Happy to iterate on this with a new PR—especially if its cleaner :-)

@allancole allancole merged commit 5cb5531 into master Nov 9, 2018
@kjellr kjellr deleted the fix/editor-empty-space-issue branch November 9, 2018 17:37
@afercia
Copy link

afercia commented Nov 22, 2018

I think this needs to be done also for the frontend. Have you seen @swissspidy 's last post on his personal blog? Plenty of  s there produced by Gutenberg in some way, and plenty of visible, large spaces.

@jasmussen
Copy link
Collaborator

@afercia See #585

@afercia
Copy link

afercia commented Nov 22, 2018

Ah great thanks.

@afercia
Copy link

afercia commented Nov 22, 2018

@jasmussen seems it was reverted? 14d240a

I see those spaces in the front-end both in Chrome and Firefox.

@jasmussen
Copy link
Collaborator

@allancole can you elaborate on the revert?

@kjellr
Copy link
Collaborator Author

kjellr commented Nov 25, 2018

When Allan made that revert, I'm not sure he realized this had been added to the front-end intentionally. We should re-add.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working has patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants