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

BUGFIX: Ad id attribute to form fields #86

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonnitto
Copy link
Member

Closes #85

@jonnitto jonnitto requested a review from mficzel August 29, 2023 19:16
Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

I like

@mficzel
Copy link
Member

mficzel commented Oct 6, 2023

Spec says that about identifiers: see https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id

Technically, the value for an id attribute may contain any character, except whitespace characters. However, to avoid inadvertent errors, only ASCII letters, digits, '_', and '-' should be used, and the value for an id attribute should start with a letter.

@lorenzulrich
Copy link

Not sure if I'm correct, but wouldn't this change lead to the id attribute value potentially starting with a number? Shouldn't it be 'field_' + field.getName() as it is/will be in PagerTiger?

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

Not so sure anymore as rendering an identifier by default is quite invasive. Assume there are better options to achieve the same.

@lorenzulrich
Copy link

lorenzulrich commented Nov 17, 2023

Having an id attribute is the only way to associate an input with a label. Therefore, if a form field is used with a label, there is no way around an id attribute. I checked on other solutions, such as TYPO3 Powermail, TYPO3 Form Framework, WordPress Gravity Forms, all of them have id attributes by default. Personally, I think it's a no-brainer to just have an id attribute.

But if we don't want this in Neos.Fusion.Form, we should at least be able to activate the id attribute:

showIdAttribute = false

        renderer = afx`
            <input
                id={props.showIdAttribute ? field.getName() : ''}

(Or similar)

@mficzel
Copy link
Member

mficzel commented Nov 17, 2023

We should have a logic that ensures uniqueness for id's or use the optIn as suggested. Without that we can easily cause issues for documents that render multiple forms and wich very likely have some fields in common.

Maybe we allow to set the id on the fieldContainer and only after that is done render it for label and field.

@lorenzulrich
Copy link

Yes, the uniqueness should be established using a "namespace", in Neos.Fusion.Form, this is done using the namespace property:

prototype(My.Site:FooForm) < prototype(Neos.Fusion.Form:Runtime.RuntimeForm) {
    namespace = 'foo_form'

field.getName() always returns the namespaced field name, therefore I think it would be (almost) safe to use as identifier.

@lorenzulrich
Copy link

@mficzel You merged sitegeist/Sitegeist.PaperTiger#18 in the meantime, but this one is still unmerged. How should be progress here?

@lorenzulrich
Copy link

"Self correction": sitegeist/Sitegeist.PaperTiger#18 doesn't depend on this issue, it works well as long as there is only one form (or unique form field names) on a page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The inputs field have no id attribute
3 participants