Bugfix/19224-treegraph-overlapping-labels-not-working

[MarkusBarstad]

trafficstars

Fixed #19224, treegraph data labels did not hide when allowOverlap was false.

For any HC pros reading this: This line confuses me.

We splat this.options.dataLabels into an array but we never use it again (except in this branch) 😕 To me it seems like this does nothing. Is there some reason not apparent in the function for doing this?

I am unsure on whether to remove this line and exactly the proper way to refer to options of dataLabels for the treegraph series.

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1204915984120495

[highsoft-bot]

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

	master	candidate	difference
highcharts.js	95.7 kB 269.9 kB	95.8 kB 270.0 kB	78 B 31 B
highstock.js	128.7 kB 372.2 kB	128.8 kB 372.2 kB	66 B 31 B
highmaps.js	121.6 kB 349.3 kB	121.6 kB 349.2 kB	19 B -109 B
highcharts-gantt.js	130.7 kB 377.1 kB	130.8 kB 377.1 kB	62 B 31 B

[highsoft-bot]

Visual test results - No difference found

[MarkusBarstad]

To me, those links data labels are still overlapping with each other and they shouldn't. Currently, we don't have logic to figure out the box of data label with text path, so I think it would be necessary to implement it.

True, they should be hidden when there is any overlapping at all.

Do you think it would be appropriate to alter the chart.hideOverlappingLabels function to handle this case as well? It hides some of the overlapping labels but it seems like it does not handle rotation well or something

[hubertkozik]

I didn't test it, but I think chart.hideOverlappingLabels works well, but getBBox of text with textPath return box without textPath, so I would try to add logic to getBBox to include textPath property and calculate proper box of that element.

[MarkusBarstad]

2. The actual rendered polygon around the green circle is jagged.

@TorsteinHonsi Is it possible that the polygon appears jagged because you have drawn a path with an unaltered polygon-array? If this is the case the polygon will have this jagged look because we only take the left side of the vertices.

Consider this example (easier to spot here because the polygons dimensions is more correct now):

The first step of the path is the top left, then bottom left, then it goes to the next 5th character's top left.

If we wanted to draw a path with a more rectangular shape we would have to introduce more coordinates to the polygon array.

This does not matter for our "pointInPoly" function because it does not care about the order of vertices, so for all intents and purposes it will see our polygon in a more 'rectangular manner' so to speak.

[TorsteinHonsi]

I didn't actually test this, but I suspect the current top-bottom, top-bottom polygon definition may create some false negative hit detection when the corner of one polygon hits the gap of another. Photoshopped:

The way to overcome that would be to instead of pushing top and bottom points to the polygon, you would push the top point and unshift the bottom point. Or vice versa. Then the vertices won't cross.

[MarkusBarstad]

The way to overcome that would be to instead of pushing top and bottom points to the polygon, you would push the top point and unshift the bottom point. Or vice versa. Then the vertices won't cross.

@TorsteinHonsi , I do not think this impacts overlap-detection, because the vertices can be in any order for our polygon detection function. Here is a stress test, 1000 points, a dataLabel for all of them, rotated differently:

However, if we do as you suggest we could have an easier time drawing the polygon with SVG paths, which could prove easier for debugging in the future.

[TorsteinHonsi]

Here is a stress test, 1000 points, a dataLabel for all of them, rotated differently:

Are those just rotation, or are they text paths? Because those are two different ways of computing the polygon.

Here's my concern, expressed in code: https://jsfiddle.net/highcharts/1kL56s72/

The green lines are the "polygons". In the fiddle, I made the text overlap without any of the polygon verteces crossing each other, which I suspect would count for a false negative.

[MarkusBarstad]

Are those just rotation, or are they text paths? Because those are two different ways of computing the polygon.

Inspecting the dom, you are correct, @TorsteinHonsi , I failed to consider the series type i was using when calling HC with dataLabels!

Here's my concern, expressed in code: https://jsfiddle.net/highcharts/1kL56s72/

The green lines are the "polygons". In the fiddle, I made the text overlap without any of the polygon verteces crossing each other, which I suspect would count for a false negative.

Yes, I see the concern 🤔 I do not think it would be a problem because the pointInPolygon function would not "see" the jagged line, but instead lines between the upper and lower vertices, but it would be nice to test this.

I can try to implement the exact overlapping-function in the demo you posted, then we can see if it registers it indeed as a false negative.

[MarkusBarstad]

Alright, @TorsteinHonsi , seems like a hit is detected in your demo! https://jsfiddle.net/mkb93/xaL6n5zw/86/

This is the exact same function as the one we use.

[TorsteinHonsi]

Thanks! Here's a false positive then: https://jsfiddle.net/highcharts/7852fu9y/ . It probably falsely detects an overlap because text 2 is surrounded by text 1.

I still think this would be avoided if the polygon was a nice fitting line around the text. Maybe the overlapping test must be updated too. It should only report a hit if any two edges of the polygons intersect.

[TorsteinHonsi]

Here's what I meant by part pushing, part unshifting the points:

diff --git a/ts/Core/Renderer/SVG/SVGElement.ts b/ts/Core/Renderer/SVG/SVGElement.ts
index 135d99f851..939f6fb575 100644
--- a/ts/Core/Renderer/SVG/SVGElement.ts
+++ b/ts/Core/Renderer/SVG/SVGElement.ts
@@ -1572,10 +1572,8 @@ class SVGElement implements SVGElementLike {
                             } = tp.getExtentOfChar(i),
                             top = y1 + offsetY,
                             left = x1 + offsetX;
-                        polygon.push(
-                            [left, top],
-                            [left, top + height]
-                        );
+                        polygon.push([left, top]);
+                        polygon.unshift([left, top + height])
                     }
 
                     const { x, y, width, height } = tp.getExtentOfChar(len),
@@ -1583,10 +1581,11 @@ class SVGElement implements SVGElementLike {
                         rightTop = y + offsetY;
 
                     // End of the polygon (vertex order does not matter)
-                    polygon.push(
-                        [rightEdge, rightTop],
-                        [rightEdge, rightTop + height]
-                    );
+                    polygon.push([rightEdge, rightTop]);
+                    polygon.unshift([rightEdge, rightTop + height]);
+
+                    // Close it
+                    polygon.push(polygon[0].slice() as [number, number]);
 
                     bBox.polygon = polygon;
                 }

The result is a polygon that (roughly) follows the perimeter of the text:

With this change, the false positive no longer happens, and as far as I can see the overlap detection works correctly. A hit is detected once the polygons visually overlap.

[MarkusBarstad]

With this change, the false positive no longer happens, and as far as I can see the overlap detection works correctly. A hit is detected once the polygons visually overlap.

Awesome, and good catch on that second false positive, @TorsteinHonsi ! I have implemented the unshifting logic you posted.

Are there any apparent things remaining on this now?

I wish I knew a cheap recipe for more stable polygons which could handle twisting paths better 🤔 As of yet, the best thing I have found is getExtentOfChar and the API for glyph-coordinates does not provide anything obvious.

[TorsteinHonsi]

I wish I knew a cheap recipe for more stable polygons which could handle twisting paths better

Yes, the current algorithm is increasingly wrong the more rotated the glyph is. With vertical text it falls apart:

I think there's one thing we should try. There is a function getStartPositionOfChar. This returns the position of the left bottom (or to be precise, baseline) position of the character. Now if we combine that with SVGRenderer.fontMetrics and the getRotationOfChar, we should be able to compute the correct top position too. For the very last element we would use getEndPositionOfChar instead.

In order to find the true bottom point, apply the baseline returned from fontMetrics.

If this works, the only case I don't see we've covered, is multiline text. It may work out of the box, or we may have to deal with it separately for example to make sure the counter is reset and includes the first and last glyph on each line.

[highsoft-bot]

Webpack monitoring

LGTM :+1:

[TorsteinHonsi]

@MarkusBarstad I see you ended up using the center of getExtentOfChar instead of getStartPositionOfChar. Did you try getStartPositionOfChar?

[MarkusBarstad]

@MarkusBarstad I see you ended up using the center of getExtentOfChar instead of getStartPositionOfChar. Did you try getStartPositionOfChar?

Yes I tried it, @TorsteinHonsi , but I had some issues with it:

Using 'h' from fontMetrics as height and using the rest of getExtentOfChar, is the best solution I have found:

[TorsteinHonsi]

Yes I tried it, @TorsteinHonsi , but I had some issues with it

@MarkusBarstad Okay, something probably went wrong. In the latest commit I applied it, it looks good now.

[MarkusBarstad]

Alright, @TorsteinHonsi I think I am approaching a fix for multi-line ones (fiddle of latest commit): https://jsfiddle.net/mkb93/xaL6n5zw/636/

[TorsteinHonsi]

Thanks, we're closing in on the point when I can't tear this apart anymore. I found two fails now:

1. Text on a curve. This worked correctly before your last one or two commits: https://jsfiddle.net/highcharts/smgczhfy/. But fails now: https://jsfiddle.net/highcharts/smgczhfy/1/
2. Text with no tspan never worked because of the way the query selector is set up: https://jsfiddle.net/highcharts/smgczhfy/2/

Other than that, some things remain before we can merge this:

1. Tests. Without them, we risk creating regressions for things that used to work, like text on a curve above.
2. Final decision on the refactor. As it stands, we are adding 6-700 kB of code into the highcharts.js core for a feature that is useful, but quite fringe. In fact, text paths are not used at all for any of the core features. It is only used by arc diagram, dependency wheel, network graph, packed bubble, sunburst and treegraph. We should consider moving the whole text path stuff into a separate module/composition that would be included by those series type bundles. It would probably save us 1-3 kB of the main bundle. The downside is that those 1-3 kB would be repeated for each the series-type bundles, but it's probably not likely that many of those are loaded in the same page.

[MarkusBarstad]

Thank you for good feedback, @TorsteinHonsi !

I only have one question: To make a polygon surrounding the curved textPath in your fiddle, we have to increase number of character-positions read, right?

Now we read every 5th character, so the polygon does not take the 'T' in your example into account, thus cutting through it.

Should we try every 3rd character instead?

[TorsteinHonsi]

To make a polygon surrounding the curved textPath in your fiddle, we have to increase number of character-positions read, right?

I don't think so. It did work after my last commit, which also used a step of 5. I think this was a regression that was introduced somehow with the multiline support.

[MarkusBarstad]

Alright, @TorsteinHonsi , seems fixed

Trying to make this work for non-tspan wrapped text, I have another issue:

I cannot split its innerHTML

Here I try to:

• Log this.element.querySelector('textPath').innerHTML
• Split the innerHTML on linebreaks ('')

But only tspan wrapped text is split into an array.

I can see that both have innerHTML logged to the console, and the failing split works in jsfiddle.

Any ideas?

[TorsteinHonsi]

Not sure if I understand the question, but by definition all texts with a break also have nested tspans. Only the oneliners have raw text.

[MarkusBarstad]

@TorsteinHonsi

Found an easy solution, should work now: https://jsfiddle.net/mkb93/94vm3er5/24/

Note that this expects text wrapped in span-tags or non-wrapped text. Are there other HTML-tags we should expect in the input?

Also, in regards to making this an extension, should it be part of OverlappingLabels.ts 🤔 ?

[TorsteinHonsi]

Thanks!

Remember to create unit tests as you go along though. Now that we're down to fixing edge cases, it's a high risk that one fix introduces failures in previous cases. If I were you I would stop here and create a battery of tests for the cases we have been through so far (text/label, wrapped/bare content, oneline/multiline).

Also, in regards to making this an extension, should it be part of OverlappingLabels.ts?

No, that module is included in the core. I think an entire new module is needed for everything textPath.

Next egde case: nested spans. Maybe not very relevant, but I think we should consider replacing this block with some generic code that does text.querySelectorAll('tspan'), then do the lines logic separately on all of those. I don't have a clear idea of it though. It may be a problem if a tspan has some raw text content, then a child tspan in addition. You could spend some time on it, but abort mission if it becomes too complicated.

[highsoft-bot]

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

	master	candidate	difference
highcharts.js	95.5 kB 269.4 kB	96.2 kB 271.0 kB	680 B 1597 B
highstock.js	128.4 kB 371.3 kB	129.1 kB 372.9 kB	673 B 1597 B
highmaps.js	121.3 kB 348.6 kB	121.9 kB 350.0 kB	620 B 1457 B
highcharts-gantt.js	130.5 kB 376.4 kB	131.1 kB 377.9 kB	666 B 1597 B

[highsoft-bot]

Visual test results - No difference found

[highsoft-bot]

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

	master	candidate	difference
highcharts.js	95.5 kB 269.4 kB	96.2 kB 271.0 kB	680 B 1597 B
highstock.js	128.4 kB 371.3 kB	129.1 kB 372.9 kB	673 B 1597 B
highmaps.js	121.3 kB 348.6 kB	121.9 kB 350.0 kB	620 B 1457 B
highcharts-gantt.js	130.5 kB 376.4 kB	131.1 kB 377.9 kB	666 B 1597 B

[highsoft-bot]

Visual test results - No difference found