Skip to content

BUGFIX: Ad id attribute to form fields#86

Open
jonnitto wants to merge 2 commits intomainfrom
bugfix/adIdToFormFields
Open

BUGFIX: Ad id attribute to form fields#86
jonnitto wants to merge 2 commits intomainfrom
bugfix/adIdToFormFields

Conversation

@jonnitto
Copy link
Copy Markdown
Member

Closes #85

@jonnitto jonnitto requested a review from mficzel August 29, 2023 19:16
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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

@lorenzulrich
Copy link
Copy Markdown

"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