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

BUGFIX: Ensure that <DropDown/> closes only after the current call-stack has been processed #3322

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

grebaldi
Copy link
Contributor

@grebaldi grebaldi commented Jan 4, 2023

fixes: #3305

The Problem

The behavior described in #3305 is a regression through #3211.

#3211 made sure that the <Label/> around the auto-publish checkbox is clickable. Normally, one would expect that clicking on the label would trigger a change in the corresponding checkbox input through event delegation.

To illustrate that idea:

      (User clicks on <Label/>)
                │
                │ // Click event is being delegated to the corresponding
                │ // <input type="checkbox"> element
                │
                ▼
(<CheckBox/> OnChange event happens)

Clicking inside the <DropDown.Contents/> also causes the outer <DropDown/> to close. Closing the <DropDown/> means, that the <DropDown.Contents/> are removed from the DOM.

This update mechanism is handled by React in a way that is non-deterministic (at least to us it is). This means that sometimes the <DropDown.Contents/> are removed before the Click event from the <Label/> component reaches the corresponding checkbox input:

 (User clicks on <Label/>)
            │
            💥 ◀─────── (DropDown.Contents are removed from DOM)
            ┊
            ▼
(<CheckBox/> is gone...)

The Solution

I lieu of setImmediate-support in browsers, I wrapped the toggling mechanism of the <DropDown/>-component in a setTimeout(..., 0) call.

This makes sure that components inside the <DropDown.Contents/> will receive all events that are currently scheduled, before the <DropDown.Contents/> are removed from the DOM.

@grebaldi grebaldi mentioned this pull request Jan 4, 2023
36 tasks
@grebaldi grebaldi added the 7.3 label Jan 4, 2023
@grebaldi grebaldi linked an issue Jan 4, 2023 that may be closed by this pull request
Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx Wilhelm!

I don't like that we need this kind of workaround but I can confirm that it resolves the issue.
We should have a different component for this kind of mechanism which doesn't do event magic.

Copy link
Member

@crydotsnake crydotsnake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Works as expected :)!

@markusguenther markusguenther merged commit ab45a1d into neos:7.3 Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Toggling Auto Publish seems not to be working 50% of the time
4 participants