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

Dialog: onHide callback should be called when onExited callback is called. #6448

Closed
didix16 opened this issue Apr 22, 2024 · 8 comments · Fixed by #6458, #6567, leoo1992/GeradorQRCode#23, leoo1992/GeradorQRCode#27 or leoo1992/GeradorQRCode#40
Assignees
Labels
Component: Documentation Issue or pull request is related to Documentation
Milestone

Comments

@didix16
Copy link
Contributor

didix16 commented Apr 22, 2024

Describe the bug

Hello there,
I'm having some wierd behaviour (maybe missunderstanding the component, i don't know) with Dialog component. As I could see in examples docs and source code, when you make click on close icon, the onClose callback is called, calling at the same time onHide if it is defined.

const onClose = (event) => {
props.onHide();
event.preventDefault();
};

However, when you have to 'close' the dialog by some custom button in dialog footer, the only way to 'close' it is calling some method to make the property visible to false and then the dialog becomes hidden but there is no call to onHide dialog, which is wierd because I need to execute some logic when onHide is called but I need that logic to be executed when user press the footer button or closes the dialog.
Yes, I know I can define a onHideCallback and set it to the footer button and the onHide prop of the dialog but then something happens. At some point, the callback is called twice in a row and I'm not getting the expected behavior that I need.
image

By the way, I'm gonna explain what I'm trying to do and set a reporducer to stackbliz, demonstrating my issue.

So what I want to do is basically to show a dialog if the value of the Autocomplete is none of the list. The check process must be executed when the user hits 'Tab' or 'Enter' key inside the input of the Autocomplete or at onBlur callback from Autocomplete.

At the moment, the implementation of pressing Enter, works great. If you leave 'Enter' key pressed, it appears the Dialog, with button autofocus, so the dialog closes, returning the focus to Autocomplete and then the callback onKeyPress is executed again, showing the Dialog, and so on...

But the problem occurs when I want to implement the onBlur behavior. I need that when dialog closes, it must focus again on Autocomplete, forcing the user to select a value from the list.
However, If I force the focus using .focus() method, the next time I leave pressed the 'Enter' key, the Dialog shows but neither without the modal effect (backdrop), nor the focus on the 'Ok' footer button which calls the setVisible(false) to hide the dialog.

So thus is why I'm trying to implement a workarround that only calls .focus() when the dialog is shown onBlur method and not when coming from keydown 'Enter' or 'Tab'.
But again, as I said before, at some point the onHide callback its been called twice, and interrupting the functionality of my workarround.

Maybe executing onHide method from dialog prop only when the dialog become hidden it fixes the bug.

I see in code that onEntered callback calls the onShow callback

const onEntered = () => {
props.onShow && props.onShow();
if (props.focusOnShow) {
focus();
}
enableDocumentSettings();
};

but the onExited callback does not call the onHide callback

const onExited = () => {
dragging.current = false;
ZIndexUtils.clear(maskRef.current);
setMaskVisibleState(false);
disableDocumentSettings();
// return focus to element before dialog was open
DomHandler.focus(focusElementOnHide.current);
focusElementOnHide.current = null;
};

So, why not calling onHide inside onExited before focus the element, for example?

Sorry for my bad english. I hope the problem is understood

Reproducer

https://stackblitz.com/edit/qezure?file=src%2FApp.jsx

PrimeReact version

10.6.3

React version

18.x

Language

TypeScript

Build / Runtime

Vite

Browser(s)

No response

Steps to reproduce the behavior

  1. Type 'i' for example in autocomplete
  2. Then leave Enter key pressed
  3. Second time that Dialog shows, Dialog appears without backdrop and without focus.

Expected behavior

On Enter keypress without leaving it, it should keep open the dialog and then closing it indefinitely until the Enter key is released.

EDIT:

I just see this comment: #880 (comment) referencing a similar issue with onHide callback.

I don't know why, if I add

if (!visible) return; // <== This should be added to work nicely!

at the top of handleDialogHide (which is the callback for onHide dialog property), then the behavior is the expected one.

But as I said before, I think we should use our custom logic without workarrounds. I think the custom logic should be called onHide callback even when a custom button changes the visibility of dialog to false (that is to say, when you want to close the dialog).

This is the new reproducer with the workarround that demonstrates the expected behaviour.

Reproducer

https://stackblitz.com/edit/qezure-z3uhd9?file=src%2FApp.jsx

@didix16 didix16 added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Apr 22, 2024
@melloware melloware added Type: Bug Issue contains a defect related to a specific component. and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Apr 22, 2024
@melloware melloware added this to the 10.6.4 milestone Apr 24, 2024
melloware pushed a commit that referenced this issue Apr 24, 2024
@didix16
Copy link
Contributor Author

didix16 commented Apr 26, 2024

Thank you guys @melloware @Rekl0w

@melloware
Copy link
Member

Looks like #6562 is now an issue which might need a tweak to this fix.

@melloware melloware reopened this May 3, 2024
@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label May 3, 2024
@melloware
Copy link
Member

Reverting this fix as it broke a new scenario. @Rekl0w will explain.

@melloware melloware removed the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label May 3, 2024
@didix16
Copy link
Contributor Author

didix16 commented May 3, 2024

Mmm I just seen the #6562 issue. So at the moment I supose I have to do the workarround meantime. I'll wait @Rekl0w explanation. Thanks @melloware

@Rekl0w
Copy link
Contributor

Rekl0w commented May 3, 2024

Simply, you have to do the workaround, it is the correct usage. Technically #880 (comment) you need to use it like this code. For now, do the workaround. I will try to explain deeply if I can find free time. Thanks for the patience. @didix16

@melloware
Copy link
Member

Yep according to PrimeTek your workaround is the correct workaround according to #880

@melloware melloware closed this as not planned Won't fix, can't repro, duplicate, stale May 3, 2024
@melloware melloware added Resolution: Workaround Issue or pull request contains a workaround. It needs to be reviewed further by Core Team and removed Type: Bug Issue contains a defect related to a specific component. labels May 3, 2024
@melloware melloware removed this from the 10.6.4 milestone May 3, 2024
@didix16
Copy link
Contributor Author

didix16 commented May 4, 2024

I see. Maybe some tips in Dialog documentation (i.e, include workaround in the examples and explain it) will help other people avoid searching workarounds/duplicate issues of same stuff.

Thanks for your work guys.

@melloware melloware added Component: Documentation Issue or pull request is related to Documentation and removed Resolution: Workaround Issue or pull request contains a workaround. It needs to be reviewed further by Core Team labels May 4, 2024
@melloware melloware added this to the 10.7.0 milestone May 4, 2024
@melloware
Copy link
Member

Agreed I just changed this to a Documentation ticket.

@melloware melloware reopened this May 4, 2024
@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label May 4, 2024
melloware added a commit to melloware/primereact that referenced this issue May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment