Fix React error when the Express Checkout's container element is not found. #9420
Conversation
Avoids printing the HTML DIV element using the `the_content` filter as it’s not secure to always be called. Templated pages don’t run this filter therefore the script to detect the payment method availability will fail.
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +15 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
asumaran
changed the title
There was a problem hiding this comment.
I've tested this fix with the original Blocks Canvas theme provided over Slack, the Tsubaki theme, the provided What Theme, and Storefront. I tested these on Chrome, Safari, and Firefox (to confirm that the Express Checkout box gets properly hidden still). And I checked on product, cart, and checkout pages. I didn't see any issues in my testing and confirmed both CC, GPay, and Apple Pay all worked to complete the checkout. I'll note that all this testing happened on a JN site as well.
I had one small note about spelling and I'd like to get at least one other approval and review on both this approach and confirmation of it working as expected across different environments.
|
@senadir @alexflorisca @ralucaStan tagging you all for awareness in case this scenario comes up with other 3P gateways that are implementing express checkout buttons. Please feel free to sanity check this solution. Thank you. |
There was a problem hiding this comment.
@asumaran we should probably rebase this PR from trunk so it's easier for the release DRI to include in the patch release.
Review: Tested this in Safari and Chrome with Storefront, What Theme and Block Canvas.
- ✅ Reproduced the bug:
- ✅ Confirmed the fix:
| // Create the DIV container on the fly | ||
| const containerEl = document.createElement( 'div' ); | ||
|
|
||
| // Ensure the element is hidden and doesn’t interfere with the page layout. | ||
| containerEl.style.border = 0; | ||
| containerEl.style.height = '0'; | ||
| containerEl.style.margin = '0'; | ||
| containerEl.style.overflow = 'hidden'; | ||
| containerEl.style.padding = '0'; | ||
| containerEl.style.position = 'absolute'; | ||
| containerEl.style.width = '0'; | ||
| containerEl.style.float = 'left'; | ||
| containerEl.style.opacity = '0'; | ||
| containerEl.style.pointerEvents = 'none'; | ||
|
|
||
| document.querySelector( 'body' ).appendChild( containerEl ); | ||
|
|
||
| const root = ReactDOM.createRoot( containerEl ); |
There was a problem hiding this comment.
I also tested this with only:
containerEl.style.display = 'none';And it worked as expected. With display: none, the element shouldn't have any pointer events and shouldn't take up any space.
We might not need to keep the element technically visible by setting other properties to 0, but perhaps I could be missing something?
The current approach also LGTM, the only downside is that it's slightly more complex and the element is still "visible" in the DOM, including for assistive technologies, although temporarily.
cc @bborman22
There was a problem hiding this comment.
I guess it doesn't matter if the button doesn't end up rendering as long as we have the onReady event to be called with the info we need.
There was a problem hiding this comment.
the only downside is that it's slightly more complex and the element is still "visible" in the DOM, including for assistive technologies
Great catch! I missed the ariaHidden not being set. I think at a minimum that should be set. Display none is certainly simpler, just wondering if there are some gotchas there as we don't use it in other places we create a hidden div? ref1 ref2 with the second ref using an iframe as one possible explanation.
There was a problem hiding this comment.
I believe it would depend mostly on Stripe, e.g. if it checks for any layout measurements of the root element, or if the contents are technically visible.
Both approaches seem to work though, so I don't have a strong preference, but I agree with @bborman22 we should probably set containerEl.setAttribute( 'aria-hidden', 'true' ); if not using display: none.
There was a problem hiding this comment.
I've updated the PR with the suggestion. 70191f7 – I've tested with regular and block-based theme. With Safari, Google Chrome, and Firefox.
There was a problem hiding this comment.
I ran through a similar set of tests as before and found no issues with the display none approach! I also gave some additional testing to the shortcode checkout as well with no issues.
LGTM! Again just want to get two approvals on this with the new approach as well.
Fixes #9419
Changes proposed in this Pull Request
Since the
the_contentfilter is never triggered when the checkout page is assigned a block-based template, the elements we were printing as containers for the buttons used to check the availability of ApplePay/GooglePay payment methods were not rendered. As a result, React failed to mount the button because the containers didn’t exist.We’re changing the approach by creating the container dynamically and appending it to the page using JavaScript. This ensures the buttons are rendered in both cases, whether using a standard theme or a block-based theme.
Why is it happening?
When a page uses a block-based template, it includes the
template-canvas.phpfile as base (ref), which prints the HTML generated from the block template. Notably, this file never callsthe_content(). (This the block template the site that reported the issue was using). – As side note, @reykjalin' fix works because this file do callwp_footer(), which applies thewp_footeraction (ref).However, when using a regular theme, the checkout block is rendered as part of the page content. In themes, the page content is printed using
the_content()(ref), which applies thethe_contentfilter (ref).Unfortunately, to render the elements that would be used as containers for the buttons used to check the availability of payment methods, this
the_contentfilter was used, which is not always called. We should have used thewp_footeraction or perhaps used JS to create the element.Testing instructions
To reproduce the bug
developbranch.[woocommerce_checkout]using the shortcode block.page-checkouttemplate to it.To confirm the fix
as-fix-react-error-container-not-foundnpm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge