-
Notifications
You must be signed in to change notification settings - Fork 21
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
Properly scan messages when the mutated element is a descendant of a form #65
Properly scan messages when the mutated element is a descendant of a form #65
Conversation
src/index.ts
Outdated
@@ -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'); |
There was a problem hiding this comment.
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).
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
.
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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);
}
}
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
Co-authored-by: Keith Dahlby <dahlbyk@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haacked we can address #65 (comment) separately if someone asks for it (I don't think we need it), but for the original purpose I think this is good to go.
I noticed a situation in watch mode where I was adding form elements dynamically and the inputs themselves would validate, but the error messages wouldn't show up. I've modified
scanMessages
to use theclosest
method to find the enclosing form when the mutated element is a descendant of a form.