Problem/Motivation

FormBuilder::prepareForm() adds a form_build_id element, which is a hidden input, for example:

<input autocomplete="off" data-drupal-selector="form-v8temxqitapn2hablkw4gfmqyls0f5pmiy9tcd3p5gy" type="hidden" name="form_build_id" value="form-V8tEmXQITAPn2hAblkW4gfMQYls0F5pmiy9tCd3P5GY" />

This is invalid HTML, as confirmed by the W3 HTML Validator:
An input element with a type attribute whose value is hidden must not have an autocomplete attribute whose value is on or off.

This error gets flagged by automated accessibility testing software, such as SortSite.
Bad value for attribute autocomplete.
<input autocomplete="off" data-drupal-selector="form-v8temxqitapn2hablkw4gfmqyls0f5pmiy9tcd3p5gy" type="hidden" name="form_build_id" value="form-V8tEmXQITAPn2hAblkW4gfMQYls0F5pmiy9tCd3P5GY" />

Looking back at the git history, autocomplete=off was added back in 2018 by #2596597: [regression] Soft reload does not clean up user inputs like in D7 (Firefox only) to fix a bug with Firefox browsers.

I have tested removing autocomplete=off and tried to reproduce the bug described in the original ticket and have been unable to reproduce the error with this fix removed. This leads me to think that either the Firefox browser has fixed the underlying issue since then, or something else within core has changed which solves it a different way. Either way, I think it's safe to remove.

Steps to reproduce

  1. Log in and go to a node add form
  2. View source and look for the hidden input element with name="form_build_id". It currently has autocomplete=off but this should be removed.

Proposed resolution

Remove autocomplete=off from FormBuilder::prepareForm()

CommentFileSizeAuthor
#18 3320467-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3320467

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nicrodgers created an issue. See original summary.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers as a volunteer commented
Issue summary: View changes

nicrodgers opened merge request !2941

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers at Welsh Government commented
Assigned: nicrodgers » Unassigned
Status: Active » Needs review
jonnyhocks’s picture

jonnyhocks CreditAttribution: jonnyhocks at Welsh Government commented
Status: Needs review » Reviewed & tested by the community

Reviewed the MR and tested (also with SortSite).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

We have to be very sure that this is fixed in Firefox - it might have been fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1752250 but https://bugzilla.mozilla.org/show_bug.cgi?id=520561 is still open.

I think we should have some test HTML from one of those issues where we can prove that this no longer happens. It would be great if @nicrodgers could detail exactly what steps were taken to prove this has been fixed in Firefox.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers at Welsh Government commented
Status: Needs work » Needs review

To test this in Firefox, I followed the steps to replicate from the linked issue:

Steps to reproduce:

  • Add an new field to a node entity e.g. "Text (formated, long) with cardinality=unlimited."
  • Navigate to node-edit of a node entity with the new field.
  • Click "Add another item".
  • Execute a soft reload (ctr+r / cmd+r).
  • Click "Add another item" after the page has been sotfly reloaded.
  • Now watch how instead of one new item there will be two new items generated.

Actual result:
* Only one new item was there, which is correct. Tested in Firefox 106.0.3 (64-bit) on Linux.

I'll set it back to Needs Review and see if we can get some more people to test it on a range of different Firefox versions and platforms.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative, +Needs tests

If this is a bug then we will need a test to make sure it doesn't come back.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

guptahemant’s picture

guptahemant CreditAttribution: guptahemant as a volunteer and at QED42 for Salsa Digital commented

I also encountered the mentioned invalid HTML issue in on one of my projects and when i tried the patch from #3, it caused the original problem (https://www.drupal.org/project/drupal/issues/2596597) to appear back on Firefox browser.

Note: On my project i tested the behaviour with an entity reference field with cardinality set as unlimited.

Device and browser information
Firefox version: 108.0.1 (64-bit)
System details: MacBook Pro with OS version 12.0.1

Hence i think we can not commit this patch until the problem is resolved at Firefox browser itself.
Previously reported issues have been closed at firefox side, hence added a new one, https://bugzilla.mozilla.org/show_bug.cgi?id=1808843

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

The last issue is closed as well, so instead of test we can revert old commit

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests

We don't have a FF test suite so this needs to be covered by manual tests.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Following the steps in #7 I got the same result with 2 new items being generated.

And this was with MR 2941 applied.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

And today Firefox 117 is out (fixed in 108)

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Liam Morland made their first commit to this issue’s fork.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at OpenPlus commented
Issue summary: View changes
Status: Needs work » Needs review

I have updated the issue fork. The merge request needs to be set to target branch 11.x. I believe only the owner of the merge request, @Nic Rodgers, is able to do this.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
FileSize
3320467-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers at Welsh Government commented
Status: Needs work » Needs review

Thanks @Liam. I've changed the target branch to 11.x
Setting back to Needs Review. We need people to test with latest firefox versions (see #14)

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Seems to be an odd failure in cspell. May need a rebase.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at OpenPlus commented
Status: Needs work » Needs review

Rebased. Spelling test now passes.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Tested in firefox with the MR applied and not noticing form_build_id appear

  • catch committed 71564bb2 on 10.3.x 
    Issue #3320467 by Liam Morland, nicrodgers, smustgrave, andypost,...

  • catch committed 720c29fa on 11.x 
    Issue #3320467 by Liam Morland, nicrodgers, smustgrave, andypost,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.