The HTML5 field_group formatter adds the same id attribute to every field. This is especially a problem when multiple nodes appear on the same page, e.g. a view of teaser. This generates invalid HMTL, since id attributes must be unique per page. See example screenshot.

screenshot-fieldgroup-duplicate-html5-ids.png

The attached patch removes the id attribute altogether so the output is valid HTML.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

maximpodorov’s picture

The patch is rerolled for the current module version.

maximpodorov’s picture

The patch is rerolled for the current module version.

Status: Needs review » Needs work

The last submitted patch, field_group-fix-html5-id-2037731-1.patch, failed testing.

maximpodorov’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review

Switching to 7.x-1.x since it's the only active branch.

maximpodorov’s picture

nils.destoop’s picture

The id's have been removed in the default behavior. However, if people preprocess an id on the group (because they really want one). The id will still be shown.

nils.destoop’s picture

Issue summary: View changes
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

rreiss’s picture

Status: Closed (fixed) » Needs review
FileSize
831 bytes

Hi.
The current "fix" on the dev version was to completely remove the field group's id attribute, I don't think this is the right decision.
What about upgrading from V1.3 to a newer version? CSS styles may be broken for those who used the IDs.
The Drupal way is to use drupal_html_id to prevent those issues.

I'm attaching my own patch (according to the code from 7.x-1.3).

My patch will only break in the case when someone used the ID selector to style multiple field group with the same ID, instead of using the class attribute. (Really bad practice!)

Please consider to change it before releasing a new version.

pelegr’s picture

I agree, it does seem a bit odd to me to completely remove
an ID attribute which may currently be in use by thousands
of users who may have behaviors which depend on this ID -
for the sake of html validation.

Perhaps the better option would be to merely
prevent duplicates as the patch above does.

nils.destoop’s picture

The id attribute was not removed completely. A setting has been added to allow people entering an id if they want.
However, you are correct. I shall write an update hook so current id's are default stored in the setting. I wil also add your drupal_html_id() call.

nils.destoop’s picture

Status: Needs review » Fixed

An update hook was written. People that install the latest version will default get the old ids. They can remove them again in the UI.

Status: Fixed » Closed (fixed)

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

  • Commit aa8454c on 7.x-1.x, 8.x-1.x authored by Zach Harkey, committed by zuuperman:
    Issue #2037731 by maximpodorov, Zach Harkey: Remove id attribute from...
  • Commit dc47719 on 7.x-1.x, 8.x-1.x by zuuperman:
    Issue #2037731 by rreiss, maximpodorov, Zach Harkey: Make id attribute...

Status: Closed (fixed) » Needs work

The last submitted patch, 9: div_duplicate_ids.patch, failed testing.

nils.destoop’s picture

Status: Needs work » Closed (fixed)
maximpodorov’s picture

Stupid robot.

milovan’s picture

Status: Closed (fixed) » Needs work

I beleive this issue made a problem with fieldset. On a configuration of fieldset group, on Manage fields page , there isn't a text field to enter an id.
Also, on node edit / add, there isn't any id shown in fieldset. In 1.3, there was an id attribute for fieldset. In this issue you added an update hook (7007) to migrate ids to the new id configuration. While it might work fot html, this doesn't work for fieldsets and its ids, as it didn't migrate them, which results in code not rendering ids for fieldsets in configuration, node add and edit, and node view.

Nemanja’s picture

This doesn't make sense to me because if we put custom ID or leave it as default it will repeat itself everywhere no metter if it's default or not... The only option is to remove ID completely but that may break people's themes so i don't think that this is good solution that you provided.

eldrupalista’s picture

This is breaking an important feature of the project I work on. I rely on on the id attribute of the groups for creating an ajax return wrapper. This commit breaks it.
I have to use 7.3 version because of this.

Anybody’s picture

I can confirm that there is a further problem:

The horizontal-tabs.js relies on the "id" attribute and on form pages (widets configuration) the ID is now completely missing so that the tab to open can no more be addressed by its ID:

Drupal.theme.prototype.horizontalTab = function (settings) {
  var tab = {};
  var idAttr = settings.fieldset.attr('id');

  tab.item = $('<li class="horizontal-tab-button" tabindex="-1"></li>')
    .append(tab.link = $('<a href="#' + idAttr + '"></a>')
    .append(tab.title = $('<strong></strong>').text(settings.title))
    );

  // No need to add summary on frontend.
  if (settings.fieldset.drupalGetSummary) {
    tab.link.append(tab.summary = $('<span class="summary"></span>'))
    }

  return tab;
};

So I see two possible solutions:
1. Use something different from the ID to address the anchor in the JavaScript above
2. Allow to set an ID optionally in the WIDGET options (in DISPLAY options this is already possible)

What do you think?

sidharthap’s picture

Hi,

Thanks for the cool module. recently i upgraded from 1.3 to 1.5. My all updates run correctly.(Field_group 7007 and 7008)
My custom code has JS using the id like (user_user_form_group_first_step) but my page shows user-user-form-group-first-step due to which my multi step form not working.

As per the comment https://www.drupal.org/node/2037731#comment-8574291 It should work correctly. I tried removing the id name from filed setting which results no id being displayed on front end and not worked.

As per this https://www.drupal.org/node/2283245 removing "drupal_html_id" from code does the magic. Please add the code back from 1.3 version to maintain the ID in proper format and may be some other solution to track the duplicate.

if (isset($group->format_settings['instance_settings']['id']) && !empty($group->format_settings['instance_settings']['id'])) {
$element['#id'] = $group->format_settings['instance_settings']['id'];
}
else {
$element['#id'] = $form['#entity_type'] . '_' . $form['#bundle'] . '_' . $view_mode . '_' . $group->group_name;
}

  • zuuperman committed aa8454c on 8.x-3.x authored by Zach Harkey
    Issue #2037731 by maximpodorov, Zach Harkey: Remove id attribute from...
  • zuuperman committed dc47719 on 8.x-3.x
    Issue #2037731 by rreiss, maximpodorov, Zach Harkey: Make id attribute...