Problem/Motivation

Putting a multifield inside a multipage field group results with Move submit button to last multipage enabled results in the multifield's "Remove" button being moved out of the multifield and onto the last page of the form.

Steps to reproduce

  1. Install a fresh copy of Drupal 7.37 with the minimal install profile.
  2. Download multifield-7.x-1.0-alpha1 and it's dependency, ctools-7.x-1.7.
  3. Download the latest version of field_group-7.x-1.x-dev.
  4. Log in to the new site as user/1.
  5. From admin/modules, enable the Field UI, Chaos tools, Field Group, and Multifield modules.
  6. From admin/structure/types/add, add a new content type. I named mine "Test".
  7. Go to the Manage fields tab for that content type (mine was at admin/structure/types/manage/test/fields). There should already be a "Title" and "Body" field in the content type.
  8. Add a new field of type "Multifield" (I named mine "Multifield 1"). Use the default settings, but set the "Number of values" (a.k.a. "Cardinality") to "Unlimited".
  9. When the Multifield has been created, click "Manage subfields" in the "Field type" column.
  10. Add a new field of type "Text" (I named mine "Textfield a"). Use the default settings.
  11. Go to the node/add page for the new content type (mine was at node/add/test).
    Note there is a "Remove" button inside the multifield control.
  12. Return to the Manage fields tab for your content type.
  13. Add a new "multipage group" group (I named mine "Multipage group i"). After it's created, click the gear in the Operations column, and set Move submit button to last multipage to "Yes".
  14. Add two "multipage" groups (I named mine "First page" and "Second page").
  15. Move "First page" and "Second page" inside "Multipage group i", in that order (i.e.: first page is first).
  16. Move the "Body" field inside "Second page".
  17. Move the "Multifield 1" inside "First page".
  18. Go to the node/add page for the new content type (mine was at node/add/test).
    Note there the "Remove" button is missing from the multifield control.
  19. Click "Next page" to go to the next page. Note there is a "Remove" button at the bottom of the second page that doesn't seem to be connected to anything.

Analysis

This happens because:

  1. the form_process_actions() give the form-actions class to all FAPI elements of type actions,
  2. this module's multipage/multipage.js moves ALL elements that match the selector .form-actions to the last page of the multipage when Move submit button to last multipage is enabled,
  3. multifield sub-forms contain a form API element of type actions — which kind-of makes sense considering they could be displayed on a page by themselves if JavaScript is disabled

Proposed resolution

I propose:

  1. Going through the first level of the form array, searching for any form elements of type actions. If we find one, we give it the class field_group_form_actions.
  2. Changing multipage/multipage.js to only move form elements with the field_group_form_actions class.

The reasoning behind this is:

  • It seems that the idea behind the Move submit button to last multipage option is to make it hard for users to accidentally submit the whole form before it's done,
  • It's reasonably safe to assume that the first level of the form array contains items for the whole form — put another way, it's more likely that any actions elements deeper than the first level belong to a sub-form,
  • If someone has a special case where they've nested the form's actions somewhere, they can "opt-into" moving the actions to the last page by adding the field_group_form_actions class — while less automatic, it reduces the chance that multipage will accidentally affect something in more-common cases (e.g.: using multifield)

Remaining tasks

  1. Write a patch
  2. Review and RTBC
  3. Commit

User interface changes

Rather than moving all items with the class form_actions to the last page of the form, only items with the class field_group_form_actions will be moved instead.

API changes

Adds a field_group_form_actions class. Items with this class will be moved to the last page of the form.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mparker17’s picture

Assigned: mparker17 » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
1.8 KB

Added a link to the issue description, attached a patch, moved to "Needs review", unassigned.

Will add steps to reproduce shortly.

Status: Needs review » Needs work

The last submitted patch, 1: stop_moving_sub_form-2497345-1.patch, failed testing.

mparker17’s picture

Issue summary: View changes

Added steps to reproduce.

mparker17’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

Invalid patch format in stop_moving_sub_form-2497345-1.patch? That's weird. Maybe it was the mixed line endings in multipage/multipage.js? Let's try it with UNIX line endings only.

(no interdiff because I only changed the line endings of the last patchfile).

Status: Needs review » Needs work

The last submitted patch, 4: stop_moving_sub_form-2497345-4.patch, failed testing.

mparker17’s picture

Status: Needs work » Needs review
FileSize
18.58 KB

Very strange.

Let's see what happens if I change all line endings in the JavaScript file... this will make it look as if all lines have changed in the file.

(no interdiff because I only changed the line endings of the javascript file).

mparker17’s picture

FileSize
623 bytes
18.61 KB

Since elements in a form array don't necessarily have a '#type' element, I've added a check to prevent "undefined element" errors.

skylord’s picture

Works fine with Paragraphs inside pages! Thanks!

Chris Matthews’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The 4 year old patch in #7 to field_group.module does not apply to the latest field_group 7.x-1.x-dev and if still applicable needs a reroll.

skylord’s picture

Reroll for last dev.

Chris Matthews’s picture

Issue tags: -Needs reroll