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.

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

#2979 : Form controls outside form hierarchy not validated #2980

Closed
wants to merge 1 commit into from
Closed

#2979 : Form controls outside form hierarchy not validated #2980

wants to merge 1 commit into from

Conversation

timrobinson33
Copy link

WARNING: This fix is dependent on the fix for #2628 which is at
https://github.com/ccwebdesign/jsdom but does not currently have a PR.

Sorry - I'm not sure what the protocol is for making a fix to an unreleased fix

As of the current released version, the elements collection does not include elements outside the form hierarchy. Once the fix for #2628 corrects it, this fix then allows the validation to reference the elements collection which will result in the correct validation being performed

@domenic
Copy link
Member

domenic commented Jul 3, 2020

Can you add new tests, or find tests to enable, for this?

@timrobinson33
Copy link
Author

@domenic I can't write tests at the moment because my fix won't do anything until #2628 is merged. When there are tests for that, it will be easy to add tests for this. In fact since my fix is a single-line and this is is arguably the same problem as #2628, it might be easier to bundle this into it.

@domenic
Copy link
Member

domenic commented Jul 4, 2020

Note that #2628 is not a pull request, so it's not something I can "merge".

@timrobinson33
Copy link
Author

@domenic Sorry I'm not that familiar with contributing to open source projects and I wasn't quite sure what to do so I apologise if I've broken the protocol. I just wanted to make it clear that there is a simple fix for this issue, even though I understand it can't be merged at the moment because it depends on #2628 which seems to be stalled. I'm happy for you to just delete the pull request if you think it's not appropriate to have it.

@ccwebdesign
Copy link
Contributor

@timrobinson33 Sorry -- didn't realize you were waiting on me. Created #3003

@domenic
Copy link
Member

domenic commented Jul 11, 2020

#3003 is now merged!

domenic added a commit that referenced this pull request Mar 28, 2021
domenic added a commit that referenced this pull request Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants