phetsims/collision-lab

ElapsedTimeNumberDisplay should use a font with monospaced numerals

Closed this issue · 9 comments

samreid commented

From phetsims/scenery-phet#616, @ariel-phet pointed out that /collision-lab/js/common/view/ElapsedTimeNumberDisplay.js sometimes shows a jitter. We are considering other fonts for that issue and, once decided, it seems like Collision Lab should use the same font. Perhaps we will factor it out to a constant like "NUMBER_FONT".

ariel-phet commented

@brandonLi8 this would be a nice polish to avoid jitter in the ElapsedTimeNumberDisplay

samreid commented

We went with const numberFontFamily = 'font-family:Trebuchet MS,Lucida Grande,monospace;'; Feel free to promote it to a static attribute of StopwatchNode (or some other constant in scenery-phet) if you want to reuse it.

brandonLi8 commented

I went ahead and promoted numberFontFamily to a static attribute and used it in ElapseTimeNumberDisplay.

To my eye, it looks like there is still jitter but I'm not sure. Assigning @ariel-phet for review in master.

Also assigning back to @samreid for review of implementation.

ariel-phet commented

No jitter on my browsers (Chrome and FF on Windows) and I can tell it is rendering Trebuchet MS. Looks good.

samreid commented

According to the documentation in Font.js, the multi-word font family names should be surrounded in quotes, like so:

  var font = new scenery.Font( {
    family: '"Times New Roman", serif',
    style: 'italic',
    lineHeight: 10
  } );

And from the documentation:

    // {string} - A comma-separated list of families, which can include generic families (preferably at the end) such
    // as 'serif', 'sans-serif', 'cursive', 'fantasy' and 'monospace'. If there is any question about escaping (such
    // as spaces in a font name), the family should be surrounded by double quotes.

According to https://stackoverflow.com/questions/13751412/why-would-font-names-need-quotes it is safe, and even recommended to include the quotes in the css font-family specification, so perhaps this can be just changed once in StopwatchNode.NUMBER_FONT_FAMILY. When changing this, please test StopwatchNode using the scenery-phet demo harness to make sure it works properly.

Everything else looks great, back to you @brandonLi8

brandonLi8 commented

@samreid,

I tried

StopwatchNode.NUMBER_FONT_FAMILY = '"Trebuchet MS", "Lucida Grande", monospace';

but the scenery-phet demo wasn't working properly (ie it looked like
image

I then tried

StopwatchNode.NUMBER_FONT_FAMILY = 'Trebuchet MS, "Lucida Grande", monospace';

and the scenery-phet demo was working correctly.

So it looks like only Lucida Grande allows quotes. @samreid How should I proceed? Either leave it as-is with no quotes on all of them or only quote Lucida Grande?

samreid commented

@jonathanolson can you please recommend how to proceed for this issue?

jonathanolson commented

Well, it looks like it was being provided to RichText, and the family was being inserted into a double-quoted string in the HTML:

"<span style="font-size: 20px; font-family:"Trebuchet MS", "Lucida Grande", monospace;">00:00</span><span style="font-size: 14px;font-family:"Trebuchet MS", "Lucida Grande", monospace;">.00</span>"

The proper way would be to use escapeHTML.js to encode those double-quotes to &quot;, however himalaya... doesn't like parsing that. So instead, it works in this case to single-quote the style attribute, so we can safely use double-quotes inside:

"<span style='font-size: 20px; font-family:"Trebuchet MS", "Lucida Grande", monospace;'>00:00</span><span style='font-size: 14px;font-family:"Trebuchet MS", "Lucida Grande", monospace;'>.00</span>"

Committed above. @samreid can you briefly check/review?

samreid commented

I inspected the commits, and tested in the scenery-phet harness (where I identified the new issue linked above), and in Gravity and Orbits. Looks great, thanks! Closing.