Skip to content

Commit 6c8f2af

Browse files
rmoestlrmoestl
committed
Use contrasting inside text color for bar by default [2951]
1 parent 487f73e commit 6c8f2af

File tree

3 files changed

+181
-20
lines changed

3 files changed

+181
-20
lines changed

‎src/traces/bar/defaults.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,15 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
4646

4747
if(hasInside || hasOutside) {
4848
var textFont = coerceFont(coerce, 'textfont', layout.font);
49-
if(hasInside) coerceFont(coerce, 'insidetextfont', textFont);
49+
if(hasInside) {
50+
var insideTextFontDefault = Lib.extendFlat({}, textFont);
51+
var isTraceTextfontColorSet = traceIn.textfont && traceIn.textfont.color;
52+
var isColorInheritedFromLayoutFont = !isTraceTextfontColorSet;
53+
if(isColorInheritedFromLayoutFont) {
54+
delete insideTextFontDefault.color;
55+
}
56+
coerceFont(coerce, 'insidetextfont', insideTextFontDefault);
57+
}
5058
if(hasOutside) coerceFont(coerce, 'outsidetextfont', textFont);
5159
coerce('constraintext');
5260
coerce('selected.textfont.color');

‎src/traces/bar/plot.js

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ function appendBarText(gd, bar, calcTrace, i, x0, x1, y0, y1) {
178178
}
179179

180180
var textFont = getTextFont(trace, i, gd._fullLayout.font),
181-
insideTextFont = getInsideTextFont(trace, i, textFont),
181+
insideTextFont = getInsideTextFont(trace, i, textFont, calcTrace[i].mc),
182182
outsideTextFont = getOutsideTextFont(trace, i, textFont);
183183

184184
// compute text position
@@ -443,9 +443,20 @@ function getTextFont(trace, index, defaultValue) {
443443
attributeTextFont, trace.textfont, index, defaultValue);
444444
}
445445

446-
function getInsideTextFont(trace, index, defaultValue) {
446+
function getInsideTextFont(trace, index, defaultFont, barColor) {
447+
var wouldFallBackToLayoutFont =
448+
(trace._input.textfont === undefined || trace._input.textfont.color === undefined) ||
449+
(Array.isArray(trace.textfont.color) && trace.textfont.color[index] === undefined);
450+
if(wouldFallBackToLayoutFont) {
451+
defaultFont = {
452+
color: Color.contrast(barColor),
453+
family: defaultFont.family,
454+
size: defaultFont.size
455+
};
456+
}
457+
447458
return getFontValue(
448-
attributeInsideTextFont, trace.insidetextfont, index, defaultValue);
459+
attributeInsideTextFont, trace.insidetextfont, index, defaultFont);
449460
}
450461

451462
function getOutsideTextFont(trace, index, defaultValue) {

‎test/jasmine/tests/bar_test.js

Lines changed: 158 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,17 @@ var destroyGraphDiv = require('../assets/destroy_graph_div');
1212
var failTest = require('../assets/fail_test');
1313
var checkTicks = require('../assets/custom_assertions').checkTicks;
1414
var supplyAllDefaults = require('../assets/supply_defaults');
15+
var color = require('../../../src/components/color');
16+
var rgb = color.rgb;
1517

1618
var customAssertions = require('../assets/custom_assertions');
1719
var assertClip = customAssertions.assertClip;
1820
var assertNodeDisplay = customAssertions.assertNodeDisplay;
1921

2022
var d3 = require('d3');
2123

24+
var BAR_TEXT_SELECTOR = '.bars .bartext';
25+
2226
describe('Bar.supplyDefaults', function() {
2327
'use strict';
2428

@@ -122,28 +126,58 @@ describe('Bar.supplyDefaults', function() {
122126
expect(traceOut.constraintext).toBeUndefined();
123127
});
124128

125-
it('should default textfont to layout.font', function() {
129+
it('should default textfont to layout.font except for insidetextfont.color', function() {
126130
traceIn = {
127131
textposition: 'inside',
128132
y: [1, 2, 3]
129133
};
130-
131134
var layout = {
132135
font: {family: 'arial', color: '#AAA', size: 13}
133136
};
137+
var layoutFontMinusColor = {family: 'arial', size: 13};
134138

135139
supplyDefaults(traceIn, traceOut, defaultColor, layout);
136140

137141
expect(traceOut.textposition).toBe('inside');
138142
expect(traceOut.textfont).toEqual(layout.font);
139143
expect(traceOut.textfont).not.toBe(layout.font);
140-
expect(traceOut.insidetextfont).toEqual(layout.font);
144+
expect(traceOut.insidetextfont).toEqual(layoutFontMinusColor);
141145
expect(traceOut.insidetextfont).not.toBe(layout.font);
142146
expect(traceOut.insidetextfont).not.toBe(traceOut.textfont);
143147
expect(traceOut.outsidetexfont).toBeUndefined();
144148
expect(traceOut.constraintext).toBe('both');
145149
});
146150

151+
it('should not default insidetextfont.color to layout.font.color', function() {
152+
traceIn = {
153+
textposition: 'inside',
154+
y: [1, 2, 3]
155+
};
156+
var layout = {
157+
font: {family: 'arial', color: '#AAA', size: 13}
158+
};
159+
160+
supplyDefaults(traceIn, traceOut, defaultColor, layout);
161+
162+
expect(traceOut.insidetextfont.family).toBe('arial');
163+
expect(traceOut.insidetextfont.color).toBeUndefined();
164+
expect(traceOut.insidetextfont.size).toBe(13);
165+
});
166+
167+
it('should default insidetextfont.color to textfont.color', function() {
168+
traceIn = {
169+
textposition: 'inside',
170+
y: [1, 2, 3],
171+
textfont: {family: 'arial', color: '#09F', size: 20}
172+
};
173+
174+
supplyDefaults(traceIn, traceOut, defaultColor, {});
175+
176+
expect(traceOut.insidetextfont.family).toBe('arial');
177+
expect(traceOut.insidetextfont.color).toBe('#09F');
178+
expect(traceOut.insidetextfont.size).toBe(20);
179+
});
180+
147181
it('should inherit layout.calendar', function() {
148182
traceIn = {
149183
x: [1, 2, 3],
@@ -802,6 +836,9 @@ describe('Bar.crossTraceCalc (formerly known as setPositions)', function() {
802836
describe('A bar plot', function() {
803837
'use strict';
804838

839+
var DARK = rgb('#444');
840+
var LIGHT = rgb('#fff');
841+
805842
var gd;
806843

807844
beforeEach(function() {
@@ -849,19 +886,13 @@ describe('A bar plot', function() {
849886
expect(pathBB.right).not.toBeGreaterThan(textBB.left);
850887
}
851888

852-
var colorMap = {
853-
'rgb(0, 0, 0)': 'black',
854-
'rgb(255, 0, 0)': 'red',
855-
'rgb(0, 128, 0)': 'green',
856-
'rgb(0, 0, 255)': 'blue'
857-
};
858-
function assertTextFont(textNode, textFont, index) {
859-
expect(textNode.style.fontFamily).toBe(textFont.family[index]);
860-
expect(textNode.style.fontSize).toBe(textFont.size[index] + 'px');
889+
function assertTextFont(textNode, expectedFontProps, index) {
890+
expect(textNode.style.fontFamily).toBe(expectedFontProps.family[index]);
891+
expect(textNode.style.fontSize).toBe(expectedFontProps.size[index] + 'px');
861892

862-
var color = textNode.style.fill;
863-
if(!colorMap[color]) colorMap[color] = color;
864-
expect(colorMap[color]).toBe(textFont.color[index]);
893+
var actualColorRGB = textNode.style.fill;
894+
var expectedColorRGB = rgb(expectedFontProps.color[index]);
895+
expect(actualColorRGB).toBe(expectedColorRGB);
865896
}
866897

867898
function assertTextIsBeforePath(textNode, pathNode) {
@@ -871,6 +902,36 @@ describe('A bar plot', function() {
871902
expect(textBB.right).not.toBeGreaterThan(pathBB.left);
872903
}
873904

905+
function assertTextFontColors(expFontColors) {
906+
return function() {
907+
var selection = d3.selectAll(BAR_TEXT_SELECTOR);
908+
expect(selection.size()).toBe(expFontColors.length);
909+
selection.each(function(d, i) {
910+
expect(this.style.fill).toBe(expFontColors[i], 'element ' + i);
911+
});
912+
};
913+
}
914+
915+
function assertTextFontFamilies(expFontFamilies) {
916+
return function() {
917+
var selection = d3.selectAll(BAR_TEXT_SELECTOR);
918+
expect(selection.size()).toBe(expFontFamilies.length);
919+
selection.each(function(d, i) {
920+
expect(this.style.fontFamily).toBe(expFontFamilies[i]);
921+
});
922+
};
923+
}
924+
925+
function assertTextFontSizes(expFontSizes) {
926+
return function() {
927+
var selection = d3.selectAll(BAR_TEXT_SELECTOR);
928+
expect(selection.size()).toBe(expFontSizes.length);
929+
selection.each(function(d, i) {
930+
expect(this.style.fontSize).toBe(expFontSizes[i] + 'px');
931+
});
932+
};
933+
}
934+
874935
it('should show bar texts (inside case)', function(done) {
875936
var data = [{
876937
y: [10, 20, 30],
@@ -999,6 +1060,84 @@ describe('A bar plot', function() {
9991060
.then(done);
10001061
});
10011062

1063+
var insideTextTestsTraceDef = {
1064+
x: ['giraffes', 'orangutans', 'monkeys', 'elefants', 'spiders', 'snakes'],
1065+
y: [20, 14, 23, 10, 59, 15],
1066+
text: [20, 14, 23, 10, 59, 15],
1067+
type: 'bar',
1068+
textposition: 'auto',
1069+
marker: {
1070+
color: ['#ee1', '#eee', '#333', '#9467bd', '#dda', '#922'],
1071+
}
1072+
};
1073+
1074+
it('should use inside text colors contrasting to bar colors by default', function(done) {
1075+
Plotly.plot(gd, [insideTextTestsTraceDef])
1076+
.then(assertTextFontColors([DARK, DARK, LIGHT, LIGHT, DARK, LIGHT]))
1077+
.catch(failTest)
1078+
.then(done);
1079+
});
1080+
1081+
it('should use defined textfont.color for inside text instead of the contrasting default', function(done) {
1082+
var data = Lib.extendFlat({}, insideTextTestsTraceDef, { textfont: { color: '#09f' } });
1083+
1084+
Plotly.plot(gd, [data])
1085+
.then(assertTextFontColors(Lib.repeat(rgb('#09f'), 6)))
1086+
.catch(failTest)
1087+
.then(done);
1088+
});
1089+
1090+
it('should use matching color from textfont.color array for inside text, contrasting otherwise', function(done) {
1091+
var data = Lib.extendFlat({}, insideTextTestsTraceDef, { textfont: { color: ['#09f', 'green'] } });
1092+
1093+
Plotly.plot(gd, [data])
1094+
.then(assertTextFontColors([rgb('#09f'), rgb('green'), LIGHT, LIGHT, DARK, LIGHT]))
1095+
.catch(failTest)
1096+
.then(done);
1097+
});
1098+
1099+
it('should use defined insidetextfont.color for inside text instead of the contrasting default', function(done) {
1100+
var data = Lib.extendFlat({}, insideTextTestsTraceDef, { insidetextfont: { color: '#09f' } });
1101+
1102+
Plotly.plot(gd, [data])
1103+
.then(assertTextFontColors(Lib.repeat(rgb('#09f'), 6)))
1104+
.catch(failTest)
1105+
.then(done);
1106+
});
1107+
1108+
it('should use matching color from insidetextfont.color array instead of the contrasting default', function(done) {
1109+
var data = Lib.extendFlat({}, insideTextTestsTraceDef, { insidetextfont: { color: ['yellow', 'green'] } });
1110+
1111+
Plotly.plot(gd, [data])
1112+
.then(assertTextFontColors([rgb('yellow'), rgb('green'), LIGHT, LIGHT, DARK, LIGHT]))
1113+
.catch(failTest)
1114+
.then(done);
1115+
});
1116+
1117+
it('should fall back to textfont array values if insidetextfont array values don\'t ' +
1118+
'cover all bars', function(done) {
1119+
var trace = Lib.extendFlat({}, insideTextTestsTraceDef, {
1120+
textfont: {
1121+
color: ['blue', 'blue', 'blue'],
1122+
family: ['Arial', 'serif'],
1123+
size: [8, 24]
1124+
},
1125+
insidetextfont: {
1126+
color: ['yellow', 'green'],
1127+
family: ['Arial'],
1128+
size: [16]
1129+
}
1130+
});
1131+
var layout = {font: {family: 'Roboto', size: 12}};
1132+
1133+
Plotly.plot(gd, [trace], layout)
1134+
.then(assertTextFontColors([rgb('yellow'), rgb('green'), rgb('blue'), LIGHT, DARK, LIGHT]))
1135+
.then(assertTextFontFamilies(['Arial', 'serif', 'Roboto', 'Roboto', 'Roboto', 'Roboto']))
1136+
.then(assertTextFontSizes([16, 24, 12, 12, 12, 12]))
1137+
.catch(failTest)
1138+
.then(done);
1139+
});
1140+
10021141
it('should be able to restyle', function(done) {
10031142
var mock = Lib.extendDeep({}, require('@mocks/bar_attrs_relative'));
10041143

@@ -1170,6 +1309,9 @@ describe('A bar plot', function() {
11701309
font: {family: 'arial', color: 'blue', size: 13}
11711310
};
11721311

1312+
// Note: insidetextfont.color does NOT inherit from textfont.color
1313+
// since insidetextfont.color should be contrasting to bar's fill by default.
1314+
var contrastingLightColorVal = color.contrast('black');
11731315
var expected = {
11741316
y: [10, 20, 30, 40],
11751317
type: 'bar',
@@ -1182,7 +1324,7 @@ describe('A bar plot', function() {
11821324
},
11831325
insidetextfont: {
11841326
family: ['"comic sans"', 'arial', 'arial'],
1185-
color: ['black', 'green', 'blue'],
1327+
color: ['black', 'green', contrastingLightColorVal],
11861328
size: [8, 12, 16]
11871329
},
11881330
outsidetextfont: {

0 commit comments

Comments
 (0)