The funny indentation this had is fairly standard with d3, to mark operations within a chain that change the selection. Not sure how consistent we've been with that but it was done that way intentionally.

It looks like you've based this on pie/defaults but you've got more logic around arrays in the input. So two questions:

• for the cases that are covered by pie, does this give the same inheritance from layout.font and textfont to insidetextfont and outsidetextfont, particularly around cases where parts of the font come from one default and other parts come from the other one? for example if layout.font supplies the family, textfont supplies the size, and insidetextfont makes it white.
• what extra cases does this cover and should we extend pie to cover these too?

Yes, I've tried to replicate the behaviour of pie traces. This is tested here. The test is a good example of the extra cases handled here.

The main difference is that these attributes now also accept arrays (so that fonts can be defined per bar).

I think a lot of the functionality implemented in this file could be moved to Lib and reused by pie traces.

Cool. Looks like the inheritance for non-arrays is as I'd expect 👍 . For arrays though, I'm not sure about the structure - I'd think for consistency with data attributes and src tags, it would be better to have family, size, and color be arrays, rather than have font be an array of objects.

We don't coerce entries inside data arrays or arrayOk attributes.

Non-text entries are usually via fallbacks later on in the pipeline e.g. in the drawing step for scatter text here.

The reason being: we try to make Plots.supplyDefaults scale ~ number of attribute as opposed to ~ length of the data.

What the pie trace type is currently doing (here) looks sufficient to me at the default step.

@etpinard I don't understand the phrase "to make Plots.supplyDefaults scale ~ number of attribute"? Could you give me an example?

As in: there are about as many coerce statements as there are attributes.

What you added above has about as many coerce statements as there are items in the text attribute arrays.

@etpinard What are the reasons against making Plots.supplyDefaults scale with the data? Note that the calls to nestedProperty would still scale only with the number of attributes.

Having uncoerced data in arrays after running Plots.supplyDefaults means that we have to coerce this data in Bar.plot. This increases the number of places where untrusted user input can brake Plotly.

Another disadvantage of moving the code for array coercion from Bar.supplyDefaults into Bar.plot (instead of Lib) is that pie traces wouldn't be able to reuse the code.

Perf over DRY.

supplyDefaults gets called a lot more often than .plot.

@etpinard OK, I understand. Thx for the explanation.

For pies we did this a bit differently - in 'inside' mode we would always draw the text inside the slice, and shrink it if need be so it would fit (no matter how small the space is). And in 'auto' mode we would first draw it inside, then if it had to be scaled down to fit inside we'd move it outside.

This algorithm should be easier with a rectangular bar than with a wedge-shaped pie slice - do you want to give it a try?

Yes, no problem. One question though. If a text doesn't fit, shall I consider rotating the text before moving it outside?

If a text doesn't fit, shall I consider rotating the text before moving it outside?

Yes, I think so. Basically do everything you'd do with explicitly inside text, which includes seeing if you can make it bigger by rotating it, and only after that see if you can make it bigger by moving it outside. I guess even outside text can get squished here if it's wider than the bar... so it would be silly to move the text outside and still not be able to expand it!

makes me wonder if we need an attribute textorientation or something, that would be 'auto' (whatever is biggest), 'horizontal', or 'vertical'.

I consider rotating the text before moving it outside?

I think so, as long as the text can only be horizontal or vertical (no diagonal allowed).

@alexcjohnson I've set a 3px padding. This means that unless a bar is larger than 6px, no labels could be drawn inside. Is it OK not to draw the label in those cases? Or shall I remove the padding if the bar is 6px or less?

FYI @alexcjohnson will be offline until Monday.

6px sounds ok to me.

An alternative that might be more analogous to what we did with pies (where we didn't explicitly include padding, but fitting text into a rectangle into a wedge effectively results in some padding in most cases) would be to scale down the padding by the same factor you use to scale down the label. The only context I see this making a big difference is print / pdf, where hover & zoom are unavailable but the interested viewer may be able to zoom in the document (or hold it up to their nose) and still read it.

Not blocking - we can always tweak this later if it becomes important - but I think it would be a bit nicer that way.

is there a criterion by which we should rotate an outside label ('very small size') to vertical like you do with inside labels ('Three')?

Also, and this applies to all the inside labels, I'd have expected inside labels to be pushed next to the bar end, the same way as you have outside labels right on the other side of the bar end. It looks like some people want the labels in the middle, so perhaps this should be an option (perhaps textalign: middle or end) but I think semantically it's a better default to draw people's eyes toward the bar end. Seems like that's somewhat more common as well.

@alexcjohnson Without going into the details, the idea is that if a text doesn't fit inside a bar, we shrink the text by computing a scaling factor (smaller than 1).

To avoid shrinking the text unnecessarily or too much, the text is rotated 90 degrees when this results in a larger scaling factor (still smaller than 1).

I'm not sure if I did a good job with the above explanation. If not, I can give an example.

Yes, that's the correct behavior for inside labels, and it's basically the same thing we do for pies.

But there were two additional issues in my comment:

• for outside labels, shall we do the same thing? ie rotate the bar 90 degrees when it would allow us to enlarge the scaling?
• for inside labels, once we've figured out the maximum scaling, can we push the label over near the end of the bar rather than the middle?

@alexcjohnson I've just realised that my implementation for outside labels doesn't do what I said 😞 . How do you think it should be? Shall I rotate it like in the case of inside labels?

👍 Yes, I'll push the labels close to the bar ends. One question: at the moment I'm using a 1px padding, shall I increase it (the padding in the ggplot2 example looks too much, though)?

Shall I rotate it like in the case of inside labels?

Yes, I think so. One concern with this (it's already a concern but will get worse when we rotate the text) is whether we need to include that label in autorange (via Axes.expand and its ppadplus and ppadminus options). That could get awfully annoying though, as you'd have to make the outside text elements available for sizing during setPositions rather than plot, but then you still need to know the bar width to tell whether to rotate or not... ugh. @etpinard thoughts?

1px padding, shall I increase it (the padding in the ggplot2 example looks too much, though)?

Agreed about ggplot2. For hover text we use 3px - how about that?

no need to clean (plotly.js lingo for backward-compatible mappings) attribute that have never been released.

That's what this line change is meant for; i.e. to exclude bar's textposition from those mappings.

Oops. You're right. My mistake. 😄

nicely done here.

We don't need to run this loop if textposition: 'none', correct?

Last night, you made me hesitate! But after further thought, here's the correct reasoning.

Bars with textposition: 'none' are still visible (only the bar text is hidden) and thus they can be the outmost bar.

@n-riesco general comment: would mind adding the text attributes and friends to the existing mocks you added for width / offset / base? We have over 300 image mocks now - which take about 2 and 1/2 minute to tests. It's time to be more meticulous when adding new mocks.

I should've thought of that.

very nice here.

Asserting getBoundingClientRect return values would be very susceptible to cross-browser cross-env discrepancies.

Since I've used Drawing.bBox(node) to compute the location of bar texts and Drawing.bBox(node) uses getBoundingClientRect this shouldn't be an issue.

I noticed, however, in certain cases, a rounding error that would cause the bar text to be negligibly larger than computed. For that reason, and also because it looks neater, I added some padding to bar texts. See here and here.

Great. Nice and clear.

This is pretty much on bar with the Drawing does for scatter traces. So 👍

That said, I believe (I haven't tried yet) that some of this logic could be moved to the calc (which is called even less often than plot and supplyDefaults). But, that will be for a different PR.

@etpinard Is there anything in Drawing that you think I should've reused?

Drawing is pretty messy at the moment. Though it would be nice to merge the text-drawing logic from scatter, pie and bar together at some point. No action required for this iteration.

Can you simply reuse Lib.coerce here?

I couldn't do it when textfont accepted an array, but now I should be able to.

⬆️ blocking before making a PR to plotly.js

@etpinard After some sleep, I remembered that the reason why I introduced those functions is that I don't know how to use Lib.coerce to coerce array elements. Even if something like Lib.coerce('textfont.family[' + index + ']', dflt) works, iterating an array like this must be awfully inefficient.

Would it be OK for me to modify src/lib/corce.js and move coerceString to a coerce field and update the corresponding coerceFunction to use this new coerce field?

iterating an array like this must be awfully inefficient.

Very true. That's why we don't use Lib.coerce on array items usually. You probably noticed that Drawing doesn't have any coerce statements.

For what it's worth, here's an example of how to get it to work with array under info_array.

So, I'll leave it up to you. I'm ok with the current solution. But ideally, we should move all array-item casting to the calc step - which can wait for a refactor PR.

Hmm. Looks like that "size": 32 isn't reflected on the baseline image.

@etpinard This is on purpose to force a rotation in all the outside labels. If the font is small enough, the label can be drawn without rotation.

You can play with the size to see what I mean.

Got it. Thanks for the info!

If I understand correctly, only the outer most bar can have outside text?

Yes, that's correct. Is there anything wrong?

Nothing wrong. Just double checking. 👍

why?

I wanted to have a small negative bar, so that the label "outside text" doesn't fit inside the bar and has to be drawn below the bar (as opposed to the positive bar case that has outside texts drawn above the bar).

got it, thanks!

Nice test. Can you make sure that Plotly.restyle works with the new attributes too?

⬆️ blocking before making a PR to plotly.js

Two questions here:

1. why is "outside text" outside? Wouldn't it be just as big on the inside?
2. Where did "0" go? Even though the bar has no size, because it's the last bar we could show it where the last bar would have been (and likewise, it's confusing that the "4" from trace 0 shows up on the outside, it feels like only trace 1 should be able to have a label there.

Why is "outside text" outside? Wouldn't it be just as big on the inside?

The way I understood the criteria for auto is that if it doesn't fit inside, it goes outside (no matter if shrinking is required to draw it outside).

Where did "0" go?

0-size bars aren't even included in gd.calcdata

it feels like only trace 1 should be able to have a label there

I think this feeling is specific to this case, because trace[1] only has bars with positive sizes. If trace[1][3] was -1 instead of 0, then the bar and the label would be drawn on the negative side.

The way I understood the criteria for auto is that if it doesn't fit inside, it goes outside (no matter if shrinking is required to draw it outside).

Yeah, that's probably what I said :) But would you agree that if the shrinking is the same either way then it should stay inside?

0-size bars aren't even included in gd.calcdata

OK cool - yes, definitely an edge case, lets leave this one as it is.

@alexcjohnson OK, I'll change auto:

• if it fits inside, the label will be drawn inside
• if it doesn't, the label will be drawn wherever is larger

It's confusing to read this plot with the different text labels rotated different directions. I'd rotate them only clockwise (if they're going to rotate)

How about we don't rotate the labels at all and let the user decide?

Something like this:

    textrotation: {
        valType: 'number',
        dflt: null,
        arrayOk: true,
        role: 'info',
        description: [
            'Rotates the bar text (in clockwise degrees).'
        ].join(' ')
    },