Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

• WordPress.com Simple releases happen daily.
• WoA releases happen weekly.
• Releases to self-hosted sites happen monthly. The next release is scheduled for March 5, 2024 (scheduled code freeze on March 4, 2024).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

• To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the rnmobile/ensure-playsinline-setting-confirms-to-core-video branch.

• To test on Simple, run the following command on your sandbox:

  bin/jetpack-downloader test jetpack rnmobile/ensure-playsinline-setting-confirms-to-core-video
  

Interested in more tips and information?

• In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
• Read more about our development workflow here: PCYsg-eg0-p2
• Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@Automattic/jetpack-crew, as we're proposing a change to web file here, I'd be grateful for your review. 🙇‍♀️

I tried testing this and had slightly unexpected results. Not sure if I'm testing it right, though.
I accessed the post in the app and toggled the "inline" setting on, saved the post, and exited the editor.
When I opened that post on the web, the inline toggle was on, but whenever I tried leaving the post (without making any changes to it), it warned me about unsaved changes. The unsaved change appeared to be turning the inline toggle off.

Can you reproduce this behavior?

However, I noticed that blocks created previous to this change result in a block validation error.

I presume that we shouldn't change the block's markup without adding a deprecation to the block.

Ah, interesting, I was testing for that but didn't run into the error. We should definitely add deprecation handling then 👍

However, I noticed that blocks created previous to this change result in a block validation error.
I presume that we shouldn't change the block's markup without adding a deprecation to the block.

@fluiddot, were those blocks created based on the changes in #35637? If so, those changes aren't yet shipped to users, so I'm wondering if there would be a case where users would actually run into what you've described. 🤔

@fluiddot, were those blocks created based on the changes in #35637? [...]

Yes, the blocks I tested were created based on the changes in #35637.

[...] If so, those changes aren't yet shipped to users, so I'm wondering if there would be a case where users would actually run into what you've described. 🤔

Good point. If this change will be released along with #35637, then what I mentioned shouldn't be an issue. Tomorrow we are releasing a new Gutenberg Mobile version (which will include #35637), so if we merge this today we'll be on time to have both. That said, I'll proceed with approving the PR, thanks 🙇 !

@bindlegirl, thank you for testing!

I tried testing this and had slightly unexpected results. Not sure if I'm testing it right, though.
I accessed the post in the app and toggled the "inline" setting on, saved the post, and exited the editor.
When I opened that post on the web, the inline toggle was on, but whenever I tried leaving the post (without making any changes to it), it warned me about unsaved changes. The unsaved change appeared to be turning the inline toggle off.

Were you testing the app locally (with these changes applied) or with a production build? I tested with these changes checked out in both the app and web, but wasn't able to reproduce what you're describing:

I'll go ahead to merge based on the above and then incorporate this into Gutenberg Mobile for tomorrow's release.

@bindlegirl let me know if you're able to consistently reproduce the issue you were running into with the local changes from the app (or when this is released in 24.4). I'll also be keeping an eye out for reports when the change is released to users. 🙇‍♀️

@SiobhyB This is what I see when testing on web (the inline toggle is set to off when reopening the post):

As far as I can tell, saving in app, and opening in web editor works as expected. It could be a problem with my testing environment if you are unable to reproduce it. Or with my testing procedure. 🙂

@bindlegirl, ah I see, thank you for elaborating! My testing was focused on the app, if I check now I can confirm I can reproduce what you're seeing on Atomic sites that have VideoPress enabled on the web.

I'll create a separate PR to revert these changes.

I've got the revert PR up in #36030.

@bindlegirl, thank you for spotting! I was eager to get a potential fix into the app's next release, which is why I kind of rushed merging, but should have looked more carefully into your first comment. Thank you again for following up!

We'll have to come up with another solution for the app bug but the setting has always been broken on mobile, so this isn't a regression for us. We can wait to address in another release.

Good catch @bindlegirl! I checked the block's attributes in the case shared #35981 (comment), and noticed that the block has both playsInline and playsinline attributes. In this PR, we considered that Plays inline setting would always be controlled by playsinline attribute of VideoPress v5. However, since it can have both, we should take this into account and determine which one should be used. Here's a proposal for addressing it:

diff --git forkSrcPrefix/projects/plugins/jetpack/extensions/blocks/videopress/save.js forkDstPrefix/projects/plugins/jetpack/extensions/blocks/videopress/save.js
index a731b3ea65c298e0ecf9a99014436e16dcd89fb8..65e9c17e614ac1ae04eb8eef9fcb1529ab57f8bb 100644
--- forkSrcPrefix/projects/plugins/jetpack/extensions/blocks/videopress/save.js
+++ forkDstPrefix/projects/plugins/jetpack/extensions/blocks/videopress/save.js
@@ -42,9 +42,18 @@ const VideoPressSave = CoreVideoSave => props => {
 		 * @see https://github.com/WordPress/gutenberg/blob/c5f9bd88125282a0c35f887cc8d835f065893112/packages/editor/src/hooks/generated-class-name.js#L42
 		 * @see https://github.com/Automattic/wp-calypso/pull/30546#issuecomment-463637946
 		 */
-		// Rename `playsinline` to `playsInline` to conform the block schema of core Video block.
-		const { playsinline: videoPressPlayinline, ...restAttributes } = props.attributes;
-		const coreVideoAttributes = { ...restAttributes, playsInline: playsinline };
+		// Due to different naming for the Plays inline setting between this block and core Video block, and potentially
+		// having both defined in block's attributes. We need to determine which value should be used and ensure we use
+		// the `playsInline` attribute defined by the core Video block, as it's the block that ultimately will be rendered.
+		const {
+			playsinline: videoPressPlayinline,
+			playsInline: corePlayinline,
+			...restAttributes
+		} = props.attributes;
+		const coreVideoAttributes = {
+			...restAttributes,
+			playsInline: corePlayinline || videoPressPlayinline,
+		};
 
 		return CoreVideoSave( {
 			...props,

cc @SiobhyB

@fluiddot, the solution you proposed seems promising! I would be happy to help review if anyone picks up a PR. I'm working on the release today ahead of my support rotation and, as this isn't regression, won't prioritise working on it myself.