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.

Sign up for GitHub

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

Jump to bottom

Add support for numeric font weight #6990

Merged
merged 33 commits into from
May 8, 2024
Merged

Add support for numeric font weight #6990

Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
37dee48
Revert "no integer weight"
archmoj May 3, 2024
3212866
also drop font-weight 400 when exporting to SVG
archmoj May 3, 2024
448eb80
test scatter font weight
archmoj May 3, 2024
3a3f458
test bar font weight
archmoj May 3, 2024
15b25a5
use numeric font-weight in mocks
archmoj May 3, 2024
be24a14
skip validating font-weight-bar mock
archmoj May 3, 2024
d8424a3
draft log
archmoj May 6, 2024
353efc4
numeric text weight in gl3d
archmoj May 6, 2024
abb3fc8
link to include extra valid font-weight options in css-font and gl-text
archmoj May 6, 2024
8266a6d
test numeric font-weight values in scattergl pipeline
archmoj May 6, 2024
46e6b27
Revert "link to include extra valid font-weight options in css-font a…
archmoj May 6, 2024
3e4942a
fall back for unsupported font-weight values
archmoj May 6, 2024
72044b5
scattergl numeric font-weight render using bold/normal fallback
archmoj May 6, 2024
4d52885
correct mock name
archmoj May 6, 2024
b92ef23
improve scattergl test
archmoj May 6, 2024
b125396
test numeric weight in gl2d axis text
archmoj May 6, 2024
54005b9
convert typed array spec in integer, number, color, etc
archmoj May 6, 2024
09f4dd3
handle typed arrays in scatter3d
archmoj May 6, 2024
f67b40c
handle typed arrays in gl-axes3d
archmoj May 6, 2024
99162e5
handle numeric font weight in mapbox supported fonts
archmoj May 6, 2024
990fa8d
fix toSVG using weight: 400
archmoj May 6, 2024
f3c0356
improve mapbox text weight and italic handling
archmoj May 6, 2024
a5cc7f8
fix support of Open Sans Extrabold fonts
archmoj May 7, 2024
63824c1
test metropolis fonts on mapbox
archmoj May 7, 2024
190aef1
test italic Metropolis fonts
archmoj May 7, 2024
091e7d3
add weight test for open sans fonts - duplicate simple open-sans base…
archmoj May 7, 2024
82de3ff
add weight test for metropolis fonts - duplicate simple metropolis ba…
archmoj May 7, 2024
59779f3
Revert "test italic Metropolis fonts"
archmoj May 7, 2024
f6fcbd7
test numeric weights on axes 3d
archmoj May 8, 2024
10f477f
bump gl-axes3d and gl-scatter3d
archmoj May 8, 2024
82863fd
refactor src/traces/scattergl/convert.js
archmoj May 8, 2024
bff00ac
improve font family checks
archmoj May 8, 2024
48d057f
add comment
archmoj May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions draftlogs/6990_add.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Add support for numeric text font `weight` [[#6990](https://github.com/plotly/plotly.js/pull/6990)]
15 changes: 14 additions & 1 deletion src/lib/coerce.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ exports.valObjectMeta = {
requiredOpts: [],
otherOpts: ['dflt', 'min', 'max', 'arrayOk'],
coerceFunction: function(v, propOut, dflt, opts) {
if(isTypedArraySpec(v)) v = decodeTypedArraySpec(v);
archmoj marked this conversation as resolved.
Show resolved Hide resolved

if(!isNumeric(v) ||
(opts.min !== undefined && v < opts.min) ||
(opts.max !== undefined && v > opts.max)) {
Expand All @@ -114,8 +116,15 @@ exports.valObjectMeta = {
'are coerced to the `dflt`.'
].join(' '),
requiredOpts: [],
otherOpts: ['dflt', 'min', 'max', 'arrayOk'],
otherOpts: ['dflt', 'min', 'max', 'arrayOk', 'extras'],
Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

coerceFunction: function(v, propOut, dflt, opts) {
if((opts.extras || []).indexOf(v) !== -1) {
propOut.set(v);
return;
}

if(isTypedArraySpec(v)) v = decodeTypedArraySpec(v);
archmoj marked this conversation as resolved.
Show resolved Hide resolved

if(v % 1 || !isNumeric(v) ||
(opts.min !== undefined && v < opts.min) ||
(opts.max !== undefined && v > opts.max)) {
Expand Down Expand Up @@ -156,6 +165,8 @@ exports.valObjectMeta = {
requiredOpts: [],
otherOpts: ['dflt', 'arrayOk'],
coerceFunction: function(v, propOut, dflt) {
if(isTypedArraySpec(v)) v = decodeTypedArraySpec(v);
archmoj marked this conversation as resolved.
Show resolved Hide resolved

if(tinycolor(v).isValid()) propOut.set(v);
else propOut.set(dflt);
}
Expand Down Expand Up @@ -198,6 +209,8 @@ exports.valObjectMeta = {
requiredOpts: [],
otherOpts: ['dflt', 'arrayOk'],
coerceFunction: function(v, propOut, dflt) {
if(isTypedArraySpec(v)) v = decodeTypedArraySpec(v);
archmoj marked this conversation as resolved.
Show resolved Hide resolved

if(v === 'auto') propOut.set('auto');
else if(!isNumeric(v)) propOut.set(dflt);
else propOut.set(modHalf(+v, 360));
Expand Down
31 changes: 22 additions & 9 deletions src/plots/font_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,27 @@ module.exports = function(opts) {
var editType = opts.editType;
var colorEditType = opts.colorEditType;
if(colorEditType === undefined) colorEditType = editType;

var weight = {
editType: editType,
valType: 'integer',
min: 1,
max: 1000,
extras: ['normal', 'bold'],
dflt: 'normal',
description: [
'Sets the weight (or boldness) of the font.'
].join(' ')
};

if(opts.noNumericWeightValues) {
weight.valType = 'enumerated';
weight.values = weight.extras;
weight.extras = undefined;
weight.min = undefined;
weight.max = undefined;
}

var attrs = {
family: {
valType: 'string',
Expand Down Expand Up @@ -49,15 +70,7 @@ module.exports = function(opts) {
editType: colorEditType
},

weight: {
editType: editType,
valType: 'enumerated',
values: ['normal', 'bold'],
dflt: 'normal',
description: [
'Sets the weight (or boldness) of the font.'
].join(' ')
},
weight: weight,

style: {
editType: editType,
Expand Down
2 changes: 1 addition & 1 deletion src/snapshot/tosvg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ module.exports = function toSVG(gd, format, scale) {

// Drop normal font-weight, font-style and font-variant to reduce the size
var fw = this.style.fontWeight;
if(fw && fw === 'normal') {
if(fw && (fw === 'normal' || fw === '400')) { // font-weight 400 is similar to normal
txt.style('font-weight', undefined);
}
var fs = this.style.fontStyle;
Expand Down
1 change: 1 addition & 0 deletions src/traces/scattergl/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ var attrs = module.exports = overrideAll({
editType: 'calc',
colorEditType: 'style',
arrayOk: true,
noNumericWeightValues: true,
variantValues: ['normal', 'small-caps'],
description: 'Sets the text font.'
}),
Expand Down
12 changes: 9 additions & 3 deletions src/traces/scattergl/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ function convertTextStyle(gd, trace) {
if(
isArrayOrTypedArray(tfs) ||
Array.isArray(tff) ||
Array.isArray(tfw) ||
isArrayOrTypedArray(tfw) ||
Array.isArray(tfy) ||
Array.isArray(tfv)
) {
Expand All @@ -207,7 +207,7 @@ function convertTextStyle(gd, trace) {
) * plotGlPixelRatio;

fonti.family = Array.isArray(tff) ? tff[i] : tff;
fonti.weight = Array.isArray(tfw) ? tfw[i] : tfw;
fonti.weight = weightFallBack(isArrayOrTypedArray(tfw) ? tfw[i] : tfw);
fonti.style = Array.isArray(tfy) ? tfy[i] : tfy;
fonti.variant = Array.isArray(tfv) ? tfv[i] : tfv;
}
Expand All @@ -216,7 +216,7 @@ function convertTextStyle(gd, trace) {
optsOut.font = {
size: tfs * plotGlPixelRatio,
family: tff,
weight: tfw,
weight: weightFallBack(tfw),
Copy link
Contributor

@emilykl emilykl May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 48d057f.

style: tfy,
variant: tfv
};
Expand All @@ -225,6 +225,12 @@ function convertTextStyle(gd, trace) {
return optsOut;
}

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

Choose a reason for hiding this comment

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

Shouldn't the threshold be 400?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
return w;
}

function convertMarkerStyle(gd, trace) {
var count = trace._length;
Expand Down
45 changes: 45 additions & 0 deletions src/traces/scattermapbox/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';

// 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 = [
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

'Metropolis Black Italic',
'Metropolis Black',
'Metropolis Bold Italic',
'Metropolis Bold',
'Metropolis Extra Bold Italic',
'Metropolis Extra Bold',
'Metropolis Extra Light Italic',
'Metropolis Extra Light',
'Metropolis Light Italic',
'Metropolis Light',
'Metropolis Medium Italic',
'Metropolis Medium',
'Metropolis Regular Italic',
'Metropolis Regular',
'Metropolis Semi Bold Italic',
'Metropolis Semi Bold',
'Metropolis Thin Italic',
'Metropolis Thin',
'Open Sans Bold Italic',
'Open Sans Bold',
'Open Sans Extrabold Italic',
'Open Sans Extrabold',
'Open Sans Italic',
'Open Sans Light Italic',
'Open Sans Light',
'Open Sans Regular',
'Open Sans Semibold Italic',
'Open Sans Semibold',
'Klokantech Noto Sans Bold',
'Klokantech Noto Sans CJK Bold',
'Klokantech Noto Sans CJK Regular',
'Klokantech Noto Sans Italic',
'Klokantech Noto Sans Regular'
];

module.exports = {
isSupportedFont: function(a) {
return supportedFonts.indexOf(a) !== -1;
}
};
60 changes: 54 additions & 6 deletions src/traces/scattermapbox/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ var Colorscale = require('../../components/colorscale');
var Drawing = require('../../components/drawing');
var makeBubbleSizeFn = require('../scatter/make_bubble_size_func');
var subTypes = require('../scatter/subtypes');
var isSupportedFont = require('./constants').isSupportedFont;
var convertTextOpts = require('../../plots/mapbox/convert_text_opts');
var appendArrayPointValue = require('../../components/fx/helpers').appendArrayPointValue;

Expand Down Expand Up @@ -369,11 +370,58 @@ function arrayifyAttribute(values, step) {

function getTextFont(trace) {
var font = trace.textfont;
var str = '';
if(font.weight === 'bold') str += ' Bold';
if(font.style === 'italic') str += ' Italic';
var textFont = font.family;
if(str) textFont = textFont.replace(' Regular', str);
textFont = textFont.split(', ');
var family = font.family;
var style = font.style;
var weight = font.weight;

var parts = family.split(' ');
var isItalic = parts[parts.length - 1] === 'Italic';
if(isItalic) parts.pop();
isItalic = isItalic || style === 'italic';

var str = parts.join(' ');
if(weight === 'bold' && parts.indexOf('Bold') === -1) {
str += ' Bold';
} else if(weight <= 1000) { // numeric font-weight
// See supportedFonts

if(parts[0] === 'Metropolis') {
str = 'Metropolis';
if(weight > 850) str += ' Black';
else if(weight > 750) str += ' Extra Bold';
else if(weight > 650) str += ' Bold';
else if(weight > 550) str += ' Semi Bold';
else if(weight > 450) str += ' Medium';
else if(weight > 350) str += ' Regular';
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') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

str = 'Open Sans';
if(weight > 750) str += ' Extrabold';
else if(weight > 650) str += ' Bold';
else if(weight > 550) str += ' Semibold';
else if(weight > 350) str += ' Regular';
else str += ' Light';
} else if(parts.slice(0, 3).join(' ') === 'Klokantech Noto Sans') {
str = 'Klokantech Noto Sans';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in bff00ac.

if(parts[3] === 'CJK') str += ' CJK';
str += (weight > 500) ? ' Bold' : ' Regular';
}
}

if(isItalic) str += ' Italic';

if(str === 'Open Sans Regular Italic') str = 'Open Sans Italic';
else if(str === 'Open Sans Regular Bold') str = 'Open Sans Bold';
else if(str === 'Open Sans Regular Bold Italic') str = 'Open Sans Bold Italic';
else if(str === 'Klokantech Noto Sans Regular Italic') str = 'Klokantech Noto Sans Italic';

// Ensure the result is a supported font
if(!isSupportedFont(str)) {
str = family;
}

var textFont = str.split(', ');
return textFont;
}
43 changes: 4 additions & 39 deletions src/traces/scattermapbox/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,44 +8,7 @@ var handleLineDefaults = require('../scatter/line_defaults');
var handleTextDefaults = require('../scatter/text_defaults');
var handleFillColorDefaults = require('../scatter/fillcolor_defaults');
var attributes = require('./attributes');

// 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 = [
'Metropolis Black Italic',
'Metropolis Black',
'Metropolis Bold Italic',
'Metropolis Bold',
'Metropolis Extra Bold Italic',
'Metropolis Extra Bold',
'Metropolis Extra Light Italic',
'Metropolis Extra Light',
'Metropolis Light Italic',
'Metropolis Light',
'Metropolis Medium Italic',
'Metropolis Medium',
'Metropolis Regular Italic',
'Metropolis Regular',
'Metropolis Semi Bold Italic',
'Metropolis Semi Bold',
'Metropolis Thin Italic',
'Metropolis Thin',
'Open Sans Bold Italic',
'Open Sans Bold',
'Open Sans Extra Bold Italic',
'Open Sans Extra Bold',
'Open Sans Italic',
'Open Sans Light Italic',
'Open Sans Light',
'Open Sans Regular',
'Open Sans Semibold Italic',
'Open Sans Semibold',
'Klokantech Noto Sans Bold',
'Klokantech Noto Sans CJK Bold',
'Klokantech Noto Sans CJK Regular',
'Klokantech Noto Sans Italic',
'Klokantech Noto Sans Regular'
];
var isSupportedFont = require('./constants').isSupportedFont;
archmoj marked this conversation as resolved.
Show resolved Hide resolved

module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout) {
function coerce(attr, dflt) {
Expand Down Expand Up @@ -104,12 +67,14 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
var clusterEnabled = coerce('cluster.enabled', clusterEnabledDflt);

if(clusterEnabled || subTypes.hasText(traceOut)) {
var layoutFontFamily = layout.font.family;

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',
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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".

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracked in #6993.

weight: layout.font.weight,
style: layout.font.style,
size: layout.font.size,
Expand Down
Loading