Consolidate Safari Rerenders into one script #49215
Conversation
Previously every duotone selector had its own script tag on the page. This consolidates them into one script tag and loops through all the selectors to apply the hack. Also, previously it used document.querySelector for the rerender, which would only grab the first matched element when there may be many on the page.
| /* | ||
| * Simply accessing el.offsetHeight flushes layout and style | ||
| * Accessing el.offsetHeight flushes layout and style |
There was a problem hiding this comment.
Removed "Simply" for inclusion and brevity sake.
lib/class-wp-duotone-gutenberg.php
Outdated
| '<script> | ||
| ( | ||
| function() { | ||
| %s.forEach( selector => { | ||
| document.querySelectorAll( selector ).forEach( function( el ) { | ||
| if( ! el ) { | ||
| return; | ||
| } | ||
| var display = el.style.display; | ||
| el.style.display = "none"; | ||
| el.offsetHeight; | ||
| el.style.display = display; | ||
| } ); | ||
| } ); | ||
| } | ||
| )(); | ||
| </script>', | ||
| wp_json_encode( $selectors ) |
There was a problem hiding this comment.
II think this will be easier to review if we leave it expanded, and then instead of printf we can use remove the whitespace when we output it with [normalize_whitespace](https://developer.wordpress.org/reference/functions/normalize_whitespace/) or similar.
There was a problem hiding this comment.
I don't think normalize_whitespace will work, I just used preg_replace.
|
|
||
| global $is_safari; | ||
| if ( $is_safari && ! empty( $selectors ) ) { | ||
| self::safari_rerender_hack( $selectors ); |
There was a problem hiding this comment.
I considered using array_map or array_columns here, but collecting $selectors in the foreach is probably easier to grok.
|
I think this is looking good, but even when I removed the code altogether it still works in Safari, so I think we need better testing instructions! Maybe @ajlende knows... |
|
Flaky tests detected in 5b81ec9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4478040074
|
|
@scruffian Safari may have fixed their bug. I have a codepen that I used for testing. This is what I was seeing before.
And this is what I'm seeing now.
Do you know how many version of Safari we're supporting? If it's fixed in all of them, we can get rid of this code! |
|
@scruffian and I see that we're supporting the |
|
Woo! Nice! I should have checked that first! Great catch. |
Fixes #49170.
Why?
Previously every duotone selector had its own script tag on the page. This consolidates them into one script tag and loops through all the selectors to apply the hack. Also, previously it used document.querySelector for the rerender, which would only grab the first matched element when there may be many on the page.
How?
Collects all the selectors and the JS can now handle an array of strings rather than only one at a time.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast