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

Gallery block: Determine galleryWithImageBlocks prop value based on the WordPress version of the site when value is unknown #47782

Closed
fluiddot opened this issue Feb 6, 2023 · 6 comments · Fixed by wordpress-mobile/WordPress-iOS#20736
Assignees
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended

Comments

@fluiddot
Copy link
Contributor

fluiddot commented Feb 6, 2023

Description

As described in wordpress-mobile/WordPress-Android#17712, we pick the wrong version of the Gallery block when a self-hosted site is in WordPress 5.9+ and the Gutenberg plugin is not installed. This issue is mainly affecting the cases where the prop used to pick the Gallery version (galleryWithImageBlocks) is unknown, which leads to using v1 since it's the default value.

The Gallery block v2 is supported in WordPress 5.9+, so in order to avoid using the wrong version when having an unknown value, we could determine the Gallery block version to use based on the site's WordPress version.

I'm leaving some code references to pinpoint the logic used to determine the Gallery block version to use:

galleryWithImageBlocks prop set by the app

iOS:

if let galleryWithImageBlocks = editorSettings?.galleryWithImageBlocks {
settingsUpdates["galleryWithImageBlocks"] = galleryWithImageBlocks
}

Android:
Boolean galleryWithImageBlocks = null;
if (editorTheme.containsKey(MAP_KEY_GALLERY_WITH_IMAGE_BLOCKS)) {
galleryWithImageBlocks = editorTheme.getBoolean(MAP_KEY_GALLERY_WITH_IMAGE_BLOCKS);
}

Picking Gallery block version based on galleryWithImageBlocks

class Editor extends Component {
constructor( props ) {
super( ...arguments );
// need to set this globally to avoid race with deprecations
// defaulting to true to avoid issues with a not-yet-cached value
const { galleryWithImageBlocks = true } = props;
window.wp.galleryBlockV2Enabled = galleryWithImageBlocks;

function getGalleryBlockV2Enabled() {
// We want to fail early here, at least during beta testing phase, to ensure
// there aren't instances where undefined values cause false negatives.
if ( ! window.wp || typeof window.wp.galleryBlockV2Enabled !== 'boolean' ) {
throw 'window.wp.galleryBlockV2Enabled is not defined';
}
return window.wp.galleryBlockV2Enabled;
}
/**
* The new gallery block format is not compatible with the use_BalanceTags option
* in WP versions <= 5.8 https://core.trac.wordpress.org/ticket/54130. The
* window.wp.galleryBlockV2Enabled flag is set in lib/compat.php. This method
* can be removed when minimum supported WP version >=5.9.
*/
export function isGalleryV2Enabled() {
if ( Platform.isNative ) {
return getGalleryBlockV2Enabled();
}
return true;
}

if ( ! isGalleryV2Enabled() ) {
return <EditWithoutInnerBlocks { ...props } />;
}
return <EditWithInnerBlocks { ...props } />;

Step-by-step reproduction instructions

  1. Create a self-hosted site with WordPress version 5.9 or above.
  2. Check that the Gutenberg plugin is not installed. If not, uninstall it.
  3. Create a post.
  4. Insert a Gallery block.
  5. Observe that the Gallery block is v1 (content is not rendered using inner blocks).

Expected behaviour

The correct version of the Gallery block should be used.

Actual behaviour

In some cases, the wrong version of the Gallery block is used.

Screenshots or screen recording (optional)

N/A

WordPress information

  • WordPress version: N/A
  • Gutenberg version: N/A
  • Are all plugins except Gutenberg deactivated? N/A
  • Are you using a default theme (e.g. Twenty Twenty-One)? N/A

Device information

  • Device: N/A
  • Operating system: N/A
  • WordPress app version: trunk
@fluiddot fluiddot added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Block] Gallery Affects the Gallery Block - used to display groups of images [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention labels Feb 6, 2023
@fluiddot
Copy link
Contributor Author

fluiddot commented Feb 6, 2023

Setting this issue as a high priority because, although it might be an edge case, we detected that around 10% of sessions are self-hosted sites having galleryWithImageBlocks prop as unknown. It's hard to know from this 10% how many are being impacted by the issue but still, the numbers seem relevant to provide a fix.

@derekblank derekblank self-assigned this Apr 21, 2023
@derekblank
Copy link
Member

We detected that around 10% of sessions are self-hosted sites having galleryWithImageBlocks prop as unknown.
By unknown, do you mean that 10% of sessons with self-hosted sites, galleryWithImageBlocks returns undefined, or false?

In my testing with WordPress 5.9+ on Android without the Gutenberg plugin, isGalleryV2Enabled() returned false. If I understand the issue correctly, to use the Gallery Block v2 this case, perhaps we could modify isGalleryV2Enabled to check the WP version number and return true it is 5.9 or greater, which could enable the v2 Gallery Block for these users. If the galleryWithImageBlocks returns undefined however (I could not replicate this), perhaps we could still use this same approach to check the version number and safely return true if it is 5.9 or greater. WDYT?

Also, what do you think the most efficient way to retrieve the WP version number would be here? Do we already have this available as a value within the editor?

@fluiddot
Copy link
Contributor Author

fluiddot commented May 8, 2023

Hey @derekblank , sorry for the delayed replay 🙇 . I tried to spare time to find a consistent way to reproduce the issue before sharing my insights but couldn't finish it. In the meantime, let me share a response to some of the above questions.

In my testing with WordPress 5.9+ on Android without the Gutenberg plugin, isGalleryV2Enabled() returned false. If I understand the issue correctly, to use the Gallery Block v2 this case, perhaps we could modify isGalleryV2Enabled to check the WP version number and return true it is 5.9 or greater, which could enable the v2 Gallery Block for these users.

Exactly, this is the issue we need to solve. We need to ensure that self-hosted sites with WordPress 5.9+ always use Gallery v2. We can update isGalleryV2Enabled although I think we currently don't have access to the WP version within the editor. Unless we expose it, we could apply this solution on the native side, probably in the code references I shared in the issue's description.

If the galleryWithImageBlocks returns undefined however (I could not replicate this), perhaps we could still use this same approach to check the version number and safely return true if it is 5.9 or greater. WDYT?

Yes, I think we can assume that sites with WP 5.9+ can always use Gallery v2. Hence, probably we don't need to identify the edge case where galleryWithImageBlocks returns undefined.

@derekblank
Copy link
Member

derekblank commented May 12, 2023

@fluiddot I spent some time searching for the best way to compare the WordPress version number in order to set the Gallery block version to v2 for self-hosted sites using WP 5.9+.

In reviewing all of the code references you identified in the parent comment, handling this via the native refs in particular (Gutenberg.swift and RNReactNativeGutenbergBridgeModule.java) feels like a bit of an indirect resolution for an edge case, but it may still be the preferred solution.

Obtaining the WP version with Javascript seems surprisingly difficult (whether it's via the editor or outside of it), but feels like a much more direct way to handle the Gallery flag logic for WP 5.9+. I would have expected a site's WP version to be available via an API like wp-json that we could have leveraged within the editor with apiFetch, and was surprised to learn the WP version number is not present here. Perhaps it would be possible to introduce a custom endpoint for our use case, but that may be even more roundabout than just receiving the version from the host apps. If we had access to this value in the editor's Javascript, however, we could pretty easily handle the Gallery flag logic on any of the three other Javascript code references you mentioned.

One of the more elegant solutions seems to be to match how this is handled for the List and Quote blocks using the mobile block editor settings. I created a PR to demonstrate this, but in testing realized that it may not fully work for users who do not have the Gutenberg plugin installed due to the code being in lib/experiments, and this is the group we are trying to reach most. However, I'm curious about your thoughts on this approach, and if a similar technique could be modified to reach users without the plugin installed. Handling the logic here feels closest to the "source" of the flag to me, but it may not be possible due to the plugin constraint.

After reviewing the above solutions, if we feel modifying the flag in the host apps may still be the better solution, could you describe this a bit further? This solution is less obvious to me. Would you expect that we handle the Gallery v2 flag logic directly in the Swift and Java files respectively using the WP version integer, or use the host apps to access the WP version integer and pass it via the bridge?

@fluiddot
Copy link
Contributor Author

In reviewing all of the code references you identified in the parent comment, handling this via the native refs in particular (Gutenberg.swift and RNReactNativeGutenbergBridgeModule.java) feels like a bit of an indirect resolution for an edge case, but it may still be the preferred solution.

@derekblank The idea of adding the condition here is to delegate picking the Gallery version to the host app, similar to other settings we have to configure the editor (reference).

Obtaining the WP version with Javascript seems surprisingly difficult (whether it's via the editor or outside of it), but feels like a much more direct way to handle the Gallery flag logic for WP 5.9+.

I agree that handling the edge case in JS (i.e. the editor) might seem more straightforward, but keep in mind that the editor is configured by the host app. This is a nuance that makes the editor's architecture diverge from the web version. In the web version, the editor can be configured via the backend as it's rendered within the WordPress instance. In our case, since the editor is served by the apps, we don't have access to that mechanism of backend rendering. The closest thing we have to the backend is actually the host apps.

I would have expected a site's WP version to be available via an API like wp-json that we could have leveraged within the editor with apiFetch, and was surprised to learn the WP version number is not present here.

Yep, I think I also tried to fetch the WP version in the past unsuccessfully. In any case, the WP version is available in the host app. We already query data related to the site from the host app, so doing the same for the WP version might make sense.

Perhaps it would be possible to introduce a custom endpoint for our use case, but that may be even more roundabout than just receiving the version from the host apps.

I think adding an endpoint would be the most complex option for this case.

One of the more elegant solutions seems to be to match how this is handled for the List and Quote blocks using the mobile block editor settings. I created a PR to demonstrate this, but in testing realized that it may not fully work for users who do not have the Gutenberg plugin installed due to the code being in lib/experiments, and this is the group we are trying to reach most. However, I'm curious about your thoughts on this approach, and if a similar technique could be modified to reach users without the plugin installed. Handling the logic here feels closest to the "source" of the flag to me, but it may not be possible due to the plugin constraint.

The problem, as you mentioned, is that this solution won't work for sites that don't have the Gutenberg plugin. This is the case we are trying to cover, so we'd need to figure out something different. Following the approach of introducing the condition in the backend, the only alternative I see is exposing this setting somehow from WordPress. But for something so specific, I wouldn't go that way.

After reviewing the above solutions, if we feel modifying the flag in the host apps may still be the better solution [...]

I agree, this solution seems the best option.

[...], could you describe this a bit further? This solution is less obvious to me.

I think the task could be broken down in:

  1. Find the logic where we calculate the value galleryWithImageBlocks that is passed to the editor on WP-Android.
  2. Find the logic where we calculate the value galleryWithImageBlocks that is passed to the editor on WP-iOS.
  3. Add the condition that checks the WP version and forces the true value if it's 5.9+. NOTE: The WP version is usually an attribute that is associated with the Blog model (example).

Part of the task would be to navigate the host app's code in order to find where to apply the solution. Let me know if this helps you and don't hesitate to share more questions.

Would you expect that we handle the Gallery v2 flag logic directly in the Swift and Java files respectively using the WP version integer, or use the host apps to access the WP version integer and pass it via the bridge?

I'd say that unless we foresee the need for the WP version in the editor in future developments, I'd go with applying the solution only on the native side.

@derekblank
Copy link
Member

I think the task could be broken down in:

  1. Find the logic where we calculate the value galleryWithImageBlocks that is passed to the editor on WP-Android.
  2. Find the logic where we calculate the value galleryWithImageBlocks that is passed to the editor on WP-iOS.
  3. Add the condition that checks the WP version and forces the true value if it's 5.9+. NOTE: The WP version is usually an attribute that is associated with the Blog model (example).

I've identified this area for iOS and implemented the change to BlockEditorSettings+GutenbergEditorSettings.swift, and it appears to be working in my testing. This PR implementation is ready for testing: wordpress-mobile/WordPress-iOS#20736

On the Android side, @mkevins and I identified that themeSupportsGalleryWithImageBlocks would be a good candidate to try similar logic as the iOS PR, so I'll implement that next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
None yet
2 participants