Skip to content
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

Ensure commands work in the new iframed Block Editor #105

Merged
merged 18 commits into from
Aug 25, 2023
Merged

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Aug 21, 2023

Description of the Change

In WordPress 6.3, the Block Editor now loads in an iframe by default. Any commands that interact with the Block Editor will now fail in that state, so this PR updates those commands to utilize the iframe element, if it exists.

A new getBlockEditor command has been added that will return either the main body element or the iframe element, depending on which one exists. We then utilize this a few places to ensure we're targeting the right element.

We also set chromeWebSecurity to false, which allows Cypress to load the iframe. Important to note that for any project using this tool, they'll probably need to set that config value themselves as well.

Finally we update the versions of WordPress we run E2E tests on to ensure we're testing 6.3

Closes #104, #91, #101

How to test the Change

Ensure E2E tests are passing.

In addition, feel free to pull these changes into other projects and ensure E2E tests for those projects pass as well.

Changelog Entry

Added - New command, getBlockEditor, which returns the proper editor element.
Changed - Update the versions of WordPress we test on to include 6.3
Fixed - Ensure all E2E tests pass when running on WordPress 6.3

Credits

Props @dkotter, @iamdharmesh, @TylerB24890

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@dkotter dkotter self-assigned this Aug 21, 2023
README.md Outdated Show resolved Hide resolved
@dkotter
Copy link
Collaborator Author

dkotter commented Aug 22, 2023

@iamdharmesh I'm working to ensure all our commands work properly in 6.3, mostly using what you did on Autoshare for Twitter (thanks!). I have all tests passing now besides a couple tests around inserting blocks. I believe we're running into the issue I documented before (#91). In doing some debugging, it seems that opening the block inserter works fine, searching and inserting a block works fine but as soon as we close the block inserter, a JS error happens that causes the test to fail.

This JS error comes from WordPress itself but I'm assuming it has to do with how we're interacting with that inserter. I've tried multiple things to get that working but no luck so far. If you have some time over the next week or so, mind taking a look at this?

Edit: a little additional information, seems to only fail when inserting a paragraph or heading blocks. The other tests we run are passing, so not sure what the difference is there.

iamdharmesh
iamdharmesh previously approved these changes Aug 23, 2023
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkotter Thank you for updating the block editor commands and making it work. I checked on the error and figured out that it happening due to 2 errors (CORS error in getting an image from https://pd.w.org/ and removeEventHander error (Which may be due to the first one).

We have the chromeWebSecurity option to handle this kind of CORS error in Chrome browser but it not works with other browsers as mentioned here and here. Also, I didn't found any other way make this work. I have updated the script to run tests on the Chrome browser and it's working fine.

I think we should proceed here by opening an issue for check and making this work in other browsers later(hopefully, we will figure out some way meanwhile). What do you think?

Thanks

@dkotter dkotter marked this pull request as ready for review August 23, 2023 13:43
@dkotter dkotter requested review from jeffpaul and a team as code owners August 23, 2023 13:43
@dkotter
Copy link
Collaborator Author

dkotter commented Aug 23, 2023

I have updated the script to run tests on the Chrome browser and it's working fine.

My main concern here is this will get our E2E tests to work but anyone using this library will have to make the same change in their tests (worth noting that in my testing, problems only exist in WordPress 6.2+ and only if using the insertBlock command), which we could just document that in the README but would be great if things work without any additional config changes.

Your comment about the CORS error with the image pointed me in the right direction though. I was curious why things fail with the Paragraph block and Heading block but not with the other blocks we test (Twitter, Pull Quote, WinAmp) and in testing, found out that the latter three blocks have no patterns associated with them, so this led me to believe that the Block Patterns were causing issues here.

In searching on how to disable these, everything points to a couple PHP approaches:

add_action(
	'after_setup_theme',
	function() {
		remove_theme_support( 'core-block-patterns' );
	}
);
add_filter( 'should_load_remote_block_patterns', '__return_false' );

I tried both of these separately and they both fixed the issue. The only problem here is same as the above, we can add this to our custom plugin to get our E2E tests to pass but other plugins using this library would also have to do the same. So I went down the path of trying to figure out how to remove patterns using JS only.

I ended up finding an approach that uses __experimentalBlockPatterns, which I don't love having to use experimental properties, but that does seem to fix things and should in theory mean this will work for other projects that use this library with no additional config changes.

@dkotter dkotter removed request for a team and jeffpaul August 23, 2023 20:35
@dkotter dkotter added this to the 0.2.0 milestone Aug 23, 2023
…e we need from that into our utils. This means other projects don't have to load that same dependency for things to work
@dkotter
Copy link
Collaborator Author

dkotter commented Aug 24, 2023

@iamdharmesh Sorry, one more change here. Was testing these changes yesterday on ClassifAI to ensure they would fix things there, realized that this package doesn't include any other packages in the final release. So because we use the Cypress iframe package, you need to install that in each project now in order to use the utils, which I don't love.

Looking at that iframe package, it's fairly simple and hasn't been updated in 3 years, so doesn't appear to be maintained. I've decided to copy over the code we need instead of loading that as a dependency. This ensures we don't have to load this dependency on other projects and things should mostly just work out of the box. Let me know if you have any concerns with this though.

@iamdharmesh
Copy link
Member

I ended up finding an approach that uses __experimentalBlockPatterns, which I don't love having to use experimental properties, but that does seem to fix things and should in theory mean this will work for other projects that use this library with no additional config changes.

This seems the best option we have as of now. Thanks a lot for checking on this and figuring out the solution that will work for other projects with no additional config changes

Was testing these changes yesterday on ClassifAI to ensure they would fix things there, realized that this package doesn't include any other packages in the final release. So because we use the Cypress iframe package, you need to install that in each project now in order to use the utils, which I don't love.

After digging into this I think the root cause for the cypress-iframe package not being included in the final release is due to it was added as devDependencies. Adding it into dependencies may solve the issue here and we don't have to install it on each project. But as you mentioned cypress-iframe package is fairly simple and hasn't been updated in 3 years, Making it part of this repo by copying the code makes more sense here as it gives us direct control over code.

@dkotter Thanks again for digging into this and fixing the issues in the best possible way. We are good to go here. 🚀

@dkotter dkotter merged commit 1ae38a2 into develop Aug 25, 2023
@dkotter dkotter deleted the fix/104 branch August 25, 2023 14:51
github-actions bot pushed a commit that referenced this pull request Aug 25, 2023
Ensure commands work in the new iframed Block Editor
@dkotter dkotter mentioned this pull request Aug 29, 2023
4 tasks
@iamdharmesh iamdharmesh linked an issue Aug 30, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment