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

Swiping to dismiss full-height bottom sheet causes full-height set for subsequent bottom sheet #28173

Open
dcalhoun opened this issue Jan 13, 2021 · 3 comments
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Enhancement A suggestion for improvement.
Projects

Comments

@dcalhoun
Copy link
Member

dcalhoun commented Jan 13, 2021

Describe the bug
When dismissing a full-height bottom sheet (e.g. link picker in #23922) by swiping downward or tapping the overlay, rather than tapping the cancel/back button, the full-height styling is then erroneously applied to the next bottom sheet opened. This was discovered while building #25810, as it also occurs for the full-height focal point picker.

To reproduce

  1. Launch content editor.
  2. Insert Paragraph block.
  3. Tap the Link button to insert a link.
  4. Tap "Link to" to set URL.
  5. Swipe down to dismiss the bottom sheet OR in landscape orientation tap the overlay outside the bottom sheet.
  6. Tap the Link button to insert a link.

Expected behavior
When opening the Link bottom sheet the second time it should not display at full-height of the display, but should only take up the bottom portion of the display.

Screenshots

Details
Expected Actual
Expected Actual
Interactions
RPReplay_Final1610545956.mp4

Editor version (please complete the following information):

  • WordPress version: 5.6
  • Does the website have the Gutenberg plugin installed, or is it using the block editor that comes by default?: default
  • If the Gutenberg plugin is installed, which version is it?: 9.6.2

Smartphone (please complete the following information):

  • Device: iPhone 11 Pro
  • OS: 14.3
  • WordPress App Version: 16.3
@dcalhoun dcalhoun added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Jan 13, 2021
@dcalhoun
Copy link
Member Author

After playing with this problem, I wonder if an appropriate solution would be to "reset" the full-height status within the clean-up function of the useFocusEffect. From testing, this does work. My only concern would be if it causes any performance degradation, as I haven't fully come to understand the bottom sheet architecture.

useFocusEffect(
  useCallback(() => {
    // ...existing logic...
    return () => {
      setIsFullScreen(false);
    };
  })
);

@hypest hypest added this to Triage in Mobile Apps via automation Jan 14, 2021
@hypest hypest moved this from Triage to To do in Mobile Apps Jan 14, 2021
@geriux
Copy link
Member

geriux commented Jan 19, 2021

Thanks for creating this issue @dcalhoun! Maybe @dratwas can give some insight regarding the bottom sheet architecture and your possible fix from above.

dcalhoun added a commit to dcalhoun/gutenberg that referenced this issue Jan 19, 2021
Originally added to address full-height styles lingering when
swiping-to-dismiss a bottom sheet, it's now recognized this fix does not
address the root issue. Additional details are captured in a new issue.

WordPress#28173
dcalhoun added a commit to dcalhoun/gutenberg that referenced this issue Feb 3, 2021
Originally added to address full-height styles lingering when
swiping-to-dismiss a bottom sheet, it's now recognized this fix does not
address the root issue. Additional details are captured in a new issue.

WordPress#28173
@dcalhoun
Copy link
Member Author

@dratwas following up to see if you have any thoughts on my proposed fix. No need to deep dive into the issue, just curious of your initial reaction.

geriux added a commit that referenced this issue Feb 15, 2021
* Render media within settings panel

Initial pass at rendering media. Potentially includes unnecessary focus
styles and media upload callback handlers.

* Display media edit button

Currently, `isSelected` must be true for the edit button to render.

* Add "Edit focal point" bottom sheet cell

* Replace unused bottom cell value

Leveraging `customActionButton` allows for rendering the cell child
icon, rather than relying upon an empty value.

* Enable navigation to edit focal point sheet

* [wip] Add focal point settings component

* [wip] Add native focal point picker

* Create foundation for focal point picker UI

Remove unused code. Set up required navigation structure to pass route
parameters.

* Remove duplicative effect

This same code is executed a few lines prior.

* Add focal point picker media and range controls

Render media and range control within focal point picker.

* Remove unnecessary minHeight prop

When editing the focal point picker, we need to render the image with
its full aspect ratio, rather than restricting the height.

* Add focal point picker handle

* Position focal point handle based upon state

* [wip] Add drag handler for focal point picker

* Fix lint warning

* Update focal point picker blue

Previous color appears to have been removed from the source.

* Fix bad merge conflict resolution

* Fix various lint warnings

* Add TODO note

* Change default position for focal point range cells

* Add TapGestureHandler to focal point picker

* Round value for RangeControl text input

Avoid rendering floats within the `RangeControl` text input.

* Add focal point hint to Cover Block media preview

* Simplify RangeControl props

Remove unnecessary props.

* Clear focal point when clearing media

* Add Fixed background toggle

* Display button to attach media when no media is attached

* Fix beginning drag atop focal point picker drag handle

Previously, if you began your drag directly atop the drag handle, the
position would not be updated as you dragged. Instead, the parent would
manage the pan gesture and attempt to scroll the bottom sheet content.

* Render media preview within fixed dimensions

To avoid images with extreme aspect ratios creating large amounts of
scrolling, we limit the dimensions of the media preview. This prevents
the min-height value from being applied to this element and animating
multiple locations on the screen, in the bottom sheet and in the page
content.

* Leverage full-height sub-sheet for focal point picker

Better manage media display and scroll management through a larger focal
point editing sheet.

* Sync pan gesture values to sliders

`@react-native-communitye/slider` is an uncontrolled component, so we
must increment a key to sync the pan gesture values to the sliders.

https://git.io/JTe4A

* Display percentage suffix for sliders

* Improve focal point picker styles

* Reduce max-height for media preview

* Limit max-height for focal point picker media

* Replace RNGH with RN PanResponder to fix Android modals

Even though work has been done, it would appear that RNGH doesn’t work
on Android within modals.

- https://git.io/JTkgM
- https://git.io/JTkgS
- https://git.io/JTkg9
- https://bit.ly/3nHbpVl

* Improve PanResponder implementation

Reduce the frequency the position state is updated to improve
performance.

* Abstract FocalPoint into separate file

Address PR feedback.

* Revert "Leverage full-height sub-sheet for focal point picker"

This reverts commit c0b7577. Allowing
scrolling of the full-height bottom sheet content for landscape
orientation proved to be difficult. Now that we set a max-height for the
focal point picker media, a full-height bottom sheet may not be all that
necessary.

* Add drag handle tooltip

* Hoist tooltip visibility control into focal point picker

Because the focal point picker needs a `PanResponder` atop the entire
image, it makes more sense to allow it to manage the touch input
required to toggle the tooltip visibility.

* Prevent collapsed placeholder element while image size is fetched

To prevent the dynamically sized image element from collapsing, this
displays the element at the parent's full width until we know the aspect
ratio of the image.

* Revert "Revert "Leverage full-height sub-sheet for focal point picker""

This reverts commit d537371.

* Fix odd layout jumping when incrementing slider key

When incrementing the slider key for the focal point picker, a strange
jump would occur between initial mount and second mount. The jump
appeared to be related to rendering the "value component" in
`BottomSheet.Cell`. Replacing the empty `value` with the `leftAlign`
label fixes the issue and appears to create no regression.

- https://git.io/JTmWU

* Revert "Revert "Revert "Leverage full-height sub-sheet for focal point picker"""

This reverts commit 1ad1e26.

* Limit fixed background setting to images

The web interface disallows setting fixed backgrounds for media types of
video.

* Duplicate styles needed for focal point settings

* Add dynamic focal point to media preview

* Ensure focal point coordinates are never empty

Avoid error thrown from referencing empty focal point.

* Disable focal point hint for fixed background media

* Render media preview for videos

* Fix video media preview sizing

* Improve video media preview sizing

Add border to match image media previews. Ensure element does not
collapse while video dimensions are loaded.

* Add video support to focal point picker

* Simplify prop type logic

Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com>

* Address review feedback

* Refactor Tooltip to dismiss on tap

Refactor to align closer to existing tooltip component so that the tip
dismisses on tap anywhere, rather than on focal point handle drag.

* Add focal point picker tooltip flag

* Improve placeholder media to prevent layout jumps

* Tooltip overlay avoids blocking child context interactivity

Leverage the responder capture callbacks to dismiss the tooltip, but
promote the overlay to the controlling responder.

* Improve tooltip styles

Reorganize styles to clarify magic numbers.

* Add guard for undefined prop callback

* Fix focal point picker drag handle jump

Replace `setOffset` with `extractOffset` as it accomplishes the intended
goal directly. It also helps avoid a jump that occurs when the tooltip
is dismissing while interacting with the drag handle.

* Support left aligned tooltips

* Page template picker shares tooltip

Leverage a shared tooltip between page template picker and focal point
picker.

* Expand Tooltip positioning flexibility

* Support right aligned tooltips

* Tapping tooltip label dismisses tooltip

* Update focal point picker tooltip usage

* Fix disallowed syntax throwing lint errors

* Abstract Cover controls to reduce file size

Relocate the controls sections into a new file to reduce the size of the
Cover edit file.

* Fix lint warning

* Replace potential circular dependency with type check utility

The previous approach likely was a circular dependency causing the unit
test failures. This change leverages the file type check utility used
by the existing web focal point picker.

* Fix erroneously flush media preview

Add bottom margin to media preview to avoid odd styling.

* Prevent focal point hint overlapping edit media button

The `Image` component renders an edit button. By adding a `children`
prop, we can now render the focal point hint between the image media and
the edit button, ensuring that the edit button is always accessible.

The `Video` component does not render an edit button and therefore does
not need to render the focal point hint as a child.

* Replace HoC with Hook

Address code review feedback.

* Revert "Update focal point picker tooltip usage"

This reverts commit d2b9fb8e6addefc29762c62188ded8fd0a537769.

* Revert "Page template picker shares tooltip"

This reverts commit 633b1d50e3409d9b7ae83c77b26524517760bc6b.

* Leverage UnitControl for focal point picker

Replace custom suffix for `RangeControl` with newly build `UnitControl`.
This adds the ability to disable the unit picker when there is only a
single unit provided to `UnitControl`, as switching between an option of
one unit provides little value. Disabling the picker also matches the
web experience.

* Avoid scrolling focal point picker with full-height bottom sheet

Leverage entire device height to avoid the need to scroll the focal
point picker bottom sheet.

* Remove unnecessary bottom sheet methods

These were originally copied from the `ColorPickerSettings`, but do not
appear to be applicable for this focal point picker implementation.

* Ensure full-height bottom sheet does not linger

Without explicitly disabling the full-screen height bottom sheet when
the bottom sheet is closed, it will remaining enabled. This results in
erroneously apply full-height styles to all bottom sheets.

The existing `useFocusEffect` callback is only invoked when the
back/cancel button is tapped, but not when dismissing the bottom sheet
via swiping or tapping the overlay.

* Fix bad merge conflict resolution

The page template picker tooltip was erroneously re-added during a merge
conflict resolution.

* Remove lingering, unused code

* Fix negative and lost values when dragging outside bounds

Previously, if the slider/drag handle was dragged outside of the edge
of the range/image, the value would either be set to a negative value or
ignored altogether. This ensures that negative values are not set and
that instead are set to zero.

* Add comments for additional context

* Avoid unnecessary re-renders with useCallback

Address code review feedback that we should avoid these arrow functions
within the component return.

* Display tooltip within demo editor

* Remove unnecessary useCallback usage

This code was not providing any performance gains, as the children
components are not memoized.

* Avoid importing react directly

Fix lint errors.

* Remove short-sighted fix for full-height bottom sheet

Originally added to address full-height styles lingering when
swiping-to-dismiss a bottom sheet, it's now recognized this fix does not
address the root issue. Additional details are captured in a new issue.

#28173

* Remove unused import

* Fix broken min-height increment

The incorrect `key` caused the `UnitControl` to be unnecessarily
re-mounted, which cleared out the interval that incremented the value
when the user held their finger down on the increment/decrement button.

This mistake was likely the result of a bad merge conflict, as it was
already fixed previously.

#27005

* Remove erroneously committed configuration file

* Mobile - Wip - Picker and BottomSheet

* Add MediaUpload within bottom sheet context

react-native-modal does not support opening multiple modals on iOS. In
order to dismiss the bottom sheet prior to opening the media picker
modal we must provide the MediaUpload access to the bottom sheet
context to register a callback to open the second modal once the first
has closed.

Additionally, this newly added MediaUpload cannot be used for the
ImageEditingButton because the "Replace" functionality of this button
involves opening _two_ pickers. The first one would dismiss bottom
sheet, thus removing access to this new MediaUpload picker. This results
in the second picker no longer having access to the media upload
callback. Because of this, the ImageEditingButton continues to use the
pre-existing MediaUpload in the parent Cover edit component.

* Compute relative coordinates in PanResponder release

Ideally, the x and y coords are merely locationX and locationY from the
nativeEvent. However, we are required to compute these relative
coordinates to workaround a bug affecting Android's PanResponder.
Specifically, dragging the handle outside the bounds of the image
results in inaccurate locationX and locationY coordinates to be
reported. https://git.io/JtWmi

* Capture Podfile.lock changes

* Capture Podfile.lock changes

* Revert "Capture Podfile.lock changes"

This reverts commit 203635216d4b30c65ad25a51a23b57154a8a3def and
67f18a13e8121d4b4d15d75553945aa62e6f7b5e.

* Apply consistent media background

Share media background color for inspector controls and focal point
picker.

* Remove highlight border from focal point media

Add prop to allow disabling the blue border applied to images when they
are "selected" for editing.

* Disable focal point picker when fixed background enabled

We desire that the focal point cannot be changed when fixed background
is enabled. This is because the focal point coordinates have no impact
in this context. This also matches the behavior for the web editor.

* Remove extra padding atop and beneath focal point picker settings

* Update focal point hint styles

Increase focal point hint contrast and balance sizing.

* Use two digit strings for coordinates

Ensure the coordinate values are strings with two digits after the
decimal, rather than floats. This helps ensure the data match between
web and native.

* Fix style lint error

* Reduce opacity of disabled focal point navigation button

Lower opacity to communicate when the edit focal point navigation button
is disabled while fix background is toggled on.

* Remove bottom margin beneath media preview

* Allow focal point cross-hair to overflow horizontal bounds

Avoiding cutting off the cross-hair improves its visibility when it is
situated along the edge of the media preview.

* Allow long press action for bottom cell

This supports use cases like long pressing edit an image.

* Add edit image accessibility label

* Improve accessibility of edit image button

Previously, it was impossible to select the edit image button, as it was
rendered beneath the image itself.

* Reorder Cover block media settings sections

* Add clear media button to inspector controls Cover image edit button

Align with image edit button found within the canvas.

* Improve double modal handling for iOS

This allows dismissing the bottom sheet _after_ the second picker,
rather than before it. This is helpful for the scenario where a user
cancels the first picker. Now the bottom sheet will still be open in
that scenario.

* Increase a11y label accuracy

The related menu also provides the ability to clear the media.

* Improve accessibility of add media button

Previously, voice over would announce nothing when selecting this
button, as it did not have a label or hint.

* Pass string through a11n tooling

Avoid a static English string.

* Improve focal point settings media styles

Address styling oddities for images and videos within the focal point
settings. Namely, Android would render videos with the incorrect width,
overflowing the parent container.

* Avoid invisible, paused videos on Android

Rendering `react-native-video` with `paused` set to `true` causes the
video to not render visibly on Android only. This is presumably related
to other issues regarding `paused`. https://git.io/Jt6Dr

By seeking the video to 0, the video correctly renders on Android.

* Prevent image from overflowing width

* Add CHANGELOG note

Co-authored-by: David Calhoun <dpcalhoun@gmail.com>
Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com>
@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Enhancement A suggestion for improvement.
Projects
Mobile Apps
  
To do
Development

No branches or pull requests

3 participants