From 56d50d9f086dd6c6508a4671e3cd8268c16abaa8 Mon Sep 17 00:00:00 2001 From: Jonathan Olson Date: Wed, 28 Feb 2024 13:42:37 -0700 Subject: [PATCH] Safari SVG workaround for https://github.com/phetsims/scenery/issues/1610, https://github.com/phetsims/qa/issues/1039#issuecomment-1949196606 --- js/display/drawables/TextSVGDrawable.js | 45 +++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/js/display/drawables/TextSVGDrawable.js b/js/display/drawables/TextSVGDrawable.js index 25c5439ac..eb8e38386 100644 --- a/js/display/drawables/TextSVGDrawable.js +++ b/js/display/drawables/TextSVGDrawable.js @@ -22,6 +22,15 @@ const keepSVGTextElements = true; // whether we should pool SVG elements for the // See https://github.com/phetsims/scenery/issues/455 for more information. const useSVGTextLengthAdjustments = !platform.edge; +// Safari seems to have many issues with text and repaint regions, resulting in artifacts showing up when not correctly +// repainted (https://github.com/phetsims/qa/issues/1039#issuecomment-1949196606), and +// cutting off some portions of the text (https://github.com/phetsims/scenery/issues/1610). +// We have persistently created "transparent" rectangles to force repaints (requiring the client to do so), but this +// seems to not work in many cases, and seems to be a usability issue to have to add workarounds. +// If we place it in the same SVG group as the text, we'll get the same transform, but it seems to provide a consistent +// workaround. +const useTransparentSVGTextWorkaround = platform.safari; + class TextSVGDrawable extends TextStatefulDrawable( SVGSelfDrawable ) { /** * @public @@ -39,8 +48,25 @@ class TextSVGDrawable extends TextStatefulDrawable( SVGSelfDrawable ) { if ( !this.svgElement ) { const text = document.createElementNS( svgns, 'text' ); - // @protected {SVGTextElement} - Sole SVG element for this drawable, implementing API for SVGSelfDrawable - this.svgElement = text; + // @private {SVGTextElement} + this.text = text; + + // If we're applying the workaround, we'll nest everything under a group element + if ( useTransparentSVGTextWorkaround ) { + const group = document.createElementNS( svgns, 'g' ); + group.appendChild( text ); + + this.svgElement = group; + + // "transparent" fill seems to trick Safari into repainting the region correctly. + this.workaroundRect = document.createElementNS( svgns, 'rect' ); + this.workaroundRect.setAttribute( 'fill', 'transparent' ); + group.appendChild( this.workaroundRect ); + } + else { + // @protected {SVGTextElement|SVGGroup} - Sole SVG element for this drawable, implementing API for SVGSelfDrawable + this.svgElement = text; + } text.appendChild( document.createTextNode( '' ) ); @@ -59,7 +85,7 @@ class TextSVGDrawable extends TextStatefulDrawable( SVGSelfDrawable ) { * Implements the interface for SVGSelfDrawable (and is called from the SVGSelfDrawable's update). */ updateSVGSelf() { - const text = this.svgElement; + const text = this.text; // set all of the font attributes, since we can't use the combined one if ( this.dirtyFont ) { @@ -91,6 +117,19 @@ class TextSVGDrawable extends TextStatefulDrawable( SVGSelfDrawable ) { text.removeAttribute( 'lengthAdjust' ); text.removeAttribute( 'textLength' ); } + + if ( useTransparentSVGTextWorkaround ) { + // Since text can get bigger/smaller, lets make the region larger than the "reported" bounds - this is needed + // for the usually-problematic locales that have glyphs that extend well past the normal browser-reported + // bounds. Since this is transparent, we can make it larger than the actual bounds. + const paddingRatio = 0.2; + const horizontalPadding = this.node.selfBounds.width * paddingRatio; + const verticalPadding = this.node.selfBounds.height * paddingRatio; + this.workaroundRect.setAttribute( 'x', this.node.selfBounds.minX - horizontalPadding ); + this.workaroundRect.setAttribute( 'y', this.node.selfBounds.minY - verticalPadding ); + this.workaroundRect.setAttribute( 'width', this.node.selfBounds.width + 2 * horizontalPadding ); + this.workaroundRect.setAttribute( 'height', this.node.selfBounds.height + 2 * verticalPadding ); + } } // Apply any fill/stroke changes to our element.