-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Currency Switcher Block flag rendering on Windows platform #6832
Fix Currency Switcher Block flag rendering on Windows platform #6832
Conversation
Check if the browser supports flag emojis before rendering it.
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: +32 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Don't we also list the currencies with flags on the frontend (currency switcher block & widget)? I think removing the flag when it's Windows isn't a solution, maybe we need to find a polyfill or something to fix it. A quick SO search ended up with these solutions: https://stackoverflow.com/questions/62729729/how-do-i-view-country-flags-on-windows-10-through-html |
The main issue is with WP Emoji which replaces the emoji flag with a We are trying to render the flag that is ignored on Windows due to the WP Emoji replacement, and when we toggle the "Display flags" option, it tries to dismount the never mounted flag throwing the fatal error. I only have WP Emoji on the admin dashboard locally, and it renders just the country code in the shop pages on Windows, which would be OK to have on the settings page, but I didn't see a way to disable WP Emoji there. I'll try to find an alternative approach, but for now, I don't see a better way to do it rather than not rendering at all if the emoji flags aren't supported. Thanks for your input @tpaksu! |
@eduardoumpierre I couldn't get a Windows browser to test it, as I don't have a Windows PC anymore (gave away my Windows desktop 😢) maybe someone else should continue reviewing this. |
Thanks, @tpaksu! I reviewed the code from |
That's a good investigation! So, maybe it's something different running on the admin side? |
That's hard to tell because we have the static files with some modifications on our side, but it's definitely not working as it should. It checks each child node and evaluates if it should replace the emojis, but it seems to be checking just the outer nodes. I'll spend some time this week investigating this behavior. |
Thanks! I'll join the investigation if I can spare some time. Looks interesting :) |
@eduardoumpierre I have a Windows 10 desktop I can use to test the flag rendering if needed. Just let me know. |
I think we could explore an alternative approach before proceeding with not rendering the flags at all on Windows, but I'll let you know. Thank you @allie500! |
@eduardoumpierre I have Windows 11 on my personal PC if that needs to be tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for investigating and working on a solution to this issue, Eduardo! The code change is simple and easy to understand. I was able to reproduce the block crashing behavior on my Windows 10 desktop on the develop
branch. The block didn't crash on my Windows 10 desktop with this PR branch checked out.
I also ran through the test on my Mac laptop and Google Pixel with the PR branch checked out. The block didn't crash on either device.
LGTM
@tpaksu I will proceed with merging this PR as it fixes the crashing issue. We can open an issue to investigate the wp-emoji behavior and explore a better solution in the future. |
@eduardoumpierre sure, it would be easier if wp-emoji would really skip rendering on non-supporting platforms. If it's surfaced again, we can discuss it then. Thanks for merging this! |
Fixes #6192
WP Emoji replaces the flag emoji with an image if it's not natively supported by the browser. This behavior is problematic on Windows because it renders an
<img>
tag inside the<option>
, which can lead to crashes.Exception
Changes proposed in this Pull Request
Testing requirement
Testing instructions
Before checking out to this branch
npm run tube:setup
npm run tube:start
Checkout to this branch
npm run tube:stop
npm run changelog
to add a changelog file, choosepatch
to 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