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.

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

pie insidetext colour by default isn't readable #2951

Closed
dsmmcken opened this issue Aug 28, 2018 · 7 comments
Closed

pie insidetext colour by default isn't readable #2951

dsmmcken opened this issue Aug 28, 2018 · 7 comments
Assignees
Labels
bug something broken feature something new

Comments

@dsmmcken
Copy link

If a user creates a pie chart using all default settings and the text labels default to inside the slice, then some labels become unreadable depending on slice colour (below WCAG guidelines). For inside labels, colour should auto set to contrasting. A plot produced with all default settings should always be usable.

screen shot 2018-08-28 at 6 10 12 pm

Link to codepen

Furthermore, if a user sets a custom marker colours array, there is no way for them to set the text colour to contrast or set text colours individually.

@alexcjohnson
Copy link
Collaborator

Thanks @dsmmcken - this issue also pertains to bars:
screen shot 2018-08-28 at 6 46 52 pm
We should:

  • make the default flip from black to white per slice the same way hover labels do
  • allow an array for font properties - was left as a TODO and never followed up on:
    // TODO make those arrayOk?

Bars already allow array font properties:
screen shot 2018-08-28 at 6 56 27 pm
But there's a bug (see trace 0 above, the lower set of positive bars) where insidetextfont is ignored when you explicitly set textposition: 'outside' but the text gets drawn inside anyway because another set of bars got stacked on top.
https://codepen.io/alexcjohnson/pen/aamzMP?editors=1010

@alexcjohnson alexcjohnson added bug something broken feature something new labels Aug 28, 2018
@rmoestl rmoestl self-assigned this Sep 25, 2018
@rmoestl
Copy link
Contributor

rmoestl commented Sep 27, 2018

Tasks

To summarize, we need to implement two new features and fix one bug:

  • Choose a contrasting text color for inside labels by default
  • Allow to set font properties for inside labels on a per slice basis
  • Fix bar (and potentially pie) to use insidetextfont if the bar label is drawn inside the bar (to avoid overlapping) even if textposition is set to outside

API changes

From an API point of view, I believe we need to address how font properties are inherited from layout.font all the way down to pie.insidetextfont and bar.insidetextfont. With the new default behavior (contrasting), layout.font.color can no longer be inherited by pie.insidetextfont.color.

Still, I believe users need an option to back away from the new contrasting default. Why? Because there might exist pie and bar charts that set layout.font.color to change the color of inside pie labels globally.

Therefore I suggest to add the new boolean attribute pie.insidetextfont.contrasting that defaults to true. This breaks aforementioned charts, but users at least have a way to fix that with pie.insidetextfont.contrasting: false.

@alexcjohnson and @etpinard, what do you think?

Bug in online API reference?

The color attribute description, see https://plot.ly/javascript/reference/#bar-insidetextfont-color, lacks that it can be an array of colors as well.

@etpinard
Copy link
Contributor

Choose a contrasting text color for inside labels by default
Allow to set font properties for inside labels on a per slice basis
Fix bar (and potentially pie) to use insidetextfont if the bar label is drawn inside the bar (to avoid overlapping) even if textposition is set to outside

This sums up what's at hand very well. Thanks for writing this down!

Still, I believe users need an option to back away from the new contrasting default. Why? Because there might exist pie and bar charts that set layout.font.color to change the color of inside pie labels globally.

I agree with you 💯 % here.

Therefore I suggest to add the new boolean attribute pie.insidetextfont.contrasting that defaults to true. This breaks aforementioned charts, but users at least have a way to fix that with pie.insidetextfont.contrasting: false.

Interesting idea. There's #1597, that could offer an alternative solution. I'm not entirely sure what's best going forward. This is something we could talk about during next week's plotly.js meeting.

The color attribute description, see https://plot.ly/javascript/reference/#bar-insidetextfont-color, lacks that it can be an array of colors as well.

Good eye. Interestingly though, it is listed properly:

var textFontAttrs = fontAttrs({
editType: 'calc',
arrayOk: true,
description: ''
});

insidetextfont: extendFlat({}, textFontAttrs, {
description: 'Sets the font used for `text` lying inside the bar.'
}),

.. it might be a bug in https://github.com/plotly/documentation/tree/source

@dsmmcken
Copy link
Author

dsmmcken commented Sep 28, 2018

There's #1597, that could offer an alternative solution.

Please, no. Not as a default anyways. While in theory a border on text makes it readable on all colours, it also makes it ugly on all colours.

I also dislike the border as used for the same purpose on the hoverlabels, which can't be disabled from the API. I can set a bordercolor: transparent which for whatever reason is then also used for the font colour!?, so now I have to specify that too, but that means now I can't get it to be auto contrasting with the label colour, so now I would have to start manual specifying font colors all because I don't want the border. Alternately, I can kill it with a CSS override, which is what I currently do on all my plots. --Edit: that behaviour should probably be considered a bug and I'll open a separate ticket.

If that is the route you want to take on this, can I suggest two things:

  1. allow any bordercolor to be set as "none" (hoverlabels should accept that as well).
  2. still allow font color to be explicitly set to be set in the API as "contrasting" or "auto"

@rmoestl
Copy link
Contributor

rmoestl commented Oct 1, 2018

Thanks @dsmmcken. I think both solutions – let's call them auto-contrasting and text with outline – are valuable in different situations.

In the Google Maps scenario (referenced in #1597) where text labels overlay multiple background colors, I think text with outline is the only viable solution. But in other scenarios, like pie charts with homogeneous backgrounds, I'd say auto-contrasting is the more aesthetic solution.

Now, do pie or even bar really have homogeneous backgrounds? What if a long pie label overflows its slice and reaches into the neighbor slice with an entirely different bg-color? Then we have a Google Maps like scenario again.

In my opinion it boils down to and depends on the use case:

  • For charts you don't control the input data (products etc.) you certainly would want to have text with outline
  • For manually created one-off charts I think you'd always want to give auto-contrasting a try

I think Plotly.js should have both modes eventually. The question is, how should the API look like and which mode should be the default.

.. it might be a bug in https://github.com/plotly/documentation/tree/source

Should I create a bug report? Or should I just ping @cldougl?

@cldougl
Copy link
Member

cldougl commented Oct 1, 2018

@rmoestl at first glance it looks like a bug w/ the documentation-- if you can create a report in the doc repo: https://github.com/plotly/documentation/issues and tag me there that would be super helpful!

@rmoestl
Copy link
Contributor

rmoestl commented Oct 11, 2018

Here's a summary of a discussion w/t Alex and Etienne that clarified the questions above:

  • A new attribute pie.insidetextfont.contrasting isn't really needed. If a user really don't want auto-contrasting, they can set insidetextfont.color to whatever color they want, e.g. dark-grey which is the default right now.
  • It is agreed upon that this really is a fix to non-readable labels. So no text with outline solution discussed above, which could be a future feature though. But it's serves a different use case.
  • In addition users will be able to set individual insidetext colors per slice (arrayOk)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

No branches or pull requests

5 participants