Fix for #7416: Hidden ticklabels with ticklabelposition "inside" take up plot space

[my-tien]

In this PR the text of hidden ticklabels is removed so that they don't take up space anymore.

Before the fix (in upper subplot an invisible label "100" takes up space):

After the fix (the vertical grid lines in the subplots are aligned with each other as expected):

Fixes issue: #7416

Disclaimer

I am required to add that…

the software is provided "as is", without warranty of any kind, express or implied, including but not limited to the warranties of merchantability, fitness for a particular purpose and noninfringement. in no event shall the authors or copyright holders be liable for any claim, damages or other liability, whether in an action of contract, tort or otherwise, arising from, out of or in connection with the software or the use or other dealings in the software.

[alexcjohnson]

Nice find and diagnosis @my-tien! I wonder though if there's a better way to do this - specifically, adding this whole extra loop at the end, in addition to being unnecessary vs. just doing the desired thing from the start, I worry it will cause those labels to be missing later like if you drag the axis around, either during or after the drag.

Instead, I wonder if we can avoid this extra step by instead of opacity: 0 | 1 use display: none | block here:

https://github.com/plotly/plotly.js/blob/8e2a5d764c6c4b4d82ba958c2631ed26b9c00cb8/src/plots/cartesian/axes.js#L3654

and here:

https://github.com/plotly/plotly.js/blob/8e2a5d764c6c4b4d82ba958c2631ed26b9c00cb8/src/plots/cartesian/axes.js#L3712-L3714

Because unlike opacity: 0, display: none causes getBoundingClientRect to give zero size.

[my-tien]

Hey Alex, thanks a lot for taking the time! 👍 You are absolutely right, in fact I tried to do this exact same change before, but did it incorrectly: I tried t.style('display', null), but of course it should be t.style('display', 'none'). Now I tried it correctly and it seems to work fine. I will push an update later today.

[my-tien]

@alexcjohnson Pushed new implementation with display: 'none' | null as is being done at several other locations in axes.js.

[alexcjohnson]

@my-tien this looks great. There are some more test failures in axes_test.js, see https://app.circleci.com/pipelines/github/plotly/plotly.js/11937/workflows/35ad298f-0a05-44b8-95de-f8adbfef3f09/jobs/264167 - as mentioned @emilykl is working on the image test failures in #7418 but please make sure that only image tests are failing 😅

[my-tien]

The failing tests were exactly those I adjusted so that they passed on my machine. I reverted the changes.

[alexcjohnson]

Excellent, looks great! This time the image tests actually ran, and for some reason container-colorbar-vertical-w-margin failed to render in both cases - can you try this mock on your branch and see if something really did break there? Once that passes we'll also need a baseline image for your new mock, but that's easiest to do off the artifacts in the test-baselines job, once we get to that point.

[my-tien]

So the issue is related to automargin logic: A bbox was calculated from the display: 'none' label if automargin was set to true . I also saw that adjustTicklabelOverflow was called again and again, alternating the label display between null and 'none'. I now have new failing jasmine tests which I'll have to figure out.

Comments on my attempted fix welcomed as I'm not yet very clear about the required rendering and re-rendering steps.

[my-tien]

Okay, I think I understand a little bit more what's going on. Next commit incoming soon... But this change will probably change a number of baseline images because hidden tick labels affected a couple of them.

[emilykl]

@my-tien Likewise please merge ~main~ master into this branch to take advantage of the changes in #7418 . (I'm not sure you're actually hitting those issues in this branch, but just in case.)

[emilykl]

@alexcjohnson @my-tien One interesting case from the image diffs -- I don't think the rightward image is wrong, since it was kind of just a fortunate edge case that the gridlines were aligned before, but for a user who finds themself in this situation, is there an easy way in the figure definition to force the gridlines to be aligned?

main	my-tien:label-spacing

[emilykl]

@my-tien The container-colorbar image diffs still seem wrong to me (given the way the legend extends off the bottom of the chart) -- are you still working on the automargin behavior?

Otherwise this is looking good to me.

[my-tien]

Hi Emily, thanks for looking at this!

@my-tien The container-colorbar image diffs still seem wrong to me (given the way the legend extends off the bottom of the chart) -- are you still working on the automargin behavior?

Otherwise this is looking good to me.

I'm not 100% sure what legend you mean, do you mean the yellow box around the colorbar? (previous) (new)

The colorbar container and the x-axis ticks are now cut-off because the hidden -0.5 label on the y-axis doesn't take up space anymore and therefore doesn't push down the margins, i.e. same behavior as when yaxis.automargin is set to false. (I also think it is correct, that -0.5 is hidden, because the default ticklabeloverflow behavior for this axis is hide past (graph)div). If I now set layout.title.automargin: false as well, the chart is exactly 300 px high as specified in the layout height.

Compare to the current behavior when automargin is completely turned off, the colorbar container is also cut-off. So if this is a bug, I think it happens somewhere else: codepen

[alexcjohnson]

One interesting case from the image diffs -- I don't think the rightward image is wrong, since it was kind of just a fortunate edge case that the gridlines were aligned before, but for a user who finds themself in this situation, is there an easy way in the figure definition to force the gridlines to be aligned?

The main situation you'd want these aligned is when they're either literally the same x axis or the two x axes are matched. Hopefully that's handled correctly already? Absent that I'd say the behavior here is correct.

The container-colorbar image diffs still seem wrong to me (given the way the legend extends off the bottom of the chart)

Hmph there are a bunch of issues here I'm not sure we ever really considered re: automargin, and I think @my-tien is right that this is not a problem with this PR but with colorbars. This colorbar has default position (y=0.5) and size (len=1), which means it extends beyond the plot area only due to however we decide to treat its border: does the border fit inside its specified space, in which case the thicker you make the border the more it needs to squeeze its content, or does the border go outside the specified space, in which case it should impact margins. Or is the midline of the border on the specified box, in which case it's a bit of each? I don't know the right answer here but this shouldn't block this PR, we should make a new issue and consider how we can best align the behavior with other components (legends, text annotations, axis lines).

There are also the tick marks, which are apparently not hidden on the x axis but don't trigger automargin even though arguably they should. That's another issue independent of this PR, though I'd call that relatively low-priority: how often do real plots have tick marks with no labels at all?

[my-tien]

Just to make sure: from my side no additional changes are required since the existing problems weren't caused by this PR, right?

[emilykl]

Just to make sure: from my side no additional changes are required since the existing problems weren't caused by this PR, right?

@my-tien I believe that's correct, I don't think the Jasmine test failures are related.

Could you download the changed baseline images from the "Artifacts" tab of the test-baselines CircleCI job, and commit them to this PR? You may need to push a dummy commit first to trigger the pipeline again, I wasn't able to re-run it manually.

[emilykl]

Could you also merge master into this branch again? When you do so, that should trigger the pipeline.

[my-tien]

Merged and updated baseline images

[my-tien]

Hi @emilykl, is there any chance for this PR to make it into the next release?

[emilykl]

@my-tien Yes I hope we can get this into the next release. I need to figure out why the test pipelines are failing which I hope to do today (no action required on your part). Thank you for your patience.

[archmoj]

@my-tien Could you please merge master into this branch? Thank you.

[emilykl]

@my-tien, could you please cherry-pick commit dfcdfdf onto this branch?

[my-tien]

Thanks @emilykl and @archmoj and @alexcjohnson! <3