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

Tooltip closing race condition with children change callback #997

Closed
FRosner opened this issue Jan 15, 2024 · 10 comments
Closed

Tooltip closing race condition with children change callback #997

FRosner opened this issue Jan 15, 2024 · 10 comments

Comments

@FRosner
Copy link

FRosner commented Jan 15, 2024

  • dash version: #2.14.2
  • dash-bootstrap-components version: #1.5.0
  • components affected by bug: tooltip

What is happening?

In a dash SPA when opening a page with a dbc tooltip, then navigating to a new page, the page fails to render with

Error: An object was provided as `children` instead of a component, string, or number (or list of those). Check the children property that looks something like:
{
  "3": {
    "props": {
      "is_open": true
    }
  }
}

It's not specific to the SPA callback. I managed to reproduce it with a callback that upon clicking a button with a tooltip removes the button and the tooltip from the children of their parents.

See https://stackoverflow.com/questions/75175955/an-object-was-provided-as-children-instead-of-a-component-error-in-python-da for more details.

What should be happening?

The tooltip should be removed and the client side callback should not attempt to patch the non-existing tooltip component, effectively corrupting the children property.

Code

mport dash
from dash import dcc
from dash import html
import dash_bootstrap_components as dbc
from dash.dependencies import Input, Output

app = dash.Dash(__name__, suppress_callback_exceptions=True)


app.layout = html.Div([
    dcc.Location(id='url', refresh=False),
    html.Div(id='page-content'),
])


def get_overview_page():
    page = html.Div([
        dcc.Link(
            html.Button('Neu', id='new_button',
                        style={'margin-left': '10px', 'width': '100px', 'height': '27px',
                               'fontSize': '16px'}),
            href='/new-entry'
        ),
        dbc.Tooltip(
            "A.",
            target="new_button", placement="top"
        )
    ], style={'width': '30%', 'margin-top': '10px', 'display': 'inline-block', 'text-align': 'left'})
    return page


# Update the index
@app.callback(Output('page-content', 'children'),
              [Input('url', 'pathname')])
def display_page(pathname):
    if pathname == '/new-entry':
        return html.Div()
    else:
        return get_overview_page()

if __name__ == '__main__':
    app.run_server(debug=True, port=8086, host='127.0.0.1')

Error messages

Error: An object was provided as `children` instead of a component, string, or number (or list of those). Check the children property that looks something like:
{
  "1": {
    "props": {
      "is_open": true
    }
  }
}
@FRosner
Copy link
Author

FRosner commented Jan 15, 2024

I wonder if this is caused by fade or delay? I'm trying to find the code that updates the is_open erroneously... @tcbegley can you help me?

Is it here?

const hide = () => {
if (isOpenRef.current) {
hideTimeout.current = clearTimeout(hideTimeout.current);
setIsOpen(false);
if (setProps) {
setProps({is_open: false});
}
}
};
// Function to create the delay for hiding if currently open
const hideWithDelay = () => {
if (!isOpenRef.current && showTimeout.current) {
showTimeout.current = clearTimeout(showTimeout.current);
hide();
} else if (isOpenRef.current) {
clearTimeout(hideTimeout.current);
hideTimeout.current = setTimeout(hide, delay.hide);
}
};

If it's delay, setting delay={"show": 0, "hide": 0}, should be a reasonable workaround.

@tcbegley
Copy link
Collaborator

tcbegley commented Jan 16, 2024

Hi @FRosner,

Thanks for reporting this and for providing so much detail and a minimal reproduction.

The issue is indeed in hideWithDelay. When the "close" even happens, we call setProps({is_open: false}) with a delay. In this case in the intervening time the component is unmounted. That means when Dash tries to update the prop the component no longer exists.

I think the fix here will be to either:

  1. modify the timeout so that the call to setProps only happens if the component hasn't been unmounted
  2. cancel outstanding timeouts when the component unmounts

I think 2) is probably cleaner but looking over the code briefly might require a bit of refactoring. I'll see what I can do.

@FRosner
Copy link
Author

FRosner commented Jan 17, 2024

I'll see what I can do.

Thank you so much!

@FRosner
Copy link
Author

FRosner commented Jan 17, 2024

I think the fix here will be to either:

To me, 1 sounds preferrable, as it has limited side effects and makes it clearer when the function is passed to setTimeout that this is something to be aware of. Why do you think 2 is cleaner?

@tcbegley
Copy link
Collaborator

Why do you think 2 is cleaner?

Well the timeouts only make sense in the context of the component, so if the component no longer exists it seems neater to stop them from running at all rather than to run and do little / nothing. But tbh both options are similar so I'll take whatever solution is simpler to implement.

I did have a go at getting this to work yesterday but it turned out to be fiddlier than expected. I might need to refresh some of my react knowledge and try again when I get a chance.

@FRosner
Copy link
Author

FRosner commented Jan 18, 2024

Thanks! I am trying the workaround of setting hide delay to 0 in the meantime. I'd also take a stab at it but last time I used react is 5 years ago :D

@jonicohn
Copy link

jonicohn commented Apr 2, 2024

Small addition from my side:
I assume the same happens with the alert component and duration.
It gave me a headache why the app raised an error after a few seconds when switching pages in a multi-page app, displaying the new page correctly. More confusingly for me: it sometimes took a bit longer, but in the end, it was related to how fast you switched to the new page before the duration time ran out.

@tcbegley
Copy link
Collaborator

tcbegley commented Apr 2, 2024

Hi @jonicohn

Which version of dash-bootstrap-components are you using? It sounds like what you're describing could be related to this issue which should have been fixed in v1.4.2

@jonicohn
Copy link

jonicohn commented Apr 3, 2024

Hi tcbegley,

sorry, my bad. You are absolutely correct. 1.5.0 is working fine.

@tcbegley
Copy link
Collaborator

Sorry this took some time, it took me a while to get my head around it. It's now fixed in the latest version.

pip install -U dash-bootstrap-components

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants