Problem/Motivation
Seems to be some issue with the form cache interaction with the media library widget, and possibly other complex widgets. Not sure where the issue is exactly though. The problem is that after clicking the "remove" button in the media library widget to remove an already-selected item, the form builder doesn't know that the remove button was clicked, and it instead assumes the first button in the form was clicked, which could be anything. On a simple node form with just a media library widget, the button it clicks is the "add media" button so the media library selection modal opens up incorrectly.
Steps to reproduce
- Use Firefox
- Clean install of Drupal with the Media Library module enabled
- Create a content type with a media reference field with all default settings
- Create a node of that type, select a media item (upload one), and save the node
- Edit the node and click the "remove" button in the media library widget. Observe the correct behavior of the item being removed and the widget reverting to its original empty state
- Don't save the node, reload the form in your browser
- Again, click the "remove" button in the media library widget. Observe that instead of removing the item and reverting the widget to its original state, the media library selection modal opens up incorrectly.
Here's a video demonstrating the steps above. I started with a fresh form load (no form cache set in key value).
If you have a more complex node form, like one with a paragraphs field on it, what might end up happening instead of the media library modal opening is it that the entire media library widget gets replaced with a paragraph form for a paragraph item!
The wrong behavior is because of this code in FormBuilder:
// If a form contains a single textfield, and the ENTER key is pressed
// within it, Internet Explorer submits the form with no POST data
// identifying any submit button. Other browsers submit POST data as
// though the user clicked the first button. Therefore, to be as
// consistent as we can be across browsers, if no 'triggering_element' has
// been identified yet, default it to the first button.
$buttons = $form_state->getButtons();
if (!$form_state->isProgrammed() && !$form_state->getTriggeringElement() && !empty($buttons)) {
$form_state->setTriggeringElement($buttons[0]);
}
The wrong element is set as the triggering element for the form submission. Why? Because the form object that's being acted on is used a cached version of the form (cached after step #4 above) which is missing the "remove" button for the media library widget. When the form is being processed, and input elements in the form are checked to see if they were pressed and were the triggering element, the "remove" button doesn't exist in the form so it never sets itself as the triggering element, so it falls back to that logic pasted in above which just picks the first button.
Proposed resolution
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3495881
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:
- 3495881-form-cache-causes
changes, plain diff MR !10706
Comments
Comment #2
bkosborneComment #3
bkosborneComment #4
godotislate@bkosborne What browser did you test in?
For me testing locally on latest 11.x dev:
On Windows, Chrome 131.0.6778.205, when I reload, the media selection is restored on the page. When I click the remove button again, the media item is correctly removed.
Also on Windows, Firefox 133.0.3, I can reproduce the issue mentioned in the IS. However, if I revisit the page (while on the page, click in location bar and hit enter) instead of reloading, then the remove button works as expected.
It seems to be related to Firefox keeping form data on reload: https://stackoverflow.com/questions/7377301/firefox-keeps-form-data-on-r.... It's an old issue on stackoverflow, but I can confirm that if I add the
autocomplete="off"attribute to the node form, e.g.$form['#attributes']['autocomplete'] = 'off';, then reloading on Firefox works the same as Chrome.ETA. If the issue is indeed this FF thing, then probably the thing to look into is whether a specific form input in the media widget can be set as
autocomplete="off"that will prevent the browser from persisting the offending form data.Comment #6
godotislateFWIW, making sure the form_build_id hidden input value does not get retained by the browser was enough to fix the issue for me on Firefox. I put up a draft MR 10706 with that change.
Comment #7
bkosborneIt didn't occur to me to test in Chrome, thanks for that! I guess due the nature of the issue I didn't think it would be browser specific. Thanks for doing that and the MR! I'll test this next week and report back...
Comment #8
godotislateComment #9
kushagra.goyal commented@bkosborne I tested this issue on my local environment, and I can confirm that the behavior is browser-specific and occurs only in Mozilla Firefox (133.0.3).
In Firefox, the issue described in the IS is reproducible. Specifically, after removing the media and reloading the page, the previously selected media reappears. Interestingly, if the page is manually reloaded by revisiting the URL (clicking the location bar and pressing Enter), the "remove" button works as expected.
This behavior seems to be linked to Firefox's form data persistence on reload, as outlined in the StackOverflow thread: Firefox Keeps Form Data on Reload. Adding autocomplete="off" to the node form resolves the issue on Firefox and ensures consistent behavior across browsers.
I’ve reviewed the proposed changes in the MR,. This MR looks good to move forward. Let me know if additional testing is needed!
Comment #10
bkosborneOkay, confirmed that setting form_build_id autocomplete to off resolves the issue for me!
I don't know how a test can be created for this, given it only affects firefox
Comment #11
bkosborneComment #12
catchWhile we can't add firefox testing yet, should we add an assertion in an existing test somewhere that the form build ID has the attribute? Maybe with a @todo pointing to #3462680: Add ability to test on Firefox using selenium?
Comment #13
godotislateComment #14
godotislateAdded the test and comment per #12.
Comment #15
catchThat looks great.
Comment #20
catchSince this was previously RTBC before the test coverage was added, going to go ahead and commit.
Committed/pushed to 11.x, cherry-picked to 11.1.x, 10.5.x, and 10.4.x, thanks!