USWDS: Modal with nested forms. Resolves #4953 #5049

mahoneycm · 2022-11-21T17:13:15Z

Description

Resolves #4953

In its current iteration, the modal component watches all clicks while the modal is open and looks to see if it contains the CLOSER_ATTRIBUTE to trigger the modal to close. If it does not contain this attribute, it stops propagation which causes any other click events to fail.

As pointed out in #4953, this would cause some form elements to not function properly if they are nested in a modal. Consider login/registration pop-ups or scheduling assistants.

Based on the blame for that line of code, it appears the original modal creator tried to remove the stop propagation before but needed to maintain it because of a mobile safari bug. I'm not positive of what the bug was at this time but I believe this issue on stack overflow may be related. Here the user has a modal that closes when the input is tapped.

In this PR I've removed the stopPropagation function and created a modal with two form elements within it. After testing on various browsers and versions of mobile safari, it appears to work as expected.

Browsers Tested:

Various iPhones with different Mobile Safari versions (Latest is v16.0, oldest tested was v10.3)
Desktop browsers(Chrome, Safari, Edge)
Various Android devices and browsers (Chrome, Firefox, Samsung Browser)

To Test

Visit Nested Forms modal page
Open Modal
Test drop down components
Click out of modal view or use closer buttons to close

Currently pressing the escape key closes the entire modal, not just the focused element. We can discuss if we'd like to change this moving forward

Screenshots

Follow the 18F Front End Coding Style Guide and Accessibility Guide.
Run npm test and make sure the tests for the files you have changed have passed.
Run your code through HTML_CodeSniffer and make sure it’s error free.
Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

amyleadem

Nice work! The writeup is excellent and the fix feels clean. Thanks for building out a new story for demo and testing. This is all super helpful!

Everything seems to be working as expected in my browser testing. What do you think about adding a unit test for this functionality?

Also, I do think that there is an opportunity to improve the esc functionality here. I think it would be best if pressing esc while the form elements are open should close only the form element, not the modal. This could be out of scope for this issue though and might require further discussion.

Tested in:

Let me know if you have questions!

mejiaj

Looks good! I appreciate the very detailed PR notes.

Works on Safari iPhone 6 with iOS 13.4 (browserstack). I also tried testing this on an iPhone 5, but it didn't support JS template literals (`).

mahoneycm · 2023-01-05T15:09:13Z

@amyleadem @mejiaj Created a unit test for nested forms using a combo box! Passing as expected currently and breaks with stopPagination()

amyleadem · 2023-01-06T21:08:03Z

packages/usa-modal/src/test/modal-nested-form.spec.js

+        assert.strictEqual(activeContent[0], modalWrapper);
+      });
+
+      it("should show the list by clicking the toggle button", () => {


Wondering if this could be more clear. Maybe something like "Displays combobox list when clicked"?

Oh yeah I copied this from the combo box test, I'll update this to be more clear 😁

Do you think I should add a note about "Ensuring event propagation" at the end to explain why we did that test?

amyleadem · 2023-01-06T21:11:05Z

packages/usa-modal/src/test/modal-nested-form.spec.js

@@ -0,0 +1,161 @@
+const assert = require("assert");


I am curious why a new spec.js file and template were created for nested forms. The files seem very similar. Would it be possible to just add these updates and the ones from modal-nested-form.template.html to modal.spec.js and test/template.html instead of creating new files?

I believe I had a discussion with @mejiaj about the test and we mentioned creating a new test with the nested form. I also referenced combo box which has a different template and test for each of its variants.

Happy to make this the sole test if we prefer to do it all in one sweep!

@mejiaj do you have a preference on this?

@amyleadem @mahoneycm initially I suggested this as it's own test to keep the template and code simple. It looks like the test template and code are pretty much identical. In that case it makes sense to combine into one.

I'm okay if we simplify the test code to bare minimum or combine them into a single modal.spec.js.

amyleadem

Thanks for adding the unit test! The component and test both appear to work as expected. I had a couple of comments about the file structure and the test description, but other than that I think it is looking good.

amyleadem

Nice work! I approve, but am happy to defer to @mejiaj's preference about test file structure as described in this thread.

mahoneycm · 2023-01-27T16:37:58Z

@thisisdano Ready for final review!

mejiaj · 2023-02-06T17:16:50Z

@mahoneycm going to move this back to review since I haven't had a chance to look over the requested changes.

mejiaj

Functionality and test is good! Added some comments so this work is more in line with other component tests.

packages/usa-modal/src/usa-modal.stories.js

packages/usa-modal/src/usa-modal.twig

packages/usa-modal/src/test/modal.spec.js

mejiaj

@mahoneycm no issues with functionality, everything looks good. But ran into an issue on npm run build:html, which is what we use for the HTML code examples on USWDS-Site. Could you please take a look?

The compile error:

> @uswds/[email protected] build:html
> webpack build --config ./webpack.twig.config.js && npm run prettier:html

assets by status 709 KiB [cached] 193 assets
./html-templates/main.js 1 bytes [built] [code generated]

ERROR in Conflict: Multiple assets emit different content to the same filename usa-modal.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-modal~forced-action.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-modal~large.html

webpack 5.58.1 compiled with 3 errors in 1898 ms

… erros

…-components

mejiaj

I've made a small change to the test pattern structure in fec99fa. This doesn't affect the functionality ― just uses the standard test template pattern.

npm run build:html still works as expected.

mahoneycm added 2 commits November 21, 2022 11:15

Remove propogation blocker on modal

f1a4a66

Create modal with nested form example

2940cb5

mahoneycm requested review from mejiaj and amyleadem November 21, 2022 17:13

amyleadem reviewed Nov 23, 2022

View reviewed changes

mejiaj approved these changes Nov 30, 2022

View reviewed changes

mahoneycm added 4 commits November 30, 2022 16:54

Modal unit test experimentation

e379ab8

Continued modal testing

a587199

Restore modal spec testing

7bf6142

Create unit test for modal with nested forms

8fb36e5

mahoneycm requested review from amyleadem and mejiaj January 5, 2023 15:06

Revert unitTests src

d0e8665

amyleadem reviewed Jan 6, 2023

View reviewed changes

amyleadem suggested changes Jan 6, 2023

View reviewed changes

mahoneycm added 2 commits January 9, 2023 14:06

Added specificity to unit tests

4f62566

Consolidate modal unit tests

ea4ff28

mahoneycm requested a review from amyleadem January 18, 2023 22:16

amyleadem approved these changes Jan 19, 2023

View reviewed changes

mejiaj suggested changes Feb 6, 2023

View reviewed changes

packages/usa-modal/src/usa-modal.stories.js Outdated Show resolved Hide resolved

packages/usa-modal/src/usa-modal.twig Show resolved Hide resolved

packages/usa-modal/src/test/modal.spec.js Outdated Show resolved Hide resolved

mahoneycm added 3 commits February 6, 2023 13:26

Clean up story for the nested form modal

64e549d

Create separate nested forms modal template

a93f022

Use more descriptive variable name in modal unit test

577d1b9

mejiaj self-requested a review February 28, 2023 19:41

mejiaj suggested changes Feb 28, 2023

View reviewed changes

Nest nested forms template within it's own folder to avoid build:html…

49b725e

… erros

Merge branch 'develop' of github.com:uswds/uswds into cm-modal-nested…

0e823ec

…-components

mejiaj self-requested a review March 1, 2023 19:04

Use standard test template pattern

fec99fa

mejiaj approved these changes Mar 3, 2023

View reviewed changes

mejiaj requested a review from thisisdano March 3, 2023 22:56

thisisdano approved these changes Mar 7, 2023

View reviewed changes

thisisdano merged commit 6c11216 into develop Mar 7, 2023

thisisdano deleted the cm-modal-nested-components branch March 7, 2023 17:30

thisisdano mentioned this pull request Mar 9, 2023

USWDS 3.4.0 #5170

Merged

DaveSkender mentioned this pull request Mar 10, 2023

USWDS - Bug: Bad inits and teardowns on Uswds components when multiple instances in template in modern JS frameworks #5062

Closed

2 tasks

mejiaj mentioned this pull request Aug 2, 2023

USWDS - Bug: Modals break DOM #4916

Open

2 tasks

USWDS: Modal with nested forms. Resolves #4953 #5049

USWDS: Modal with nested forms. Resolves #4953 #5049

Uh oh!

Conversation

mahoneycm commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

To Test

Screenshots

Uh oh!

amyleadem left a comment

Choose a reason for hiding this comment

Uh oh!

mejiaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahoneycm commented Jan 5, 2023

Uh oh!

amyleadem Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

mahoneycm Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

mahoneycm Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

amyleadem Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

mahoneycm Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

amyleadem Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

mejiaj Jan 25, 2023

Choose a reason for hiding this comment

Uh oh!

amyleadem left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amyleadem left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahoneycm commented Jan 27, 2023

Uh oh!

mejiaj commented Feb 6, 2023

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

mejiaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mahoneycm commented Nov 21, 2022 •

edited

Loading

mejiaj left a comment •

edited

Loading

amyleadem left a comment •

edited

Loading

amyleadem left a comment •

edited

Loading

mejiaj left a comment •

edited

Loading