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.

Sign up for GitHub

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

Jump to bottom

bugfix/437-close-project-modal-on-navigation #456

Closed
wants to merge 4 commits into from
Closed

bugfix/437-close-project-modal-on-navigation #456

wants to merge 4 commits into from

Conversation

waltersajtos
Copy link
Member

Description

When a project-detail-modal was opened and the user went back using the browser navigation they would be navigated to the previous page. The expected behavior is to close the opened modal.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I updated the changelog with an end-user readable description
  • I assigned this pull request to the correct project board to update the sprint board

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.
These steps will be used during release testing.

  1. Open a project modal
  2. Click the navigation back button (top-left of your browser or back button on your mouse)
  3. The modal should close and you should still be on the overview page

Link to issue

Closes: #437

@waltersajtos
Copy link
Member Author

This solution works 90% of the time, there is one race condition. I will try to explain what's going on.

  • When the user opens a modal I'm adding an extra entry into the window.history
  • When the user navigates back with the browser navigation this entry is removed and the popState event is triggered
  • In the popState event listener I close the modal.

This flow works as expected but we still have the normal behavior of the modal where it closes when you click the backdrop

  • The user closes the modal by clicking the backdrop, this triggers the modal.onHide event
  • In the onHide event I remove the extra history entry I made with history.back(), the modal is already closed at this time.
  • The popstate event still gets triggered because of the history.back() call, this is not a problem in most situations because there is no modal to hide.

The problem occurs when the user closes the modal using the browser navigation and then instantly clicks a new one. This causes a race condition between the modal.onHide listener and the onPopState event listener. The history.back() is called by the onHide listener but then the onPopState tries to close the modal too which does exist at this time because the user already opened a new modal before the onPopState listener completed running.

@waltersajtos
Push an extra entry into the history so the user won't navigate when …
c308985 
…they use browser navigation
@waltersajtos
Merge remote-tracking branch 'origin/bugfix/437-close-project-modal-o…
7f8fbc5 
…n-navigation' into bugfix/437-close-project-modal-on-navigation

# Conflicts:
#	src/app/modules/project/overview/overview.component.ts
@waltersajtos waltersajtos changed the title bugfix/437-close-projectl-modall-on-navigation bugfix/437-close-project-modal-on-navigation Apr 5, 2021
@waltersajtos waltersajtos self-assigned this Apr 5, 2021
@niraymak
Copy link
Member

niraymak commented Apr 6, 2021 •
edited

This solution works 90% of the time, there is one race condition. I will try to explain what's going on.

  • When the user opens a modal I'm adding an extra entry into the window.history
  • When the user navigates back with the browser navigation this entry is removed and the popState event is triggered
  • In the popState event listener I close the modal.

This flow works as expected but we still have the normal behavior of the modal where it closes when you click the backdrop

  • The user closes the modal by clicking the backdrop, this triggers the modal.onHide event
  • In the onHide event I remove the extra history entry I made with history.back(), the modal is already closed at this time.
  • The popstate event still gets triggered because of the history.back() call, this is not a problem in most situations because there is no modal to hide.

The problem occurs when the user closes the modal using the browser navigation and then instantly clicks a new one. This causes a race condition between the modal.onHide listener and the onPopState event listener. The history.back() is called by the onHide listener but then the onPopState tries to close the modal too which does exist at this time because the user already opened a new modal before the onPopState listener completed running.

I discussed this with Ruben and we think we should find a workaround before merging this branch. But I think it is something for next sprint.

@waltersajtos
Copy link
Member Author

This solution works 90% of the time, there is one race condition. I will try to explain what's going on.

  • When the user opens a modal I'm adding an extra entry into the window.history
  • When the user navigates back with the browser navigation this entry is removed and the popState event is triggered
  • In the popState event listener I close the modal.

This flow works as expected but we still have the normal behavior of the modal where it closes when you click the backdrop

  • The user closes the modal by clicking the backdrop, this triggers the modal.onHide event
  • In the onHide event I remove the extra history entry I made with history.back(), the modal is already closed at this time.
  • The popstate event still gets triggered because of the history.back() call, this is not a problem in most situations because there is no modal to hide.

The problem occurs when the user closes the modal using the browser navigation and then instantly clicks a new one. This causes a race condition between the modal.onHide listener and the onPopState event listener. The history.back() is called by the onHide listener but then the onPopState tries to close the modal too which does exist at this time because the user already opened a new modal before the onPopState listener completed running.

I discussed this with Ruben and we think we should find a workaround before merging this branch. But I think it is something for next sprint.

Yeah I agree, I will look into it next sprint.

@waltersajtos
Merge branch 'develop' into bugfix/437-close-project-modal-on-navigation
bcd9291 
# Conflicts:
#	src/app/modules/project/overview/overview.component.ts
@niraymak niraymak closed this Jun 23, 2021
@niraymak
Copy link
Member

Closed due inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RubenFricke RubenFricke Awaiting requested review from RubenFricke

@macfleury-2000 macfleury-2000 Awaiting requested review from macfleury-2000

@niraymak niraymak Awaiting requested review from niraymak

Assignees

@waltersajtos waltersajtos

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Close project modal on going back
2 participants
@waltersajtos @niraymak