phetsims / friction

"Friction" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/friction
GNU General Public License v3.0
4 stars 6 forks source link

book title text too large with locale=kn #330

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air (m1 chip)

Operating System 13.1

Browser chrome

Problem description For https://github.com/phetsims/qa/issues/886, with locale=kn the textbook titles dip below the books. Other locales reach the very bottom of the book, which I think is OK, but if not let me know and I will include that list.

Visuals

Screenshot 2023-01-27 at 3 26 48 PM

ex of text reaching bottom of book (locale=km)

Screenshot 2023-01-27 at 3 36 34 PM
Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Friction‬ URL: https://phet-dev.colorado.edu/html/friction/1.6.0-rc.1/phet/friction_all_phet.html Version: 1.6.0-rc.1 2023-01-17 16:32:22 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36 Language: en-US Window: 1536x781 Pixel Ratio: 1.7999999523162842/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
zepumph commented 1 year ago

@jonathanolson spoke about how used to be solved with a very slow algorithm, but the browser may have caught up with the API we want over in https://github.com/phetsims/scenery/issues/16, and so I'll see if something good comes out of that.

jonathanolson commented 1 year ago

This might take 20 minutes or so to get working with the new APIs (but longer to test edge cases and really vet it). It also has wider (helpful) implications if this is implemented (Canvas optimizations are mostly not doable because we can't get accurate drawable bounds)

zepumph commented 1 year ago

@Nancy-Salpepi can you please see if this is better on master? I could not reproduce the problem on iPad safari/chrome or on my windows machine, so I defer to your testing (thank you and sorry).

Nancy-Salpepi commented 1 year ago

Things look good with mac + chrome now.....but not with safari. I think it is partly because of https://github.com/phetsims/scenery/issues/1458, but this is what locale=kn looks like now with safari:

Screenshot 2023-01-31 at 11 56 29 AM

and this is how it looks on chrome now:

Screenshot 2023-01-31 at 12 14 12 PM
zepumph commented 1 year ago

Great thanks! And here I actually can reproduce this problem on my iPad, I'll give it a whirl.

Nancy-Salpepi commented 1 year ago

That was my fault. The original issue was only in mac + chrome. Not with safari.

samreid commented 1 year ago

Scenery helper shows this in Chrome:

image

And this in Safari:

image

So we are seeing two problems:

  1. The glyphs are incorrect in Safari (see also the nav bar)
  2. The bounds for the bad glyphs are also incorrect.
image
Nancy-Salpepi commented 1 year ago

@samreid problem one is documented in https://github.com/phetsims/scenery/issues/1458

samreid commented 1 year ago

@zepumph and I downloaded another kn font and saw that it looked different but still had the same bounds problem in Safari:

image
samreid commented 1 year ago

We found that this patch looks great in safari:

image
```diff Subject: [PATCH] Remove useless cancelOther: false (after priority was increased) see https://github.com/phetsims/friction/issues/325 --- Index: main/scenery-phet/js/PhetFont.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/scenery-phet/js/PhetFont.ts b/main/scenery-phet/js/PhetFont.ts --- a/main/scenery-phet/js/PhetFont.ts (revision 65d9d43bf300c810f4a3c5c35ebe3da0d9b76647) +++ b/main/scenery-phet/js/PhetFont.ts (date 1675287249875) @@ -40,7 +40,7 @@ }, options ); // Guarantee a fallback family - assert && assert( options.family ); + // assert && assert( options.family ); options.family = `${options.family}, sans-serif`; super( options ); Index: main/scenery/js/util/Font.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/scenery/js/util/Font.ts b/main/scenery/js/util/Font.ts --- a/main/scenery/js/util/Font.ts (revision b7829bd583a318ae60509cff867f0c1b1efbfee8) +++ b/main/scenery/js/util/Font.ts (date 1675287457188) @@ -154,7 +154,7 @@ this._stretch = options.stretch; this._size = Font.castSize( options.size ); this._lineHeight = options.lineHeight; - this._family = options.family; + this._family = "Kannada Sangam MN"; // sanity checks to prevent errors in interpretation or in the font shorthand usage assert && assert( typeof this._style === 'string' && _.includes( VALID_STYLES, this._style ), Index: main/friction/js/friction/view/book/CoverNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/friction/js/friction/view/book/CoverNode.js b/main/friction/js/friction/view/book/CoverNode.js --- a/main/friction/js/friction/view/book/CoverNode.js (revision d8bf24a567b78b83883abbe999c151670e6a5794) +++ b/main/friction/js/friction/view/book/CoverNode.js (date 1675287312310) @@ -16,7 +16,10 @@ import FrictionConstants from '../../FrictionConstants.js'; // constants -const FONT = new PhetFont( 22 ); +const FONT = new PhetFont( { + size: 22, + family: '"Noto Sans"' +} ); const BINDING_LENGTH = 200; // dimension of the binding inline with the text of the book. const BINDING_WIDTH = 30; // thickness of the book const ROUND = 5; ```

My expectation is that people using the kn locale hopefully have a font that renders the glyphs correctly. So I think this is mainly a problem for people who don't use the kn locale frequently. It's nice to see that some fonts do not have this problem. So perhaps this should not be a blocking problem at all. To follow up at lower priority, we could reach out to our kn trusted translator and ask about this issue?

zepumph commented 1 year ago

I think we can move forward with cherry picking whatever comes out of https://github.com/phetsims/scenery/issues/16 as well as the commits in this repo.

jessegreenberg commented 1 year ago

I am not entirely sure which commits to cherry pick for this, and might need some help. I believe they are:

friction: https://github.com/phetsims/friction/commit/c687a8879a186cc7b63ed300ff0089195390c43c https://github.com/phetsims/friction/commit/9b48e0c064274739aff395cf90ffc5f0d272ae3a

scenery: https://github.com/phetsims/scenery/commit/9b39b08e639262fbdea6e952b681b848b3cf434f https://github.com/phetsims/scenery/commit/e11197c55b486c190e4dc790410f2f56f711a7df

But there is also a problem in https://github.com/phetsims/friction/issues/330#issuecomment-1412775307 that seems to still be in a patch?

samreid commented 1 year ago

Here is what it currently looks like on my Safari in master:

image

Here is what it looked like in RC.1

image
jessegreenberg commented 1 year ago

@samreid and I met to discuss the changes and agreed that it is best to move forward with the next RC without cherry-picking. We came to this conclusion because 1) The change in https://github.com/phetsims/scenery/commit/9b39b08e639262fbdea6e952b681b848b3cf434f uses a new web API. I am worried about platform specific problems and the "edge cases" mentioned in https://github.com/phetsims/friction/issues/330#issuecomment-1409417465. 2) The problem in https://github.com/phetsims/friction/issues/330#issue-1560402646 is minor, not worth time and destabilizing release branches. 3) This problem doesn't happen for me on Windows in 1.6-rc.1 or master. 4) The formatting in Safari actually seems worse in master compared to the release branch (see https://github.com/phetsims/friction/issues/330#issuecomment-1421574752) 5) @samreid found that the formatting was better with other kn fonts, so people actually using this language would presumably have a better font on their machines.

I'm going to close this issue, @Nancy-Salpepi just FYI about this decision for friction-1.6. But master is still improved since you pointed this out!