fix(@visx/text) Bad size measurements in Firefox

[vovakulikov]

🐛 Bug Fix

• Fixes: [@visx/annotation] Label component has incorrect height in Firefox #1111

I don't have a clear understanding of this but it seems like react-use-measure hook doesn't work well with inner svg element within .

I consider this problem came from how Firefox treats empty svg elements without width and height within another svg element with height and width - They usually have width=100% height=100% of parent size even if we put size value from CSS width: 0px; height: 0px; explicitly.

In the first render we're getting a really big value of Text svg height, so we're just stuck with this big initial height value here and therefore have this visual behaviour here.

Possible solution in this PR this is replace innerRef target from svg <Text /> root element to <text /> element which can be measured without this problem.

Only one concern here this is kind of breaking change in public API types (before we had innerRef: Ref<SVGSvgElement>, now we have Ref<SVGTextElement>). I see here two option how to avoid breaking change in types

• Leave public types API as it is and add casting to text ref element <text ref={innerRef as Ref<SVGTextElement>} />
• Add new prop - textInnerRef withRef<SVGTextElement> and use this prop to measure size at annotation <Label /> instead of use old innerRef
  

  visx/packages/visx-annotation/src/components/Label.tsx

  Lines 225 to 235 in d5fb82e

  	<Text
  	innerRef={titleRef}
  	fill={fontColor}
  	verticalAnchor="start"
  	x={padding.left + (titleProps?.textAnchor === 'middle' ? innerWidth / 2 : 0)}
  	y={padding.top}
  	width={innerWidth}
  	capHeight={titleFontSize} // capHeight should match fontSize, used for first line line height
  	style={titleStyle} // used for size calculation
  	{...titleProps}
  	>

[williaster]

Hey @vovakulikov thanks for really digging into the cause of the issue and the PR 🙏 One issue with this implementation is that there would be n refs, one for each line of text. I think that would likely deviate from the expected behavior on the consumer side since they'd only pass in one innerRef function.

If this is truly a bug with react-use-measure I'm wondering if we can file an issue there since otherwise we'd probably need to hack around it here. I made a simple sandbox to confirm what you described separate from visx

https://codesandbox.io/s/priceless-margulis-nctit?file=/index.tsx

chrome

firefox

[vovakulikov]

@williaster Thanks for making this demo about svg and Firefox, this is exactly what I meant.

One issue with this implementation is that there would be n refs, one for each line of text

But we render just one text element within <Text /> and use tspan for text lines, no? So it wouldn't be a problem since we still have one ref for <Text />

visx/packages/visx-text/src/Text.tsx

Lines 31 to 37 in d5fb82e

	<text transform={transform} {...textProps} textAnchor={textAnchor}>
	{wordsByLines.map((line, index) => (
	<tspan key={index} x={x} dy={index === 0 ? startDy : lineHeight}>
	{line.words.join(' ')}
	</tspan>
	))}
	</text>

[williaster]

But we render just one text element within and use tspan for text lines, no?

Ah yes I totally misread the condition on that line, sorry. In this case maybe this approach will work fine! 🎉 I might still post an issue in react-use-measure to ask about it.

as far as avoiding breaking changes for this change, I think I'd probably vote for this option rather than casting to the wrong type

Add new prop - textInnerRef withRef and use this prop to measure size at annotation instead of use old innerRef

Sorry it's so challenging to avoid breaking changes, have to be creative sometimes 😅

[vovakulikov]

@williaster Totally with you about create issue in react-use-measure. For now I've added new prop to get ref of text element which was discussed above. Thanks for reviewing this btw.

[williaster]

Thanks again @vovakulikov 🙏 This looks great ❤️

[github-actions]

🎉 This PR is included in version v1.10.0 of the packages modified 🎉