From 69196362a3903f7b6f9cc1b4667fc67965edb934 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 c86d175ed..48cc5fbea 100644 --- a/js/display/drawables/TextSVGDrawable.js +++ b/js/display/drawables/TextSVGDrawable.js @@ -18,6 +18,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 @@ -35,8 +44,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( '' ) ); @@ -55,7 +81,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 ) { @@ -87,6 +113,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.