-
Notifications
You must be signed in to change notification settings - Fork 225
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
Update DelayJS to v2 #7092
Comments
I found this PR to merge v2 into develop then we can merge develop into main and release from main but I can see some conflicts, let me check if we can resolve the conflicts from our side or we will need to check with Gael. |
@wordpressfan This has already been discussed with Gael here: https://wp-media.slack.com/archives/CUT7FLHF1/p1729614432850799?thread_ts=1729184313.104719&cid=CUT7FLHF1 |
Awesome, so we will ignore conflicts and keep the v2 changes. |
Waiting feedback for possible regression https://wp-media.slack.com/archives/CUT7FLHF1/p1731051322877729?thread_ts=1729184313.104719&cid=CUT7FLHF1 |
Regression list
non-regression
|
CC @gmetais |
Hi @Mai-Saad, Could you please describe this issue a bit more?
What is a saved tab? Is it related to the fact that DelayJS intentionally loads scripts when the page visibility is hidden? |
@gmetais Here is a video about the saved tab (usually scripts aren't delayed there) |
@gmetais Were you able to take a look at this? |
@piotrbak Not yet, I will investigate deeper all the issues reported above this Friday. (As far as I see, none of them look hard to solve. I'm confident for the moment. 🤞) |
Hi @Mai-Saad, A fix for
was merged into DelayJS |
Also please confirm it fixes this:
|
Regarding
I have contacted Hanna in private to provide a testing env. |
DelayJS is currently disabled on the page URL in your video. I've tried with some other pages but couldn't reproduce. I'd be interested in having the environment set up fine, for me to investigate the issue. |
@gmetais Any updates about this one? 🙏 |
Hi @Mai-Saad, |
@Mai-Saad, @hanna-meda, @piotrbak, I have a fix for the issue "double tap needed for menu to open with certain template", I pushed it on the following branch: https://github.com/wp-media/delay-javascript-loading/tree/fix-ios-click Some quick information related to this bug. Safari has a really strange behavior that seems to prevent the first The fix I found is as weird as the bug itself. Adding an I note that some developers had already found some similarly strange fixes for the iOS double click issue:
I will try to find the deeper reason for this bug, but in the meanwhile I think that the version published on the above branch is a good candidate for production (the change is low-risky). Could you please test on your side? |
@gmetais, thank you once again. This is indeed a capricious bug! Below are the testing results: Tested on BrowserStack (real devices): Tested on personal device: Conclusion: [Later edit] |
Thanks a lot @hanna-meda, I still don't understand which change in DelayJS v2 introduced this regression, but it definitely looks like our old double click issue. I've started working on a fix based on the idea here #3142 (comment), but with additional securities to prevent clicks being triggered twice on websites that don't experience the bug. I will finish in January. |
Hi @hanna-meda, I've updated the same branch (https://github.com/wp-media/delay-javascript-loading/tree/fix-ios-click) with a new approach for the fix and it is working fine on my test page and my test device. Do you mind making a new round of tests with the updated code please? |
DelayJS v2 must be released on WP Rocket.
The "v2" is currently available on the "v2" branch of the delay JS repo.
Here is the process to automatically update WP Rocket with a DelayJS release: https://www.notion.so/wpmedia/Maintaining-JavaScript-files-98e4112922d540da93e2536866e36fd8?pvs=4#95bb73a9447745228d1c6376e5ad3732
The text was updated successfully, but these errors were encountered: