spaceshelter / orbitar

Experimental collective social/blogging platform with self-regulation.
MIT License
59 stars 23 forks source link

Close the image zoom pop-up upon a back gesture #381

Closed di-maroo closed 4 months ago

di-maroo commented 4 months ago

On mobile devices, when the zoomed image pop-up is open, a natural way of closing it is a "back" gesture (swipe from the right edge of the screen to the left). But as of now, this gesture performs back navigation, bringing you out of the thread instead of closing the pop-up, which can be quite frustrating, especially in cases where a large thread with many unread messages was opened.

This pull request tackles the issue by pushing a new state to the history stack when the preview is open (ZoomComponent object is created and displayed) and setting up a custom popStateEvent listener that prevents the default action handler of the back event and closes the preview instead. Once the preview is closed (ZoomComponent object is destroyed), the listener is deleted and the additional state item is removed from the history stack.

Aivean commented 4 months ago

Ok, I tested it locally, and the happy path works pretty well.

However, there is a minor yet annoying bug:

  1. Zoom image
  2. Go "back" (image closes, but the "forward" action is now available)
  3. Go "forward" (url doesn't change, but the history is navigated forward)
  4. Zoom another image
  5. Go "back" (image stays open, but the url and page may change)

I tried to fiddle with the history states to fix this, but no success so far.

If possible, please try to look into it. If it's too hard to fix, I think we can merge this as is and fix later, as the issue should not happen to often.

Aivean commented 4 months ago

This should work but as for me pushing new state is unnecessary complication. Instead it could be simply

  1. on ZoomComponent creating register popevent listener
  2. if it gets called - back button was clicked, preventDefault and close popup
  3. destroy listener at component destroy

I thought the same thing, but my attempt to implement that didn't work, that why I suggested to dig it ;)

4vanger commented 4 months ago

I thought the same thing, but my attempt to implement that didn't work, that why I suggested to dig it ;)

that's because popstate is not cancellable :( https://developer.mozilla.org/en-US/docs/Web/API/Window/popstate_event

Aivean commented 4 months ago

It seems like having forward button enable is unavoidable. I'll slightly clean up the code and leave it as is:

    useEffect(() => {
        const handleBack = (event: PopStateEvent) => props.onExit();
        window.history.pushState({popupOpen: true}, '');
        window.addEventListener('popstate', handleBack);
        return () => window.removeEventListener('popstate', handleBack);
    }, []);

what about the bug that going forward is causing? https://github.com/spaceshelter/orbitar/pull/381#issuecomment-2094326985

4vanger commented 4 months ago

It seems like having forward button enable is unavoidable. I'll slightly clean up the code and leave it as is:

    useEffect(() => {
        const handleBack = (event: PopStateEvent) => props.onExit();
        window.history.pushState({popupOpen: true}, '');
        window.addEventListener('popstate', handleBack);
        return () => window.removeEventListener('popstate', handleBack);
    }, []);

what about the bug that going forward is causing? #381 (comment)

With my proposed implementation the bug won't reproduce

Aivean commented 4 months ago

@di-maroo, do you mind trying @4vanger 's suggestion?

di-maroo commented 4 months ago

If we don't do history.back on closing the preview then our artificial state will keep being added to the stack as many times as we open an image, which mean we won't be navigated back once the image is closed for as many times as times the popup was open. Eg.: open the popup, close it with a cross, try navigating back. Expected: we go back in history. Actual: we stay on the page.

I'll clean up the code as suggested but we need to do "history.back" anyway if the current state contains {popupOpen: true}.