Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Steps to reproduce this issue:
- Add a file field to the 'page' node-type, say 'field_file'.
- Go to
/node/add/page
. - Inspect the file widget markup. You'll find this piece of markup:
<div id="edit-field-file-0-upload" class="js-form-managed-file form-managed-file"> <input data-drupal-selector="edit-field-file-0-upload" type="file" id="edit-field-file-0-upload" name="files[field_file_0]" size="22" class="js-form-file form-file"> ... </div>
Note that the file input field and the div wrapper are sharing the same ID: edit-field-file-0-upload
.
Proposed resolution
- Add a new form element property
#label_for
which allows to associate the element labe with a different field by setting this property the ID of the desired field. - Manually generate an ID for the file input so that we can instruct the form element label to point to the file input element.
- Add a regression test.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#92 | 2497909-92.patch | 7.29 KB | claudiu.cristea |
Comments
Comment #1
cilefen CreditAttribution: cilefen commentedI cannot reproduce this. I added multiple file fields to content type in D7 and D8 and the issue does not occur. Does this happen when coding using the API? If so, please post the code you wrote that produces duplicate IDs.
Comment #2
Jeremy JOCHEMSKI CreditAttribution: Jeremy JOCHEMSKI commentedHello,
Thanks for the answer. The issue concernes drupal v7.37.
Do you add an id to file fields for reproducing the issue (using '#id') ?
I use webform module which put id on file fields, but it calls the core
theme function which produce the html result and duplicate the id.
It's not by coding the api but I think it concernes the core function algorithm or my understanding :
https://api.drupal.org/api/drupal/modules%21file%21file.module/function/...
On /modules/file/file.module
Line 687-690 :
It instanciate an array.
If there is an id into the elements, it puts the same id into the array.
Line 698 :
It creates a div with an id (so the same id of the file element)
Line 699 :
It creates the render of the element with the call of function drupal_attributes
so with the same element id.
So, the function theme_file_managed_file set two similar id, one into the div elem, one into the input type=["file"] elem.
I hope that my explanation is usefull.
Best regards,
Comment #3
cilefen CreditAttribution: cilefen commentedSo this is a webform module issue? Could it be a duplicate of #2059215: Replace use of IDs to classes on Webform wrappers to avoid duplicate IDs?
Comment #4
DanChadwick CreditAttribution: DanChadwick commented@cilefen - Core file fields don't use theme_file_managed_file, which is why you aren't seeing the bug. I believe if you programmatically create the managed_file field you will be able to reproduce the issue. Core field use a theme_file_widget or theme_file_widget_multiple. See modules/file/file.field.inc.
EDIT: I'm the webform maintainer. Webform isn't creating the duplicate ID, core is via FAPI and this theme function.
Comment #5
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedHello guys!
I have a problem with duplicate id on a project.
When I try to upload file I have JS-error.
http://dl1.joxi.net/drive/0005/3729/343697/150828/6a7508433a.png
Please apply my patch for your module.
Best regards.
Comment #6
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedComment #8
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedComment #10
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedSorry, I used wrong drupal version.
Comment #11
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedComment #13
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedOne more patch...
Comment #14
cilefen CreditAttribution: cilefen commentedWhat about drupal_html_id()?
Comment #15
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedGood idea!
Thank you!
I'm going push it today.
Comment #16
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedPatch with drupal_html_id()
Comment #18
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedOne more time...
Comment #19
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedHello guys!
Please check my patch.
Best regards.
Comment #20
cilefen CreditAttribution: cilefen commentedIt could probably use a test. Are we sure this isn't a problem in Drupal 8?
Comment #21
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedNo, this bug appear in my project on D7.
I also set patch version 7.x-dev.
Comment #22
cilefen CreditAttribution: cilefen commentedIssues must opened and fixed in Drupal 8 first if they affect Drupal 8 according to the backport policy.
Comment #23
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedOk, I'll check it in D8.
Comment #24
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedHello!
Yes, this bug appears in D8 core.
Comment #25
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedI've tested it in PHP 5.5 & MySQL 5.5 13,985 and everything is ok.
Please apply my patch.
Best regards.
Comment #26
cilefen CreditAttribution: cilefen commentedThese tags are not necessary.
Comment #27
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedOkay, sorry.
Comment #28
cilefen CreditAttribution: cilefen commentedThere is no absolute "right" or "wrong" tagging rule in this issue queue. In practice, useful tags are for queue management purposes, such as "Needs reroll", "needs backport to D7", or tags for sprints.
It will be worthwhile to add a regression test for this. If you are unfamiliar with tests in core I can offer guidance. To start, look in the src/Tests directory in the file module. We may be able to add to an existing test.
Comment #29
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov commentedComment #30
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedHello!
Could you provide guidance for regression tests?
I can't find it.
Thank you.
Comment #31
cilefen CreditAttribution: cilefen commentedFileFieldDisplayTest is a possible place for this. For example, at the end of FileFieldDisplayTest::testNodeDisplay(), you could perhaps add a second file field then check the raw output for duplicate IDs.
Tip: Execute this test locally on your development computer, the re-execute it as you modify it.
Comment #32
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedHello!
I implemented test for this, also I remove some unused variables and change deprecated functions to D8 methods.
And after my test we have problem with "edit-FIELD_NAME-ajax-wrapper", I can find duplicated IDs, but cannot solve it now. (I set TODO for this)
So, please check my current test and please apply my patch.
Best regards.
Comment #33
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedSorry, I've create patch via phpStorm. Sometimes it creates invalid patches :(
So, one more patch...
Comment #34
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedComment #37
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedCorrected last patch...
Comment #38
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedThis is very strange but test fails in another places...
for example http://joxi.ru/MAjGeyzueXM1re
I think this is related to new Drupal CI.
Please review my patch manually and send me feedback.
Best regards.
Comment #40
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedPlease check last comment.
Comment #44
cilefen CreditAttribution: cilefen commentedThe test failures are real, for example, ContentTranslationSyncImageTest. It is because the Html class was not imported with a use statement.
Comment #45
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedYes, I see( Sorry.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI'm not sure the theme layer is the right place to fix this? See discussion/analysis at #2594955: [D7] Duplicate HTML IDs are created for file_managed_file fields which identifies this as a deeper problem (should probably be marked as a duplicate of this one)... at least for Drupal 7 the patch there may be the right way to go. I haven't looked into Drupal 8 yet.
Note to self - whatever we do here for Drupal 7, need to test how this affects IMCE Filefield since the committed fix for the previous regression there did not match the patch posted in the issue (#2466475: Drupal core 7.36 broke IMCE for FileField Ajax upload previews) and therefore could break again if it depends on specific IDs.
Comment #47
Andrew.Mikhailov CreditAttribution: Andrew.Mikhailov at DrupalJedi commentedWhat about my patch?
Is it correct or not?
Comment #48
cilefen CreditAttribution: cilefen commented@David_Rothstein has a D7 patch in #2594955-9: [D7] Duplicate HTML IDs are created for file_managed_file fields.
Comment #49
Dragan Eror CreditAttribution: Dragan Eror at Examiner.com commentedTested patch from #44 and it works like a charm! Thanks @Andrew.Mikhailov!
Comment #50
cilefen CreditAttribution: cilefen commentedSee the standard for @todo comments.
Is changing this in the issue scope? Even if not, it could be ok in this test.
Is changing this in the issue scope? Even if not, it could be ok in this test.
Comment #52
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedComment #53
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedComment #55
cilefen CreditAttribution: cilefen commentedI renamed the D7 issue #2594955: [D7] Duplicate HTML IDs are created for file_managed_file fields because the backport policy allows it to exist. But we could think about merging these based on the reasoning in #46.
Comment #57
pfrenssenI agree that the solution from #2594955: [D7] Duplicate HTML IDs are created for file_managed_file fields looks better, it proposes a new
#label_for
property that allows to declare for which (sub) element the label is intended. This approach allows to solve this problem not only for the managed file field but for any complex field that might have a form element nested inside a wrapper.Assigning to me, I need to fix this for a D8 project today, I'll have a few hours this afternoon to work on this.
Comment #58
pfrenssenI started on this, and it appears that the
#label_for
functionality has already been implemented for D8, but the property is called#for
:This has been added in #1938926: Convert simpletest theme tables to table #type.
Comment #59
pfrenssenI marked #2594955: [D7] Duplicate HTML IDs are created for file_managed_file fields as a duplicate of this issue since the suggested approach there is already partly implemented in D8. This means we can use the same fix for both versions and we don't need two separate issues for it since we can use the normal patch workflow of fixing it first for D8 and then backporting to D7.
Here is an initial forward port of the original D7 patch from #2594955: [D7] Duplicate HTML IDs are created for file_managed_file fields. This has not yet been tested properly. No interdiff since this is a completely new approach.
Comment #60
pfrenssenAdded a test and fixed a bug that was uncovered by the test.
If this gets committed, please add credit to @predy and @David_Rothstein who wrote the original D7 patch. I only ported it to D8.
Comment #62
yogeshmpawarUpdated patch for test failures & removed one coding standard issue
Comment #64
pfrenssenIt appears there is a *second* instance of a duplicate HTML ID, the ajax wrapper that is added in
FileWidget::processMultiple
.Comment #65
pfrenssenThe second instance of duplicate HTML IDs is handled in #2346893: Duplicate AJAX wrapper around a file field.
Comment #66
pfrenssenComment #67
pfrenssenComment #68
pfrenssenI cross-posted with @Yogesh Pawar, here is a version of the patch including the fixes from #62.
Comment #71
pfrenssenComment #72
yogeshmpawarAny Update on this issue ?
Comment #73
cilefen CreditAttribution: cilefen commentedIs this a duplicate of #2346893: Duplicate AJAX wrapper around a file field?
Comment #74
cilefen CreditAttribution: cilefen commentedOh, #65.
Comment #75
joseph.olstadwe need #68 for another very important issue:
#1862892: When a theme has a managed_file field and a submit callback, call to undefined function error is thrown.
Comment #76
joseph.olstadbumping the priority because a Major requires this to be fixed.
Comment #77
star-szrI don't have a lot of deep thoughts on this one right now but one thing jumped out at me:
It's weird to me that this defaults to array, an empty string seems more appropriate.
Comment #78
star-szrAlso, the IS could use some love and this probably needs a CR because of the #label_for addition.
Comment #80
jibranRe-roll for 8.4.2
Comment #82
kim.pepperRe-roll for 8.6.x
Comment #84
claudiu.cristeaTest fix. I wasn't able to use the
AssertContentTrait
because I'm getting this error:I think the trait is only for kernel tests (?)
Comment #85
claudiu.cristeaComment #86
claudiu.cristeaForgot the patch :)
Comment #87
claudiu.cristeaComment #89
claudiu.cristeaCannot understand. Retesting.
Comment #90
jibranI have been using this patch for quite a while and it fixes the a11y issue. It seems like all the outstanding feedback has been addressed so RTBC.
Comment #91
alexpottIf there are no IDs to be found then this will pass. I'm not that that is correct. I think we should check that $seen_ids is not empty.
Comment #92
claudiu.cristea@alexpott and myself we had a chat on the remark from #91:
I also cleaned the assert method as it has been initially copied from the WTB and contains parameters that we are not using here.
Comment #93
jibranBack to RTBC as #91 has been addressed.
Comment #94
alexpottCommitted 20c250e and pushed to 8.7.x. Thanks!
As this is adding a new render property to form elements only committing to 8.7.x
Comment #95
alexpottAdding review credit.
Comment #97
jibranSorry, I don't want to sound like a broken record who is complaining about backporting all around the issue queue. I'm happy we fixed it at least in 8.7.x but here are my concerns:
Given the above info I suggest we should backport this.
As a side note, I published the change record. Once again I really appreciate that you committed this. Thank you!
Comment #98
claudiu.cristeaYes, please, it doesn't hurt an existing site in any way. Could you reconsider the decision? Thank you
Comment #99
alexpott@jibran I disagree - the problem is you never know what some one is doing in contrib and custom and they might have a form element which already has this property and adding it might break their code. Yes this is not API and is specifically not BUT it is helpful to only add this kind of change at specific times.
Comment #100
alexpottComment #102
PanchoOnly fixed in 8.7.x, so changing version.
D7 backport is being covered in #2594955: [D7] Duplicate HTML IDs are created for file_managed_file fields, so removing tag.