Skip to content

fix: Duplicate id in DOM for file field (bootstrap 4) #40565

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

Closed
wants to merge 2 commits into from
Closed

fix: Duplicate id in DOM for file field (bootstrap 4) #40565

wants to merge 2 commits into from

Conversation

theus77
Copy link

@theus77 theus77 commented Mar 23, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #40562
License MIT

This is causing an accessibility (WCAG) issue as specified here: https://www.w3.org/TR/WCAG20-TECHS/H93.html

@Nyholm
Copy link
Member

Nyholm commented Mar 24, 2021

To make it easier for me to review, could you show the HTML before and after?

@Nyholm
Copy link
Member

Nyholm commented Mar 24, 2021

If Im reading this correct. The id of the label, is the same as the id of the input. Hm.. Could you also show an example form.. This should really not be the case.

label_attr.id should be different from form.vars.id. Hm..

@theus77
Copy link
Author

theus77 commented Mar 24, 2021

If Im reading this correct. The id of the label, is the same as the id of the input. Hm.. Could you also show an example form.. This should really not be the case.

label_attr.id should be different from form.vars.id. Hm..

Here is the currently generated HTML:

<div class="form-group">
    <label data-browse="Kiezen" for="form_asset" id="form_asset_label" placeholder="Bestand">Bestand</label>
    <div class="custom-file">
        <input aria-describedby="form_asset_help" class="file asset custom-file-input" id="form_asset" lang="nl" name="form[asset]" type="file">
        <label class="custom-file-label" data-browse="Kiezen" for="form_asset" id="form_asset_label" placeholder="Bestand"></label>
    </div>
</div>

As you can see the label id is reused in the widget block

@Nyholm
Copy link
Member

Nyholm commented Mar 24, 2021

Could you show me the Form class that you are using to render this?

The id should be form_asset and form_asset_label if I remember correctly.

@Nyholm
Copy link
Member

Nyholm commented Mar 25, 2021

I just tested this in a new Symfony 4.4 installation. I cannot reproduce the bug you are describing.

This is my form:

class HelloType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('field_name', TextType::class)
        ;
    }
}

With standard form theme:

<div>
  <label for="hello_field_name" class="required">Field name</label>
  <input type="text" id="hello_field_name" name="hello[field_name]" required="required">
</div>

With bootstrap

<div class="form-group">
  <label for="hello_field_name" class="required">Field name</label>
  <input type="text" id="hello_field_name" name="hello[field_name]" required="required" class="form-control">
</div>

Feel free to make a PR to https://github.com/Nyholm/sf-issue-40565 to replicate this bug.

@stof
Copy link
Member

stof commented Mar 26, 2021

@Nyholm this PR is about the rendering of the file input in the bootstrap theme, not the text input

@Nyholm
Copy link
Member

Nyholm commented Mar 26, 2021

Oops. Thank you @stof. You are correct. But I fail to reproduce it on the FileType too.

    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('field_name', FileType::class)
        ;
    }
<div class="form-group">
    <label for="hello_field_name" class="required">Field name</label>
    <div class="custom-file"><input type="file" id="hello_field_name" name="hello[field_name]" required="required" class="custom-file-input">
    <label for="hello_field_name" class="custom-file-label"></label>
    </div>
</div>

However, this form has multiple labels...

@stof
Copy link
Member

stof commented Mar 26, 2021

hmm, the issue seems to happen when you put an id in your label_attr (which is indeed not done by default).

@Nyholm
Copy link
Member

Nyholm commented Mar 26, 2021

Here is the bootstrap_4_horizontal_layout.html.twig:

<div class="form-group row">
  <label class="col-form-label col-sm-2 required" for="hello_field_name">Field name</label>
  <div class="col-sm-10">
    <div class="custom-file">
      <input type="file" id="hello_field_name" name="hello[field_name]" required="required" class="custom-file-input">
      <label for="hello_field_name" class="custom-file-label"></label>
    </div>
  </div>
</div>

@theus77, I suspect you have a lot of custom form types in your application and one of them is not acting properly.

@Nyholm
Copy link
Member

Nyholm commented Mar 26, 2021

@stof, you are correct again! =)

    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('field_name', FileType::class, [
                'label_attr' => ['id'=>'foo']
            ])
        ;
    }
<div class="form-group">
  <label id="foo" for="hello_field_name" class="required">Field name</label>
  <div class="custom-file">
    <input type="file" id="hello_field_name" name="hello[field_name]" required="required" class="custom-file-input">
    <label for="hello_field_name" id="foo" class="custom-file-label"></label>
  </div>
</div>

But hm... The only two fields with the same ID is the two <label>...

@stof
Copy link
Member

stof commented Mar 26, 2021

I'm wondering whether the custom label added for the custom-file-label should use label_attr at all.

@theus77
Copy link
Author

theus77 commented Mar 29, 2021

I see that you reproduced it on your own. Sorry I was not able to follow on this before now.

I agree with @stof that extra label should not reuse label_attr at all (less code). I updated my PR in consequence.

@@ -123,7 +123,7 @@
{%- set type = type|default('file') -%}
{{- block('form_widget_simple') -}}
{%- set label_attr = label_attr|merge({ class: (label_attr.class|default('') ~ ' custom-file-label')|trim }) -%}
<label for="{{ form.vars.id }}" {% with { attr: label_attr } %}{{ block('attributes') }}{% endwith %}>
Copy link
Member

Choose a reason for hiding this comment

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

The attributes may have many other values. We cannot just remove this.

I think we need to make a patch in a different place. Not sure where though..

@xabbuh
Copy link
Member

xabbuh commented Jul 10, 2021

Thank you for starting the work on this @theus77. I have finished this in #42049.

@xabbuh xabbuh closed this Jul 10, 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.

5 participants