Posted by Everett Zufelt on August 21, 2012 at 11:01am
12 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | larowlan |
| Status: | needs work |
| Issue tags: | #pnx-sprint, accessibility, needs backport to D7 |
Issue Summary
Looks like the vertical tabs header (invisible) and container are rendered, even when there are no children. http://api.drupal.org/api/drupal/includes!form.inc/function/theme_vertical_tabs/7
I'm not sure if the container is for some reason required for visual consistency (I doubt it) but the header is misleading to screen-reader users.
Comments
#1
Simple fix
#2
I looked at this myself, #children isn't empty here. I applied the patch, drush cc all, then hitting the node/add/test page as anon had the vertical tabs.
See http://api.drupal.org/api/drupal/core!includes!form.inc/function/form_process_vertical_tabs/8
Perhaps we can trust that if #default_tab is not set that there are no tabs?
#3
Thanks for providing the test case Everett
#4
Patch attached
#5
#6
This one adds tests
#7
#4: vtabs-empty-1742438-4.patch queued for re-testing.
#8
The last submitted patch, vtabs-empty-1742438-6-pass.patch, failed testing.
#9
#6: vtabs-empty-1742438-6-pass.patch queued for re-testing.
#10
The last submitted patch, vtabs-empty-1742438-6-pass.patch, failed testing.
#11
Tests are failing because book doesn't set #access on it's form elements, updating logic.
New patches to follow.
#12
#13
Looking over the patch, looks like it's a useful addition.
I do wonder if there's another place to check if there's visual elements in the element_children array so that we don't have to run through the foreach check at the end. It's unlikely to have a performance hit.
Is there a place in core where it is easy to test that that vertical tabs are no longer present after this patch?
#14
Hi Mike
Testing the patch:
Additional testing:
Book is a special case as it does not set ['#access'] on it's tab - same story with translation tab.
Patch includes tests that use xpath to automate this checking.
#15
Patch applies nicely. After following the steps in #14 verifies that the header isn't there if there are no elements.
I grabbed some screenshots, but there is no change. HTML change is:
<h2 class="element-invisible">Vertical Tabs</h2><div class="vertical-tabs-panes"><input class="vertical-tabs-active-tab" type="hidden" name="additional_settings__active_tab" value="" /></div><div class="form-actions form-wrapper" id="edit-actions"><input type="submit" id="edit-submit" name="op" value="Save" class="form-submit" /><input type="submit" id="edit-preview" name="op" value="Preview" class="form-submit" /></div></div></form> </div>
</div>
<div class="form-actions form-wrapper" id="edit-actions"><input type="submit" id="edit-submit" name="op" value="Save" class="form-submit" /><input type="submit" id="edit-preview" name="op" value="Preview" class="form-submit" /></div></div></form> </div>#16
+++ b/core/includes/form.inc@@ -3796,11 +3796,26 @@ function form_process_vertical_tabs($element, &$form_state) {
+ $name = implode('__', $element['#parents']);
Unless I'm mistaken, this $name variable isn't ever used anywhere? So can we remove this (rather scary looking) line of code?
+++ b/core/includes/form.inc@@ -3796,11 +3796,26 @@ function form_process_vertical_tabs($element, &$form_state) {
+ foreach (element_children($element['group']) as $tab_index) {
+ if (!isset($element['group'][$tab_index]['#access']) ||
+ !empty($element['group'][$tab_index]['#access'])) {
+ $visible_tab = TRUE;
+ break;
+ }
+ }
This actually looks like a nice thing to split into a general helper function in case there are other places we need to do such a check. Can be a separate issue/patch.
21 days to next Drupal core point release.
+++ b/core/modules/system/lib/Drupal/system/Tests/Form/ElementsVerticalTabsTest.php@@ -40,4 +48,21 @@ class ElementsVerticalTabsTest extends WebTestBase {
+ * Ensures that the vertical-tabs wrapper markup is not shown if the user does
+ * not have access to any of the tabs.
(nitpick) Can we shorten this so it fits into 80 characters and one line?
#17
Re-roll of #12 with changes applied from #16
Kim
#18
#19
@webchick asked for a general helper function to be made out of foreach (element_children($element['group']) as $tab_index) but other than that the issues have been addressed so setting it back to RTBC.
@larowlan can you set up a new issue for that general helper function so that idea doesn't get lost?
#20
@mgifford: added #1800434: Create helper function to determine if a user has access to any tabs in a vertical tab element as follow up.
#21
Yeah, except for the 80 chars thing. I manually shortened that comment to:
/*** Ensures that vertical tab markup is not shown if user has no tab access.
*/
Let's make sure the D7 version has that text too, ok?
Committed and pushed to 8.x. Marking to 7.x for backport.
#22
Backported #17 to D7.
#23
That patch works as it did in D8. Only problem that I can see is that the tests seem to be dropped. Don't we need those in D7 too?
#24
Sorry, I didn't mean to keep that at RTBC. I can contribute a screenshot however.
#25
@mgifford, I don't understand. The tests were added to form.test in #22.
#26
Woops.. Must have been comparing with an earlier patch.
Marking it RTBC.
Edit: I should add that there is no markup change as such. The vertical tabs just don't appear because they aren't there.
#27
Thanks for the backport!
Committed and pushed to 7.x. (and manually fixed that comment again ;))
#28
Thanks.. Should have looked for that.
#29
Yeah, I'm sorry about forgetting to fix that, webchick.
#30
Can we get a change notice on this? It breaks forms that don't set #access.
Thanks.
#31
In that case, reverted this commit from 7.x while we figure this out. Also moving back to 8.x.
Mike, can you give any more details? Preferably in some form that we could translate to a test case.
#32
We recently added vertical tab support to WordPress Migrate (#1797148: Vertical tab-ify the import page), and after pulling the latest core the settings section of the form disappeared completely (#1814524: Vertical tabs broken with post-7.15 drupal) - nothing in between Blog Password and Import WordPress Blog. I briefly restored the vertical tabs by adding #access in one place, but then added it elsewhere and everything disappeared again, still figuring out exactly what it needs...
Here's a bit of the original code, which does not render:
<?php
// Import settings vertical tabs.
$form['settings'] = array(
'#type' => 'vertical_tabs',
'#title' => t('Import settings'),
'#collapsible' => TRUE,
'#collapsed' => FALSE,
'#pre_render' => array('vertical_tabs_form_pre_render'),
'#attached' => array(
'js' => array(
'vertical-tabs' => drupal_get_path('module', 'wordpress_migrate') . '/wordpress_migrate.js',
),
),
);
...
$form['settings']['wordpress_migrate_content_mapping'] = array(
'#type' => 'fieldset',
'#title' => t('Map content'),
);
$form['settings']['wordpress_migrate_content_mapping']['wordpress_migrate_page_type'] = array(
'#type' => 'select',
'#title' => t('Convert WordPress pages to'),
'#default_value' => variable_get('wordpress_migrate_page_type', $default),
'#options' => $options,
'#description' => t(''),
'#group' => 'settings',
);
...
// Select default text format for bodies etc.
$form['settings']['wordpress_migrate_text_formats'] = array(
'#type' => 'fieldset',
'#title' => t('Text formats'),
);
$options = array();
foreach (filter_formats() as $format_id => $format) {
$options[$format_id] = $format->name;
}
$form['settings']['wordpress_migrate_text_formats']['wordpress_migrate_text_format'] = array(
'#type' => 'select',
'#title' => t('Default format for text fields'),
'#default_value' => variable_get('wordpress_migrate_text_format', 'filtered_html'),
'#options' => $options,
'#description' => t(''),
);
?>
Full code at http://drupalcode.org/project/wordpress_migrate.git/blob/refs/heads/7.x-....
Thanks.
#33
So, in test case terms, _form_test_vertical_tabs_form should try adding a tab without #access, let me give that a try...
#34
Well, #access I think was a red herring. I have confirmed that our vertical tabs work with the old theme_vertical_tabs() and break with the new one. There was actually a bug in our code - it only set #group on the first element within the first fieldset (tab), it should have been setting it on the fieldsets (and only the fieldsets). But, that doesn't fix it - if I set #group on all the fieldsets, none of them is rendered. However... If I set #group on one or more (but not all) of the fieldsets, all those which do NOT have #group set do render. So, I think this patch is correct in and of itself, but it's exposing another problem - previously all the children were automatically tabified, whether or not #group was set, now it's insisting on seeing at least one valid $element['group']. Could it be that the population of ['group'] is backwards? I'm trying to work out the logic among ['group']/['#group']/['#groups']... Hmm, but first let me dig in and see why, say, the node edit form vertical tabs do work...
#35
<?php// Import settings vertical tabs.
$form['settings'] = array(
'#type' => 'vertical_tabs',
...
);
$form['settings']['wordpress_migrate_content_mapping'] = array(
'#type' => 'fieldset',
'#group' => 'settings',
...
?>
I've worked out what the buggy behavior is, although not how to fix it. In the above snippet, removing ['settings'] from the fieldset definition makes it work. So, let's take a step back and look at the documentation:
The actual behavior with this patch is:
Next step is probably writing tests to fail in the latter two cases.
#36
Here's a D7 version of the original patch plus tests designed to break in the broken scenarios...
#37
The last submitted patch, vtabs-empty-1742438-36.patch, failed testing.
#38
This snippet from form_pre_render_fieldset() is highly suspicious:
<?php// Otherwise, this element belongs to a group and the group exists, so we do
// not render it.
elseif (element_children($element['#groups'][$group])) {
$element['#printed'] = TRUE;
}
?>
So, why not render it? Is this assuming that some other piece of code (i.e., the old theme_vertical_tabs()) is rendering it?
Removing the whole chunk of code around this does allow the "Child with #group found" test to pass, and my wordpress_migrate form with the ['settings'] intact and #group specified does work. This leaves the children with no #group to account for... I've confirmed that children with no #group are not in ['group'] in theme_vertical_tabs, so the issue here is populating ['group']. Perhaps the proper logic is that a fieldset whose parent is a vertical_tabs element and has no explicit #group should default to its parent as a #group. Looking at form_process_fieldset(), which only populates ['groups'] in the presence of #group... But that's it for me for today (and probably tomorrow)...
#39
OK, I believe I've got it - simply had theme_vertical_tabs() check its direct children in addition to the contents of ['group']. Let's see how the testbot likes it.
Before rolling it for D8 - now we're doing that #access check twice within the function, is that a sufficient threshold to break it out to a helper function as part of this patch?
#40
The last submitted patch, vtabs-empty-1742438-39.patch, failed testing.
#41
Well, the Form API tests were happy with removing that chunk from form_pre_render_fieldset(), but a few specific forms out there were not. Restoring it breaks the new "Child with #group found" test, it seems like being a child AND setting #group cancel each other out. I've played around with it some more and don't have a complete solution yet, maybe someone with more experience with the Form API internals can take a crack at it.