Fix broken "Re-enable transient caching" button for "Transient caching of parsed stylesheets is disabled" site health test #6552
Conversation
|
Plugin builds for 1edd22a are ready 🛎️!
|
| @@ -84,7 +85,7 @@ public function register_ajax_script( $hook_suffix ) { | |||
| } ); | |||
| } ); | |||
| } ); | |||
| } ); | |||
| }, 1000 ); | |||
There was a problem hiding this comment.
It's an estimation to give a bit of leeway for all direct site health tests to be added to the page. One alternative could be to add a setTimeout to keep checking for when this specific site health test gets added to the page, but that seems like a waste of CPU cycles.
There was a problem hiding this comment.
But the test is not async so why would it not be on the page initially?
| window.addEventListener( 'DOMContentLoaded', ( event ) => { | ||
| var selector = SELECTOR; | ||
| var selector = SELECTOR; | ||
| setTimeout( function () { |
There was a problem hiding this comment.
Is the problem rather that the DOMContentLoaded event is being added to the window as opposed to the document? Or what if the load event on the window were used instead?
There was a problem hiding this comment.
The problem is that the DOMContentLoaded may fire before the site health test is added to the page. I guess the button was working before because the site health test was being added just in time before this event listener fires, and now that there are direct tests being added before this test, the event listener now fires before the site health test is added to the page.
There was a problem hiding this comment.
Switching this instead to run at the load event of window should do the trick, since it always runs after all DOMContentLoaded event handlers have run.
There was a problem hiding this comment.
The issue lies between the time the site health test DOM element is added to the page and when our JS executes. Both DOMContentLoaded and load events may have already passed by the time the site health is added to the page unfortunately.
There was a problem hiding this comment.
It looks like that code is running in jQuery( function( $ ) {}) which is the same as DOMContentLoaded. It doesn't work when switching to use the load event on window?
There was a problem hiding this comment.
Hmm it works in Chrome but not Firefox. The button is still being inserted after the load event handler callback executes.
There was a problem hiding this comment.
Yeah, I was getting inconsistent results as well. I think any timeout is going to be problematic and that a different approach is needed that doesn't rely on the element being in the DOM. Namely, this should have used event delegation from the start. Please try again after c577735.
| <script> | ||
| (( $, { selector, action, postArgs } ) => { | ||
| document.addEventListener( 'DOMContentLoaded', () => { | ||
| $( '.health-check-body' ).on( 'click', selector, ( event ) => { |
There was a problem hiding this comment.
Oh cool workaround! And TIL you could pass a selector to a jQuery event handler to filter the element that triggered the event.
There was a problem hiding this comment.
My stoneage JS knowledge still holds some surprises I guess. 😊
|
Everything seems to be good here, giving my 👍. |
|
The reason for 1edd22a is that I found that even with the Site Health plugin active, I could access both versions of the page: |
…g of parsed stylesheets is disabled" site health test (#6552) Co-authored-by: Pierre Gordon <[email protected]>
Summary
Revert to using a timeout to set the button functionality as originally done by @schlessera (see 5cbe86b). This gives the Site Health screen some time to add all the direct tests so that we can then add the necessary callback to the button.
This PR also ensures the button works when using the Site Health plugin.
Fixes #6549
Checklist