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

Added novalidate to boolean attributes array #273

Merged

Conversation

krytenuk
Copy link
Contributor

I have added novalidate to the AbstractHelper::$booleanAttributes array as this is a boolean attribute in HTML 5.
Currently setting the Element::setAttribute('novalidate', true) in the form when the doc-type is set to HTML5, renders novalidate="1" rather than just novalidate.
https://html.spec.whatwg.org/#attr-fs-novalidate

@krytenuk krytenuk force-pushed the fix/add-novalidate-to-boolean-attributes branch from dea11fd to 65f94da Compare June 29, 2024 19:36
@gsteel
Copy link
Member

gsteel commented Jun 29, 2024

@krytenuk - The novalidate attribute is only applicable to <form> elements and the view helper for forms already has the novalidate attribute listed:

https://github.com/laminas/laminas-form/blob/3.20.x/src/View/Helper/Form.php#L28-L37

@krytenuk
Copy link
Contributor Author

krytenuk commented Jun 30, 2024

@gsteel, thank you for your response.
Yes the attribute is set in the form view helper but not as a boolean value, the only place to currently set this is in the abstract view helper. This Without specifying the novalidate attribute as a boolean attribute, https://github.com/krytenuk/laminas-form/blob/fix/add-novalidate-to-boolean-attributes/src/View/Helper/AbstractHelper.php#L62, it does not render correctly.
The boolean attribute's check and rendering was implemented in #108.

Before implementing my fix, setting the novalidate attribute to true, Element::setAttribute('novalidate', true), renders, novalidate="1".
And setting the attribute to false, Element::setAttribute('novalidate', false), renders, novalidate="".
Also setting the attribute to null Element::setAttribute('novalidate', null) or an empty string, Element::setAttribute('novalidate', '') renders the same novalidate="".
As far as I am aware, all these are rendering incorrectly as novalidate is a boolean attribute according to https://html.spec.whatwg.org/#attr-fs-novalidate, please correct me if I am mistaken on this.

After adding the attribute to the AbstractHelper::$booleanAttributes array, the novalidate attribute renders correctly.
Element::setAttribute('novalidate', true) just renders novalidate and Element::setAttribute('novalidate', false) causes the novalidate attribute not to render. I believe this should be the expected behaviour.
Using truthy and falsy values also work.

The current logic will prevent the novalidate being rendered on any other form element as the check for this is done, https://github.com/krytenuk/laminas-form/blob/fix/add-novalidate-to-boolean-attributes/src/View/Helper/AbstractHelper.php#L396, using the array you highlighted above.

src/View/Helper/AbstractHelper.php Outdated Show resolved Hide resolved
@krytenuk krytenuk force-pushed the fix/add-novalidate-to-boolean-attributes branch from fba52df to d986888 Compare July 2, 2024 20:21
Signed-off-by: Garry Childs <garry.childs@xigen.co.uk>
Signed-off-by: Garry Childs <garry.childs@xigen.co.uk>
Signed-off-by: Garry Childs <garry.childs@xigen.co.uk>
@krytenuk krytenuk force-pushed the fix/add-novalidate-to-boolean-attributes branch from 7d8161c to 65a43cf Compare July 3, 2024 06:46
src/View/Helper/AbstractHelper.php Outdated Show resolved Hide resolved
Co-authored-by: George Steel <george@netglue.uk>
Signed-off-by: Garry Childs <info@freedomwebservices.net>
@Slamdunk Slamdunk added this to the 3.20.0 milestone Jul 8, 2024
@Slamdunk Slamdunk merged commit a50af07 into laminas:3.20.x Jul 8, 2024
11 of 12 checks passed
@Slamdunk
Copy link
Contributor

Slamdunk commented Jul 8, 2024

Thank you @krytenuk

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.

5 participants