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

Modal beforeClose behaves differently when removing open attribute #6407

Closed
nwhittaker opened this issue Feb 1, 2023 · 6 comments
Closed

Modal beforeClose behaves differently when removing open attribute #6407

nwhittaker opened this issue Feb 1, 2023 · 6 comments
Assignees
@geospatialem
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 8 Requires input from team, consider smaller steps. has workaround Issues have a workaround available in the meantime. research Issues that require additional research in order to resolve or make decision.
Milestone
2023 August Prior...

Comments

@nwhittaker
Copy link
Contributor

nwhittaker commented Feb 1, 2023 •
edited

Actual Behavior

Closing a modal via its built-in UI (close button, escape key, or outside click) can be interrupted by having its beforeClose() callback return a rejected promise.

However closing a modal by removing its open attribute can not be interrupted in the same way. The modal also enters into an inconsistent state where its open and isOpen properties are out-of-sync and an overflow-hidden class remains on the document element.

Expected Behavior

Closing the modal by removing the open attribute and having its beforeClose() callback return a rejected promise does no leave the modal in an inconsistent state.

Ideally the open attribute is restored, but if not, maybe generate a warning for this scenario and then provide an API method that can be used to programmatically close a modal in a way that is interruptible by the beforeClose() callback.

Reproduction Sample

https://codepen.io/nwhittaker-esri/pen/JjBeJVr

Reproduction Steps

  1. Visit the pen and click the Open button to open the modal.
  2. Click the modal's X button and see it does not close, as expected because beforeClose returns a rejected promise.
  3. Click the modal's Close button and see the modal disappears, unexpectedly.
  4. Use devtools to see the pen iframe's <html> element retains the modal's overflow-hidden CSS class and the .modal element within the modal's shadow-dom retains the modal--open class.

Reproduction Version

1.0.3

Relevant Info

No response

Regression?

No response

Impact

Having beforeClose() reject is useful for flows where the app needs to confirm the closure of a modal.

One workaround is for the consumer to run beforeClose() themselves before actually removing the open attribute. However this leads to some more complicated code to prevent beforeClose() from running more than once per close attempt.

Esri team

ArcGIS Field Apps
@nwhittaker nwhittaker added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Feb 1, 2023
@github-actions github-actions bot added the ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. label Feb 1, 2023
@geospatialem geospatialem mentioned this issue Mar 31, 2023
Closed
@brittneytewks brittneytewks added the research Issues that require additional research in order to resolve or make decision. label Apr 11, 2023
@geospatialem
Copy link
Member

Research needed to determine next steps, if related to #6279, or if additional mitigation is needed.

@geospatialem geospatialem removed needs triage Planning workflow - pending design/dev review. airtable labels May 25, 2023
@brittneytewks
Copy link

Hey @driskull, for research consideration in this milestone for time estimate and priority to be added

@driskull
Copy link
Member

Maybe we need to introduce a public close method that would handle this and return a promise. cc @jcfranco

Use devtools to see the pen iframe's element retains the modal's overflow-hidden CSS class and the .modal element within the modal's shadow-dom retains the modal--open class.

I think its weird that the watcher is calling beforeClose. The watcher should just honor what was done and remove the necessary classes.

@driskull driskull added estimate - 3 A day or two of work, likely requires updates to tests. 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels May 25, 2023
driskull added a commit that referenced this issue May 26, 2023
@driskull
fix(modal): changing the open property should not call beforeClose. #…
569f64f 
…6407

introduces a close public method which will call beforeClose
@driskull driskull mentioned this issue May 26, 2023
Closed
@driskull driskull added estimate - 8 Requires input from team, consider smaller steps. and removed estimate - 3 A day or two of work, likely requires updates to tests. labels May 26, 2023
@driskull
Copy link
Member

Changed the estimate since this requires a new pattern discussion.

@jcfranco jcfranco added the has workaround Issues have a workaround available in the meantime. label May 31, 2023
@driskull driskull added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Jun 6, 2023
driskull added a commit that referenced this issue Aug 7, 2023
@driskull
fix(modal): Handle removal of open attribute and prevent calling befo…
a26b38d 
…reClose twice. #6407 #6379
@driskull driskull mentioned this issue Aug 7, 2023
Merged
driskull added a commit that referenced this issue Aug 23, 2023
@driskull
fix(modal): handle removal of open attribute and prevent multiple bef…
f31588f 
…oreClose calls (#7470)

**Related Issue:** #6407 #6379

## Summary

- Add internal property `opened` to maintain open state when beforeBack
is rejected and to handle initial animation.
- Refactor `beforeClose` default value.
- Rename internal close method name to be consistent with internal open
method name
- Prevent `beforeClose` from being called twice by adding an internal
flag to ignore watch changes when necessary.
- Tests
@driskull driskull added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Aug 23, 2023
@github-actions github-actions bot assigned geospatialem and unassigned driskull Aug 23, 2023
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Aug 27, 2023
@geospatialem
Copy link
Member

Verified in 1.7.0-next.9.

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

@geospatialem geospatialem

Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 8 Requires input from team, consider smaller steps. has workaround Issues have a workaround available in the meantime. research Issues that require additional research in order to resolve or make decision.
Projects
None yet
Milestone
2023 August Priorities
Development

No branches or pull requests
6 participants
@jcfranco @nwhittaker @driskull @alisonailea @geospatialem @brittneytewks