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

Properly scan messages when the mutated element is a descendant of a form #65

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Properly scan messages when the mutated element is a descendant of a form #65

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bb44e9
Properly scan messages when the mutated element is a descendant of a …
LiteracyFanatic Jul 31, 2023
d2b4570
Check that type of root is Element before using closest
LiteracyFanatic Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,11 @@ export class ValidationService {
// we could use 'matches', but that's newer than querySelectorAll so we'll keep it simple and compatible.
forms.push(root);
}
// If root is the descendant of a form, we want to include that form too.
const containingForm = (root as HTMLElement).closest('form');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's actually check the type to make sure DocumentFragment keeps working (#45).

Suggested change
const containingForm = (root as HTMLElement).closest('form');
const containingForm = (root instanceof Element) ? root.closest('form') : null;

Seems extremely unlikely there would be a form in non-HTML, but technically closest() comes from Element.

Copy link
Owner

Choose a reason for hiding this comment

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

I haven't had a chance to look at this closely, but when I see closest here, my concern is cases where an input is not inside a form, but declares a form. Do we handle that here?

<html>
<body>
<form id="some-form"></form>
<input name="whatevs" type="input" form="some-form" />
</body>
</html>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is currently being handled separately a few lines above.

/* If a validation span explicitly declares a form, we group the span with that form. */
let validationMessageElements = Array.from(root.querySelectorAll<HTMLElement>('span[form]'));
for (let span of validationMessageElements) {
    let form = document.getElementById(span.getAttribute('form'));
    if (form) {
        this.pushValidationMessageSpan(form, span);
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@LiteracyFanatic that code handles a span with a form attribute (non-standard HTML). @haacked is asking about an input (or select/textarea) that references a form that has not otherwise been discovered as descendent/self/ancestor of the scanned root.

I think he's right that scenario isn't covered anywhere, but I'd be content to wait for someone to report that as a bug with a motivating example. At this point we haven't necessarily scanned root's validatable elements, so it's not obvious how we'd identify which additional forms to scan for validation messages. The workaround would be to manually scan the other form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that makes sense. Thanks for the clarification.

if (containingForm) {
forms.push(containingForm);
}

for (let form of forms) {
let validationMessageElements = Array.from(form.querySelectorAll<HTMLElement>('[data-valmsg-for]'));
Expand Down