The OpenID module does some JS+CSS tricks in order to hide the OpenID text field until the user clicks the “Log in using OpenID” link. This no longer works - now the OpenID text field is always visible in the login block.

I think this regression was introduced in #482636: Remove "edit-[name]-wrapper" ID from form element wrappers.

Files: 
CommentFileSizeAuthor
#13 drupal.form-item-classes.13.patch8.84 KBsun
Passed: 12422 passes, 0 fails, 0 exceptions
[ View ]
#11 drupal.form-item-classes.10.patch8.84 KBsun
Passed: 12418 passes, 0 fails, 0 exceptions
[ View ]
#9 drupal.form-item-classes.9.patch8.18 KBsun
Passed: 12415 passes, 0 fails, 0 exceptions
[ View ]
#8 drupal.form-item-classes.8.patch8.14 KBsun
Passed: 12435 passes, 0 fails, 0 exceptions
[ View ]
#5 drupal.form-item-classes.patch6.07 KBsun
Failed: Failed to apply patch.
[ View ]
#2 form-item-1.patch7.18 KBc960657
Failed: Failed to apply patch.
[ View ]
#1 openid-css-2.patch2.47 KBc960657
Failed: 12041 passes, 3 fails, 0 exceptions
[ View ]
openid-css-1.patch2.35 KBc960657
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openid-css-1.patch.
[ View ]

Comments

StatusFileSize
new2.47 KB
Failed: 12041 passes, 3 fails, 0 exceptions
[ View ]

Missed one.

Title:OpenID login JS+CSS brokenCSS+JS regressions related to form-item-[name]
Component:openid.module» forms system
StatusFileSize
new7.18 KB
Failed: Failed to apply patch.
[ View ]

The other CSS and JS changes in #482636: Remove "edit-[name]-wrapper" ID from form element wrappers have the same problem.

I am not sure that the regressions were introduced by the fix for that issue, but it looks like they appeared about the same time.

Hm. Currently we do:

<?php
 
// Add element's #type and #name as class to aid with JS/CSS selectors.
 
$class = array('form-item');
  if (!empty(
$element['#type'])) {
   
$class[] = 'form-item-' . strtr($element['#type'], array('_' => '-'));
  }
  if (!empty(
$element['#name'])) {
   
$class[] = strtr($element['#name'], array(' ' => '-', '_' => '-', '[' => '-', ']' => '')) . '-wrapper';
  }
?>

Which means:

1) You are right. :)

2) If there is a form element with #name = 'page', then that will turn into .page-wrapper. A theme might already use this class for something else. :(

3) We want to add a prefix to the '[#name]-wrapper' class. But I'm not sure, what would make sense here.

Priority:Normal» Critical

Bumping priority.

StatusFileSize
new6.07 KB
Failed: Failed to apply patch.
[ View ]

So this is my recommendation:

  • form-type-[#type] for the form item's type (makes sense, eh?)
  • form-item-[#name] for the form item's internal name; this advances on the existing, generic "form-item" class, so ++consistency.

The "-wrapper" suffix makes no sense to me. A wrapper is an additional wrapping container, usually injected by custom form builder or theme functions for advanced styling.

Assigned:Unassigned» sun

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.14 KB
Passed: 12435 passes, 0 fails, 0 exceptions
[ View ]

This patch just needs quick test. To do so, apply the patch and either clear Drupal's cache or re-install HEAD. Then approve the following:

- Install Book module, go to node/add/book, choose "create a new book", and verify that the form item description does not flip around.

- Go to admin/reports/dblog, open the "Filter log messages" fieldset, and verify that form items are aligned next to each other.

- Install OpenID module, logout, and verify that toggling OpenID login form works.

- Go to admin/structure/types/add and verify that the machine readable name behavior works.

- Go to admin/structure/taxonomy/add and verify that the machine readable name behavior works.

StatusFileSize
new8.18 KB
Passed: 12415 passes, 0 fails, 0 exceptions
[ View ]

Damn - wrong patch.

Status:Needs review» Needs work

I spent 60 minutes familiarizing myself with these issues. OpenID's show/hide functionality is indeed broken because the form has a "openid-identifier-wrapper" class, but the CSS file is trying to match a "form-item-openid-identifier-wrapper" class.

I see the sense in a "-wrapper" ending, however its much more important to prevent bleeding of a theme's styles (as Sun points out in #3). And using "form-item-" and "-wrapper" is just way too long, so just using the "form-item-" prefix is preferable.

Doing a global search for "-wrapper" it looks like Sun caught almost everything. Except in the system.js, the "update-status-module-2-wrapper" needs to be renamed "form-item-update-status-module-2" otherwise the installer's "update notifications" js is broken.

Everything else about the patch is RTBC. Will wait for a re-roll. :-)

Status:Needs work» Needs review
StatusFileSize
new8.84 KB
Passed: 12418 passes, 0 fails, 0 exceptions
[ View ]

Thank you! :)

Status:Needs review» Reviewed & tested by the community

Ok. The new patch in #11 fixes the issue I mentioned.

StatusFileSize
new8.84 KB
Passed: 12422 passes, 0 fails, 0 exceptions
[ View ]

Just in case... re-rolled for the registry removal

Status:Reviewed & tested by the community» Needs review

Boy, that is some gnarly-assed code in theme_form_element(). :P However, that was there before this patch...

I couldn't quite grok what was happening here so I checked with sun on IRC. The summary is that basically -wrapper is superfluous (and also a misnomer since we aren't 'wrapping' anything; these are classes directly on the elements), and we can settle for just form-item-NAME and form-item-TYPE to get us the selectors we're looking for. It also removes a bit of leftover code from the teaser splitter removal which was also using these wrapper classes.

This sounds sane to me, but I'd love to get at least one more themer to chime in here.

Status:Needs review» Reviewed & tested by the community

This looks good to me. Thank you for working on this. One more theme function I wont have to override! ;)

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs Documentation

Ok, committed to HEAD.

This will need to be reflected in the upgrade docs. Please spell it out too, as I'm not sure it's entirely clear how to find these class names.

The summary is that basically -wrapper is superfluous (and also a misnomer since we aren't 'wrapping' anything; these are classes directly on the elements), and we can settle for just form-item-NAME and form-item-TYPE to get us the selectors we're looking for. It also removes a bit of leftover code from the teaser splitter removal which was also using these wrapper classes.

To clarify in advance:

It's not .form-item-NAME and .form-item-TYPE. The patch may have lead to this confusion, because it fixes a CSS class of dblog.module accordingly.

It is:

.form-type-TYPE

and

.form-item-NAME

Considering the (PHP) form structure:

<?php
$form
['body'] = array(
 
'#type' => 'textarea',
  ...
);
?>

then the resulting markup will be:

<div class="form-item form-type-textarea form-item-body">
...
<textarea id="edit-body" name="body" ...></textarea>
...
</div>

That means:

- #type becomes .form-type-textarea

- #name (resp. the internal array key used) becomes .form-item-body

Both classes are applied on the wrapping div.form-item.

The inner markup for the form element is not touched at all.

Priority:Critical» Normal

Status:Needs work» Needs review

openid-css-1.patch queued for re-testing.

Assigned:sun» Unassigned

changing tag - looks like this is upgrade doc that is needed

Status:Needs review» Fixed

I added information about this to both the theme and module update guides:
http://drupal.org/update/themes/6/7#css-no-wrappers
http://drupal.org/update/modules/6/7#css-no-wrappers
Comments welcome - if something is wrong, please either edit or set the status back to "needs work".

Status:Fixed» Closed (fixed)
Issue tags:-regression, -Needs Update Documentation

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