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

Fix vertical offset due to bold/italics, and bad browser fonts. #1883

Merged
merged 2 commits into from Jun 10, 2012

Conversation

mcelrath
Copy link

@mcelrath mcelrath commented Jun 8, 2012

The identified bad mono fonts are "Courier New" from Microsoft's Core fonts:
http://en.wikipedia.org/wiki/Core_fonts_for_the_Web
and Nimbus Mono L
http://en.wikipedia.org/wiki/Nimbus_Mono_L

The bold/italic variants evidently don't have a consistent baseline. This is completely fixed by setting the vertical-align property on the span tag. (Yes, it has to be the span tag, putting it on the div or pre doesn't work)

This fixes #1503.

@certik
Copy link

certik commented Jun 8, 2012

Hi Bob, I have tested this pull request and it does fix the problem!! So I am +1.

We all owe you a beer for fixing this, thanks so much for your work!

@fperez
Copy link
Member

fperez commented Jun 9, 2012

Mmh, this is odd... I reverted to Courier New on Chrome 19 to try to see the problem (my monospaced font is normally Consolas, that doesn't have the bug). But it seems to me that this PR makes things worse, not better... With master, I get:

screenshot

But with this PR, I see this instead:

screenshot

It seems to me that with this PR, the bold words are clearly pixel off from the rest...

Am I misunderstanding something?

@mcelrath
Copy link
Author

mcelrath commented Jun 9, 2012

Well, poop. That didn't happen for me. You could try the other vertical-align
values to see if one fixes it for you. (middle?)

If it doesn't, we'll have to just remove the bold/italic.

Fernando Perez [reply@reply.github.com] wrote:

Mmh, this is odd... I reverted to Courier New on Chrome 19 to try to see the problem (my monospaced font is normally Consolas, that doesn't have the bug). But it seems to me that this PR makes things worse, not better... With master, I get:

screenshot

But with this PR, I see this instead:

screenshot

It seems to me that with this PR, the bold words are clearly pixel off from the rest...

Am I misunderstanding something?


Reply to this email directly or view it on GitHub:

#1883 (comment)

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

@fperez
Copy link
Member

fperez commented Jun 9, 2012

On Sat, Jun 9, 2012 at 9:23 AM, Bob McElrath
reply@reply.github.com
wrote:

Well, poop.  That didn't happen for me.  You could try the other vertical-align
values to see if one fixes it for you.  (middle?)

Nope, I see the same behavior with all, very strange. Linux 12.04, Chrome 19.

If it doesn't, we'll have to just remove the bold/italic.

You don't mean permanently disabling bold/italic from our syntax
highlighting altogether, right? Because that's absolutely not an
option.

It's very unfortunate that we're hit by this font/chrome bug, but if
there's no way to work around it, the solution would then be to
prominently post on the docs/site/wiki a notice along the lines of

"""
Unfortunately the default monospaced font on chrome, Courier New, has
a bug that makes it display text incorrectly when mixing bold/italics
with regular weights. Until Google fixes its mistake of using a
broken font by default, we suggest our users that they configure in
their chrome settings their monospaced font to be a different one.
The following fonts all work OK:

...

and as far as we know, the ones to avoid are:

....

"""

While we strive hard to provide good defaults out of the box for new
users, it's even more important that we don't prevent advanced users
to have a good experience.

The removal of bold/italics would degrade the editing experience
permanently for everyone in a non-fixable way, so it's simply not an
acceptable solution, I'm afraid.

@mcelrath
Copy link
Author

mcelrath commented Jun 9, 2012

Fernando Perez [reply@reply.github.com] wrote:

If it doesn't, we'll have to just remove the bold/italic.

You don't mean permanently disabling bold/italic from our syntax
highlighting altogether, right? Because that's absolutely not an
option.

I would hope having users run into this bug, and be unable to edit text in the
notebook is absolutely not an option.

It's very unfortunate that we're hit by this font/chrome bug, but if
there's no way to work around it, the solution would then be to
prominently post on the docs/site/wiki a notice along the lines of

As I mentioned before, I think it may be possible to detect this situation by
writing some characters to a hidden div, and measuring its offsetHeight.

I think no one would read a novel about browser bugs on the website.

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

@fperez
Copy link
Member

fperez commented Jun 9, 2012

On Sat, Jun 9, 2012 at 2:05 PM, Bob McElrath
reply@reply.github.com
wrote:

I would hope having users run into this bug, and be unable to edit text in the
notebook is absolutely not an option.

Oh, how does it prevent editing? What I see are ugly but purely
cosmetic issues, I didn't realize this was making cells non-editable
in some cases! Obviously that's even worse...

@mcelrath
Copy link
Author

mcelrath commented Jun 9, 2012

Fernando Perez [reply@reply.github.com] wrote:

On Sat, Jun 9, 2012 at 2:05 PM, Bob McElrath
reply@reply.github.com
wrote:

I would hope having users run into this bug, and be unable to edit text in the
notebook is absolutely not an option.

Oh, how does it prevent editing? What I see are ugly but purely
cosmetic issues, I didn't realize this was making cells non-editable
in some cases! Obviously that's even worse...

Once you have more than about 10 lines with some bold/italics, the cursor is
vertically offset, and appears to be on the next line down (or between lines).
So you can't position the cursor properly. It also shoves the last line(s) of
text in an input box off the bottom so that it's not visible (and therefore, not
editable either...)

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

@fperez
Copy link
Member

fperez commented Jun 9, 2012

On Sat, Jun 9, 2012 at 2:25 PM, Bob McElrath
reply@reply.github.com
wrote:

Once you have more than about 10 lines with some bold/italics, the cursor is
vertically offset, and appears to be on the next line down (or between lines).
So you can't position the cursor properly.  It also shoves the last line(s) of
text in an input box off the bottom so that it's not visible (and therefore, not
editable either...)

Ah, I see that now...

OK, that's indeed horrible and completely unacceptable, since it
simply makes the nb unusable.

I tested and with this PR, the slightly wonky looking pixel offset of
bold terms remains, but importantly, it doesn't build up like the
problem you explain above. So it's only a minor cosmetic issue that
doesn't break functionality.

In light of this discussion, my vote would be to merge this PR:

  • it has a tiny ugly bit but that is only cosmetic and due to a bug in
    the font itself
  • users can change their monospace font and that will do away with the
    cosmetic issue
  • it solves the broken functionality problem
  • it doesn't kneecap the editing area for advanced users.

Would you agree?

@mcelrath
Copy link
Author

I added to this branch a method to detect the bad font situation, and inform the user. With that, I'd be happy reverting the vertical-align:middle change I initially added.

Does vertical-align: middle cause any other problems (does it make text look funny with other fonts)? Of course the correct value is vertical-align: baseline for text, and I suspect we want it. I could also find a way to set middle when the bad font situation is detected.

@fperez
Copy link
Member

fperez commented Jun 10, 2012

No, I didn't see any other problems with Consolas one way or another.

@fperez
Copy link
Member

fperez commented Jun 10, 2012

OK, this is great, esp. with the new dialog warning. Awesome job, @mcelrath! I'm going to merge it now, it's seen enough review and it does address a pretty serious problem. Thanks!

fperez added a commit that referenced this pull request Jun 10, 2012
Fix vertical offset due to bold/italics, and bad browser fonts.

Adds a dialog warning users when a problematic monospaced font is detected, so they can change their configuration.
@fperez fperez merged commit 8325d15 into ipython:master Jun 10, 2012
@certik
Copy link

certik commented Jun 11, 2012

Hi, sorry that I didn't manage to reply over the weekend. Yes, previously the notebook was pretty much unusable in my Chrome, as Bob explained. This pull request makes it usable again.

However, the dialog is super annoying. I understand what the issue is and I don't mind one pixel discrepancy. I don't want to touch my Chrome setting, I am happy with the way it is. Right now every time I refresh a page or open any notebook a dialog pops up.

I wonder what other solutions could be applied. Maybe reporting the issue into Chrome bug tracker and then adding a non-intrusive note somewhere near the top of the page, saying "Note: your Chrome broswer suffers from the bug ##### (+link), the fonts will be one pixel off." And the user could click the link to learn more about the bug. And one could close this note, or just leave it be.

Bob --- do you think I should report this bug into the Chrome bug tracker?

@mcelrath
Copy link
Author

You really want to stay with that font? I was just too lazy to change it
because I wasn't using a lot of monospace. But, Courier New is pretty ugly...

Anyway, I was also thinking of silently applying the 'vertical-align: bottom'
css when the situation is detected, that can be done silently and transparently.

Ondřej Čertík [reply@reply.github.com] wrote:

Hi, sorry that I didn't manage to reply over the weekend. Yes, previously the notebook was pretty much unusable in my Chrome, as Bob explained. This pull request makes it usable again.

However, the dialog is super annoying. I understand what the issue is and I don't mind one pixel discrepancy. I don't want to touch my Chrome setting, I am happy with the way it is. Right now every time I refresh a page or open any notebook a dialog pops up.

I wonder what other solutions could be applied. Maybe reporting the issue into Chrome bug tracker and then adding a non-intrusive note somewhere near the top of the page, saying "Note: your Chrome broswer suffers from the bug ##### (+link), the fonts will be one pixel off." And the user could click the link to learn more about the bug. And one could close this note, or just leave it be.

Bob --- do you think I should report this bug into the Chrome bug tracker?


Reply to this email directly or view it on GitHub:

#1883 (comment)

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

@certik
Copy link

certik commented Jun 11, 2012

The dialog pretty much forces me to change the font. It's not such a big deal, so I will just change it and be done with it.

But in general I think that ipython should not force users to change their fonts.

@mcelrath
Copy link
Author

I just looked at the options for vertical-align, with Courier New, and they're
all bad. With middle or bottom the keywords are noticeably offset from the rest
of the text (because the proper way to align text is via the baseline).

The reason I made a dialog is because I was worried about having IPython look
crappy, and having it be very hard for the user to figure out why, or how to fix
it. (Which is what happens if I silently change some CSS) In fact I really
dislike the use of modal dialog boxes altogether (I would like it if IPython
removed all of them).

I was also worried about future people working on IPython having a very hard
time figuring out why the text alignment is behaving in a way inconsistent with
what is in the stylesheet. The "right" solution to this problem is to use
consistent fonts, either via font-family (which was voted down) or the user's
choice. Users may resist change, but this bug isn't our fault. ;)

So: I just pushed a patch which silently applies a fix, and adds a commented-out
block to notebook.css indicating that it may be applied (for future developers).
That tree is here:
https://github.com/mcelrath/ipython/tree/mono_cursor_offset

I'll let the lead developers decide which solution they like better.

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

@certik
Copy link

certik commented Jun 11, 2012

Bob, would you mind creating the bug in Chrome's bugtracker? You have much more knowledge about this than I do. From what you say, it seems to me, that this is indeed a clear bug in either the font or Chrome. Then we can maybe leave the modal dialog, but add there a link to this bug, so that users understand, that this is a bug in Chrome that needs to be fixed, that makes IPython look crappy.

@mcelrath
Copy link
Author

The bug is in the font. I reproduced it in Firefox too.

The fonts in question are 20-year old Microsoft fonts that probably, no one
should be using, and it appears that the browsers are actually using the fonts
correctly. The only reason I had them on my system was because Wine
(winetricks) installs them. I have no idea why Chrome chose them as default.

I think the bug should be considered in CodeMirror, for not measuring the height
of a line correctly when multiple bold/italic/normal fonts are on the line.
They do have a disclaimer in their docs about this situation.

Ondřej Čertík [reply@reply.github.com] wrote:

Bob, would you mind creating the bug in Chrome's bugtracker? You have much more knowledge about this than I do. From what you say, it seems to me, that this is indeed a clear bug in either the font or Chrome. Then we can maybe leave the modal dialog, but add there a link to this bug, so that users understand, that this is a bug in Chrome that needs to be fixed, that makes IPython look crappy.


Reply to this email directly or view it on GitHub:

#1883 (comment)

Cheers, Bob McElrath

"The individual has always had to struggle to keep from being overwhelmed by
the tribe. If you try it, you will be lonely often, and sometimes frightened.
But no price is too high to pay for the privilege of owning yourself."
-- Friedrich Nietzsche

@certik
Copy link

certik commented Jun 11, 2012

Ah, thanks for the clarification. So I can see now that if it is a bug in the font as well as it is some old font, the solution really is to change the font. So I did that.

I just assumed that I was using the Ubuntu default's settings and then no program should force me to change it just to work.

@fperez
Copy link
Member

fperez commented Jun 11, 2012

Still, given the experience Ondrej reports, I'm having second thoughts about the dialog too... I can imagine being in an environment where you can't change the settings (a lab computer at a school) and being driven absolutely bonkers by the dialog popping up on every file open. Should we reconsider that dialog?

@Carreau
Copy link
Member

Carreau commented Jun 22, 2012

@mcelrath

Playing with css I found that the #fonttest div is visible... (got a rogue x in my page) is it normal ? If it is necessary to still have it, could you make it the color of the background for it to disappear ?

Thanks.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Fix vertical offset due to bold/italics, and bad browser fonts.

Adds a dialog warning users when a problematic monospaced font is detected, so they can change their configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor is offset in notebook in Chrome 17 on Linux
4 participants