[RNMobile] Add Cover Block media settings

[ghost]

Description

• Gutenberg Mobile: Add Cover block media settings wordpress-mobile/gutenberg-mobile#3028
• WordPress iOS: [Gutenberg] Add Cover block media settings wordpress-mobile/WordPress-iOS#15665
• WordPress Android: [Gutenberg] Add Cover block media settings wordpress-mobile/WordPress-Android#13818

Addresses wordpress-mobile/gutenberg-mobile#1741 by adding the ability to edit the cover block focal point and toggle fixed position backgrounds.

I opted to rely upon React Native's PanResponder for the drag handle in the focal point editor. I attempted to rely upon react-native-gesture-handler, but incompatibility with Android modals forced me to rely upon PanResponder.

To support applying a max-height constraint (instead of a fill-parent strategy that all other images currently use) to an unknown sized image, I expanded the API of the existing Image component to broadcast when the image sizing metadata was known. This allows avoiding a collapsed element while the image loads.

How has this been tested?

User Test Cases

User case 1

• Open the app with metro running.
• Add a Cover block.
• Select a color.
• Add a text.
• Tap the settings icon for the block.
• Expect: Add image or video option available.
• Add an image for the block.
• Expect: Media preview displayed with focal point hint and edit media button in top right corner. Two options shown: Edit focal point and Fixed background toggle.

User case 2

• Open the app with metro running.
• Add a Cover block.
• Set an image for the block.
• Tap Edit focal point.
• Expect: Media rendered with default focal point position displayed by drag handle and X/Y sliders.
• Tap and drag handle to different location and release.
• Expect: Slider values are updated.
• Tap and drag handle on X-Axis Position slider.
• Expect: Drag handle position is updated.
• Tap and drag handle on Y-Axis Position slider.
• Expect: Drag handle position is updated.
• Tap Cancel.
• Expect: Focal point changes are not persisted.

User case 3

• Open the app with metro running.
• Add a Cover block.
• Set an image for the block.
• Tap Edit focal point.
• Tap and drag handle to different locaiton and release.
• Tap Apply.
• Expect: Focal point changes are persisted.
• Toggle Fixed backgorund.
• Expect: Focal point is cleared.

Screenshots

UX Interaction

ios-focal-point.mov

android-focal-point.mov

iOS Screenshots

Edit cover block	Edit focal point

Android Screenshots

Edit cover block	Edit focal point

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[geriux]

Hey @shadow351 👋 just gave this I quick look at the code and its going on the right direction 👍

Just left two minor comments but I'll do a further review when it's ready for review.

Thanks!

[geriux]

I duplicated an existing tooltip component for my needs rather than attempting to abstract and share it because (A) the project directory structure of when/how components are shared is unclear to me at this time and (B) the existing tooltip's overlay and relative positioning did not align my use case of attaching to an absolutely positioned drag handle.

I can help here, I'd really prefer to make the tooltip shared even if it's just the basic functionality of it.

Can you elaborate a bit more about the issues you had?

Thanks!

[ghost]

I duplicated an existing tooltip component for my needs rather than attempting to abstract and share it because (A) the project directory structure of when/how components are shared is unclear to me at this time and (B) the existing tooltip's overlay and relative positioning did not align my use case of attaching to an absolutely positioned drag handle.

I can help here, I'd really prefer to make the tooltip shared even if it's just the basic functionality of it.

Can you elaborate a bit more about the issues you had?

Thanks!

@geriux thanks for the help.

First, in which directory should I place this shared tooltip component if I intend to share it between the page templates and focal point picker?

Second, the page template UI doesn't appear to be accessible within the react-native-editor test project. Where would I find that UI so that I may test any changes I make to a shared tooltip? I successfully set up wordpress-mobile/WordPress-iOS and wordpress-mobile/WordPress-Android to access the UI.

Third, I noticed a few differences between the use cases that caused me to pause sharing the code. I welcome your thoughts and opinions on whether you believe it is still worthwhile to share the code.

Page Template	Focal Point
Requires an overlay to fill the entire window, but do not dismiss if tooltip target (page template buttons) is tapped.	Requires overlay to fill entire bottom sheet, and dismiss if tooltip target (focal point drag handle) is tapped.
Tooltip relatively (i.e. no absolute style applied) positions itself against a sibling.	Tooltip absolutely positioned against a sibling. Here the tooltip needs to be within a shared parent as the sibling, but must itself be absolutely positioned to avoid modifying the parent size and related focal point coordinate math (i.e. toggling the tooltip's presence would change the math).
Left-aligned tooltip arrow.	Center-aligned tooltip arrow.

Placing the current Tooltip into FocalPointGroup means the tooltip overlay cannot expand to cover the entire bottom sheet, because FocalPointGroup is absolutely positioned itself.

<BottomSheet>
  <FocalPointGroup> /* absolutely positioned based on focal point coordinates */
    <Tooltip /> /* overlay unable to "break out" of FocalPointGroup */
    <FocalPointIcon />
  </FocalPointGroup>
</BottomSheet>

Placing the current Tooltip outside of FocalPointGroup means the tooltip no longer knowns where it should place itself in relation to the focal point icon.

<BottomSheet>
  <Tooltip /> /* not within FocalPointGroup, so does not know coordinate position to match */
  <FocalPointGroup> /* absolutely positioned based on focal point coordinates */
    <FocalPointIcon />
  </FocalPointGroup>
</BottomSheet>

I have not fully explored the feasibility of these options, but here are the ideas I have thus far for sharing a tooltip between the two use cases.

Option 1

Expand the current Tooltip to allow dictating the placement coordinates to match the FocalPointGroup. We'd just have to allow a placement prop.

<BottomSheet>
  <Tooltip placement={{ x: focalPoint.x, y: focalPoint.y }} />
  <FocalPointGroup placement={{ x: focalPoint.x, y: focalPoint.y }}>
    <FocalPointIcon />
  </FocalPointGroup>
</BottomSheet>

Option 2

Rather than including the overlay within Tooltip, we could export a separate TooltipOverlay component so the parent component could dictate its dimensions/behavior. This could be a render prop or we could use React context.

<BottomSheet>
  <Tooltip.Overlay style={{ height: '100%', width: '100%' }}>
    {({ visible }) => (
      <FocalPointGroup placement={{ x: focalPoint.x, y: focalPoint.y }}>
        <Tooltip visible={visible} />
        <FocalPointIcon />
      </FocalPointGroup>
    )}
  </Tooltip.Overlay>
</BottomSheet>

A third option may be something simpler that shares the basic functionality, as you stated. I'd be curious to hear what you are picturing. Let me know what you think. Thanks!

[ghost]

@geriux I refactored my tooltip in this branch to match the expectations that tapping anywhere should dismiss the modal (per Thomas' request). I then explored a variation of the tooltip that would allow using it for both use cases — page templates and focal point picker. The exploration is still rough, but I can clean it up more if you believe it is worth pursuing.

My thoughts on it are the it definitely expands the tooltip API surface area and adds complexity. I am not sure if it provides meaningful value at this point with only two points of usage. Part of me wonders if we should wait to see if more use cases arise before settling on an API for a shared tooltip. What do you think?

[dcalhoun]

Edit menu/actions

Do you think tapping anywhere on the thumbnail cell should show the ActionSheet (example)? And if so, should the edit button be anchored to the top-right of the cell and not the image?

🟢 Touchable Cell: Yes, for both visual users and a11y, I think it makes sense for the entire cell to be touchable. Yes, moving the edit button to top-right of the cell would make more sense to me. I will implement that within this PR.

@iamthomasbishop I'm going to reverse course on a "Touchable Cell" that triggers the ActionSheet/menu. From building it out, I've discovered there is an issue in the bottom sheet that causes it to not appropriately disregard taps on buttons when the bottom sheet scrolls. I.e. it triggers a button tap even if you are swiping to scroll. To be clear, this issue appears to already exist for all buttons in the bottom sheet, but the large height/width of the new focal point preview cell exacerbates the issue as it is much easier to trigger this scenario.

I am happy to create a separate issue for the button sensitivity in the bottom sheet. How do you feel about postponing this "Touchable Cell" change and retain the smaller edit image button for the time being?

Video of bottom sheet sensitivity

erroneous-bottom-sheet-touch.mov

Additionally, here is an example of what Horizontal Overflow visibility for the focal point cross-hair looks like:

Horizontal Overflow example Image

Also, from playing with Vertical Overflow visibility, it feels a bit awkward for the cross-hair to sit atop the button beneath. Aside from the side effects and testing concerns I mentioned for the bottom cell, this awkwardness may be a reason to avoid Vertical Overflow visibility. Thoughts?

Vertical Overflow example Image

[iamthomasbishop]

Thanks @dcalhoun!

Height Transition

it would appear this regression is unrelated to my work

Sounds good, thank you for investigating!

Overflow

Would you be OK with postponing this for another Issue?

Additionally, here is an example of what Horizontal Overflow visibility for the focal point cross-hair looks like:

Fixing the horizontal overflow makes a big difference, I'm much less concerned about the vertical, so yes we can worry about that in a separate pass.

Adding it to the canvas edit button may be more challenging, as tapping it would require a double-navigation event

That makes sense, let's defer that for another time. Just thinking out loud, we could potentially just drop the user on the top-level sheet and show a hint of some sort (perhaps something like the little pulsing blue dots that we show for Quick Start after the site creation flow).

Clear media Menu: Related to options within the ActionSheet/menu, do we desire to include Clear Media as an option for the inspector controls edit button? That is currently an option for the canvas edit button.

I was thinking yes, in the spirit of consistency. While I tend to prefer not duplicating entry points if possible, if we're going to use the same edit menu component in multiple places, it would make sense for those actions to be consistent.

Tap Media Action

Given this topic spans multiple media type blocks, I would ask if you are OK addressing changes to this in a separate ticket?

Definitely, fine to defer on that as well.

I've discovered there is an issue in the bottom sheet that causes it to not appropriately disregard taps on buttons when the bottom sheet scrolls

Ahh, that's right, I had forgotten about that interaction issue. I'm fine with deferring that one to another day as well.

it feels a bit awkward for the cross-hair to sit atop the button beneath

I see what you mean. That's totally fine to let the button sit "over" the cross-hair, seems like a reasonable trade-off.

---

I think I covered all of your questions, but let me know if I missed or need to clarify anything 😄

[dcalhoun]

@iamthomasbishop thank you. You have answered all my questions. I will move forward seeking final code review and merge this work. If you'd like for me to hold off to allow you to review another build or have further discussion, just let me know. 😃

[dcalhoun]

@geriux I believe this is ready for another code review. All the sibling PRs have been updated, including installable builds. Please let me know if there is anything else I need to do. Thanks!

[dcalhoun]

@geriux I have addressed the a11y string issue and the missing video within the inspector for Android. This should be ready for review again. New builds are available on the sibling branches. Thanks!

[geriux]

LGTM! Nice work @dcalhoun! 🎉 👏

This was a complex task and I'm so happy it's finally ready to be merged! Thank you for all the changes.

Can you add this new feature to the release notes before merging? Thanks!

Tested on both iOS and Android.

[hypest]

👋 @dcalhoun , this might be a naive question: as I'm trying to understand the two different components named FocalPointSettings (one here and the other one here), can you elaborate when you get the chance, on what's the difference between the two, perhaps in their usage too, and whether we could maybe rename one or the other to differentiate?

[dcalhoun]

@hypest the former is the button in the parent sheet used to navigate to the focal point picker. The latter is the focal point picker itself, which is the child sheet. I agree that more explicit naming would be helpful.

IIRC I attempted to inline the latter into its parent, but navigation would not work. I never identified why that was. It likely related to the absence of a React Navigation context. Hence, it was moved into a separate file and confusingly named.

Long-term, the ideal would be to refactor this work to leverage the new BottomSheetSubSheet introduced in #28543. That would move some of the complexity of child sheet to an abstracted location.

[hypest]

Aha, I see now, thanks for elaborating David!

With us still relying quite a bit on search-in-files for navigating the codebase, I think it would pay off if we try to do some renaming here, though not a high priority by any measure.