Add support for numeric font `weight` #6990

archmoj · 2024-05-03T21:55:26Z

This PR adds numeric weight options to various fonts.

Please note that scattergl traces has limited support of numeric values i.e. the values are mapped to either bold or normal.

TODO:

Opened Open Sans should have Extrabold instead of Extra Bold? openmaptiles/fonts#24 considering current bug with extra bold using Open Sans with mapbox
Add test and fall backs for text rendered into WebGL subplots e.g. 'mapbox'
Add extra tests for mapbox
Partial support in scattergl i.e. 400 --> 'normal', 700 --> 'bold'
Also opened Missing valid integers between 1 and 1000 jednano/css-font-weight-keywords#1
Support in gl3d
Support base64 font.weight in gl3d

@plotly/plotly_js
cc: @ndrezn

This reverts commit f4d4f4c.

emilykl · 2024-05-06T13:53:45Z

src/lib/coerce.js

@@ -114,8 +114,13 @@ exports.valObjectMeta = {
            'are coerced to the `dflt`.'
        ].join(' '),
        requiredOpts: [],
-        otherOpts: ['dflt', 'min', 'max', 'arrayOk'],
+        otherOpts: ['dflt', 'min', 'max', 'arrayOk', 'extras'],


@archmoj Is this adding a new functionality to coerce, where numeric (or other) properties can also be configured to accept a list of 'extra' values?

That's pretty cool -- is there anywhere we should document that internally?

Good question.
Yes that's true.
We have other types that support the extras option so I think it's clear internally.
But on plotly.py we need to see if this changes would be reflected properly by codegen.

Good point.

Makes me think, it would be awesome if we could build checking the Plotly.py codegen into the CI process. It would have to produce some kind of output that's easy to verify. It could run only if the schema.json has changed.

Perhaps add a line in the docstring here explaining extras for integers (like the explanation here for flaglist)

For flag lists combination mean something. That's why it is commented.

emilykl · 2024-05-06T14:34:31Z

test/image/baselines/zz-gl3d_font-weight-scatter.png

For this mock, maybe use a font that has more available weights, to test that they work properly in WebGL.

Actually I was interested to test it with the default font as that's what most users use at this moment.

…nd gl-text" This reverts commit abb3fc8.

stackgl_modules/package.json

archmoj · 2024-05-06T21:43:05Z

OK. This is ready for review.

src/lib/coerce.js

emilykl · 2024-05-08T18:54:34Z

src/traces/scattermapbox/constants.js

+
+// Must use one of the following fonts as the family, else default to 'Open Sans Regular'
+// See https://github.com/openmaptiles/fonts/blob/gh-pages/fontstacks.json
+var supportedFonts = [


@archmoj Does Mapbox make their list of supported fonts public anywhere as part of their package? Just wondering if there's a way we can ensure this list is always up-to-date without updating it manually.

No in fact we use latest v1 version of mapbox-gl. Due to the licence changes of their v2+ no significant v1 is expected. On the other hand we may switch to maplibre-gl-js which is the continuation of mapb0x-gl v1. See #5578.
BTW we could possibly improve our docs for these fonts in a separate PR.

OK got it. Do you know to what extent maplibre supports different font weights? Basically, will adding these options make switching to maplibre significantly more complicated?

I don't think so. It's a kind of problem we could address when/if we switch to it.

@emilykl , there is a migration branch in here maplibre where we should be able to try out:

Migrate to MapLibre #7015

emilykl · 2024-05-08T20:09:31Z

src/traces/scattermapbox/convert.js

+            else if(weight > 350) str += ' Regular';
+            else str += ' Light';
+        } else { // Other families
+            str = 'Klokantech Noto Sans';


Doesn't this else mean the font will default to Klokantech Noto Sans if it's not Metropolis or Open Sans?

oh I see, maybe not, because it has already passed through the supplyDefaults step -- can you confirm @archmoj ?

Good point. No that shouldn't happen because further down we test and ensure the font is supported.
I could restrict this else to make it read better?

Addressed in bff00ac.

src/traces/scattermapbox/defaults.js

emilykl · 2024-05-08T20:14:47Z

src/traces/scattermapbox/defaults.js

        handleTextDefaults(traceIn, traceOut, layout, coerce,
            {
                noSelect: true,
                noFontVariant: true,
                font: {
-                    family: supportedFonts.indexOf(layout.font.family) !== -1 ? layout.font.family : 'Open Sans Regular',
+                    family: isSupportedFont(layoutFontFamily) ? layoutFontFamily : 'Open Sans Regular',


ok, naive question because I don't quite understand -- if the user passes just Metropolis (for example) as the font family, won't that register as "not supported" because the string Metropolis is not in the supported fonts list, and so it will get replaced with Open Sans?

Now that we have weight and style options, would it make sense to change that behavior? Since a user could pass family: 'Metropolis', weight: 'bold' and we could use that to determine they want the font face Metropolis Bold.

Actually I was thinking about that and I'd be happy to work on it in a separate PR.
Obviously we don't want to break previous mocks.
Instead we could work on "adding support for minimal family names in mapbox".

ok, that's fine to do in another PR. Would be nice to have!

Tracked in #6993.

emilykl · 2024-05-08T21:11:46Z

src/traces/scattergl/convert.js

@@ -216,7 +216,7 @@ function convertTextStyle(gd, trace) {
        optsOut.font = {
            size: tfs * plotGlPixelRatio,
            family: tff,
-            weight: tfw,
+            weight: weightFallBack(tfw),


Why is this needed if we are already disallowing numeric weight values at the schema level?

Good question.
This is for scattergl with limited support of weight noting that for arrayOk attributes we do not check every element at the supply defaults step which could result in performance issues during interactions with the plot.
As a result this fall back is needed to map unsupported values in arrays to supported options.

gotcha, thank you. Could you perhaps add that as a comment above the function definition?

Addressed in 48d057f.

emilykl · 2024-05-08T21:23:52Z

src/traces/scattermapbox/convert.js

+            else if(weight > 250) str += ' Light';
+            else if(weight > 150) str += ' Extra Light';
+            else str += ' Thin';
+        } else if(parts.slice(0, 2).join(' ') === 'Open Sans') {


nit: Couldn't this be family.startsWith('Open Sans') ?

Sure. That may break IE11 which is fine as of today :)

emilykl · 2024-05-08T21:25:25Z

src/traces/scattergl/convert.js

@@ -225,6 +225,12 @@ function convertTextStyle(gd, trace) {
    return optsOut;
 }

+function weightFallBack(w) {
+    if(w <= 1000) {
+        return w > 500 ? 'bold' : 'normal';


Shouldn't the threshold be 400?

500 is Medium looks very similar to Regular.
IMHO it's debatable between 450/500 to 550.

Right, ok, I guess if the only options are normal or bold we have to draw the line somewhere. Maybe just add a comment explaining why 500 is chosen as the threshold.

emilykl · 2024-05-08T21:29:46Z

test/image/baselines/zz-gl2d_font-weight-scatter.png

Is it not possible to use numeric weights in gl2d?

Yes. For heatmapgl gl2d scene the numeric values work nicely similar to gl3d scenes and scatter3d.
Also for scattergl (since it uses a different pipeline) it works using the fallback which maps the values to bold and normal i.e. limited support.

emilykl · 2024-05-08T21:33:34Z

@archmoj This is looking really good. 🎉

Could you summarize which trace types have limited/no support for numeric font weights?

This still needs to be tested with the Plotly.py codegen, right?

archmoj · 2024-05-08T21:42:56Z

This still needs to be tested with the Plotly.py codegen, right?

Good call.
I could test that. In case of noticing a problem around integers we could switch to use enumerated values here.

archmoj · 2024-05-08T21:49:14Z

@archmoj This is looking really good. 🎉

Could you summarize which trace types have limited/no support for numeric font weights?

Good call. Added to the PR description:
scattergl traces has limited support of numeric values i.e. the values are mapped to either bold or normal.

archmoj added 6 commits May 3, 2024 16:55

Revert "no integer weight"

37dee48

This reverts commit f4d4f4c.

also drop font-weight 400 when exporting to SVG

3212866

test scatter font weight

448eb80

test bar font weight

3a3f458

use numeric font-weight in mocks

15b25a5

skip validating font-weight-bar mock

be24a14

archmoj added feature something new status: has TODOs labels May 3, 2024

archmoj added 2 commits May 6, 2024 09:11

draft log

d8424a3

numeric text weight in gl3d

353efc4

emilykl reviewed May 6, 2024

View reviewed changes

archmoj added 11 commits May 6, 2024 12:28

link to include extra valid font-weight options in css-font and gl-text

abb3fc8

test numeric font-weight values in scattergl pipeline

8266a6d

Revert "link to include extra valid font-weight options in css-font a…

46e6b27

…nd gl-text" This reverts commit abb3fc8.

fall back for unsupported font-weight values

3e4942a

scattergl numeric font-weight render using bold/normal fallback

72044b5

correct mock name

4d52885

improve scattergl test

b92ef23

test numeric weight in gl2d axis text

b125396

convert typed array spec in integer, number, color, etc

54005b9

handle typed arrays in scatter3d

09f4dd3

handle typed arrays in gl-axes3d

f67b40c

archmoj commented May 6, 2024

View reviewed changes

stackgl_modules/package.json Outdated Show resolved Hide resolved

archmoj commented May 6, 2024

View reviewed changes

stackgl_modules/package.json Outdated Show resolved Hide resolved

handle numeric font weight in mapbox supported fonts

99162e5

archmoj requested a review from emilykl May 6, 2024 22:27

fix toSVG using weight: 400

990fa8d

archmoj commented May 8, 2024

View reviewed changes

src/lib/coerce.js Show resolved Hide resolved

archmoj commented May 8, 2024

View reviewed changes

src/lib/coerce.js Show resolved Hide resolved

archmoj commented May 8, 2024

View reviewed changes

src/lib/coerce.js Show resolved Hide resolved

emilykl reviewed May 8, 2024

View reviewed changes

src/traces/scattermapbox/defaults.js Show resolved Hide resolved

emilykl reviewed May 8, 2024

View reviewed changes

improve font family checks

bff00ac

emilykl reviewed May 8, 2024

View reviewed changes

add comment

48d057f

emilykl approved these changes May 8, 2024

View reviewed changes

archmoj mentioned this pull request May 8, 2024

Improve scattermapbox.textfont family options and descriptions #6993

Closed

archmoj merged commit d135fff into master May 8, 2024
1 check passed

archmoj deleted the font-weight-integer branch May 8, 2024 22:59

archmoj mentioned this pull request May 8, 2024

Additional test for 6990, rename mocks and update baselines using new rendering order on CI #6994

Merged

FlorentinTh mentioned this pull request Jul 17, 2024

[Snyk] Upgrade plotly.js-cartesian-dist-min from 2.12.1 to 2.33.0 FlorentinTh/LE2ML-GUI#768

Open

GregBrimble mentioned this pull request Aug 4, 2024

[Snyk] Upgrade plotly.js from 1.54.1 to 2.33.0 GregBrimble/workers.sh#581

Open

FlorentinTh mentioned this pull request Aug 9, 2024

[Snyk] Upgrade plotly.js-cartesian-dist-min from 2.12.1 to 2.34.0 FlorentinTh/LE2ML-GUI#773

Open

guaiya0507 mentioned this pull request Aug 9, 2024

[Snyk] Upgrade plotly.js-dist-min from 2.33.0 to 2.34.0 guaiya0507/mytool#3

Open

This was referenced Sep 29, 2024

[Snyk] Upgrade plotly.js-cartesian-dist-min from 2.12.1 to 2.35.0 FlorentinTh/LE2ML-GUI#778

Open

[Snyk] Upgrade plotly.js-cartesian-dist-min from 2.12.1 to 2.35.1 FlorentinTh/LE2ML-GUI#781

Open

extrafu mentioned this pull request Oct 2, 2024

[Snyk] Upgrade plotly.js-dist-min from 2.32.0 to 2.35.2 inverse-inc/packetfence#8336

Merged

FlorentinTh mentioned this pull request Oct 2, 2024

[Snyk] Upgrade plotly.js-cartesian-dist-min from 2.12.1 to 2.35.2 FlorentinTh/LE2ML-GUI#782

Open

FlorentinTh mentioned this pull request Oct 22, 2024

[Snyk] Upgrade plotly.js-cartesian-dist-min from 2.12.1 to 2.35.2 FlorentinTh/LE2ML-GUI#785

Open

Add support for numeric font weight #6990

Add support for numeric font weight #6990

Conversation

archmoj commented May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archmoj commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilykl May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilykl commented May 8, 2024

archmoj commented May 8, 2024

archmoj commented May 8, 2024

Add support for numeric font `weight` #6990

Add support for numeric font `weight` #6990

archmoj commented May 3, 2024 •

edited

Loading

emilykl May 8, 2024 •

edited

Loading