Fix iframe stylesheets cloning for Firefox #38049
Conversation
github-actions
bot
added
the
First-time Contributor
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @miguelpeixe! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
skorasaurus
added
the
Browser Issues
| doc.head.appendChild( inlineCssElement.cloneNode( true ) ); | ||
| } | ||
| } | ||
| node.addEventListener( 'load', appendStyles ); |
There was a problem hiding this comment.
Is the fix to move appending to the load event?
There was a problem hiding this comment.
Can we not simply move the whole effect into the load handler?
There was a problem hiding this comment.
It is, I propose using useRefEffect() because it's the common strategy for this component DOM manipulation. setRef() is also aware of Firefox iframe behavior.
This PR was missing the same type of check, which I added in 6d6553f.
|
|
||
| doc.head.appendChild( ownerNode.cloneNode( true ) ); | ||
| if ( isMatch ) { |
There was a problem hiding this comment.
The head.appendChild() was moved to the appendStyles() function. The idea is that the initial stylesheets iteration clones and pushes the node to the clonedStyles array. This way the appendStyles() function can also detect the node through doc.contains( style ) and be dedicated to only append the style at the appropriate time.
There was a problem hiding this comment.
The initial stylesheets iteration and cloning don't have to be inside the effect, 0cef19c moves it to the upper level for better performance.
| } | ||
|
|
||
| const { ownerNode, cssRules } = styleSheet; | ||
| const useEditorStyles = () => |
There was a problem hiding this comment.
Minor: it's better to use named functions for debugging.
There was a problem hiding this comment.
Can we also document that this is a compatibility layer, if we remove compat from the name?
There was a problem hiding this comment.
Thank you for reviewing! Updated in e61ecab
There was a problem hiding this comment.
I missed another arrow function, fixed in e752a13
| } | ||
|
|
||
| node.addEventListener( 'load', appendStyles ); | ||
| } ); |
There was a problem hiding this comment.
The dependencies are missing for this hook
There was a problem hiding this comment.
I'm sorry, I don't understand which dependencies you are referring to.
| // Search the document for stylesheets targetting the editor canvas. | ||
| const clonedStyles = []; | ||
| Array.from( document.styleSheets ).forEach( ( styleSheet ) => { |
There was a problem hiding this comment.
Can we move all this into useRefEffect? It would also make it much easier to see the essence of this PR.
There was a problem hiding this comment.
We can, but I believe it'd be less performant. The iteration through the root document styles doesn't have to be executed on changes to the iframe node.
| @@ -74,21 +73,49 @@ function styleSheetsCompat( doc ) { | |||
| selectorText.includes( `.${ BLOCK_PREFIX }` ) ) | |||
| ); | |||
|
|
|||
| if ( isMatch && ! doc.getElementById( ownerNode.id ) ) { | |||
There was a problem hiding this comment.
What's the reason for removing the second check?
There was a problem hiding this comment.
doc does not exist at this point. This initial iteration through root document styles is dedicated to matching and cloning the nodes to the clonedStyles variable. Please observe lines 100-102 of the PR, in which the checks are made in order to append the style.
|
I believe this was fixed in #40842. Thank you for your contribution! |
Description
This PR changes the
styleSheetsCompat()strategy for stylesheets cloning into iframes to useuseRefEffect()in order to fix Firefox compatibility.Closes #37961
How has this been tested?
Using the sample plugin provided in #37961, compare the result of the post editors' Patterns list in Chrome and Firefox.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).