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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c960657’s picture

FileSize
2.47 KB

Missed one.

c960657’s picture

Title: OpenID login JS+CSS broken » CSS+JS regressions related to form-item-[name]
Component: openid.module » forms system
FileSize
7.18 KB

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.

sun’s picture

Hm. Currently we do:

  // 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.

sun’s picture

Priority: Normal » Critical

Bumping priority.

sun’s picture

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.

sun’s picture

Assigned: Unassigned » sun

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

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.

sun’s picture

Damn - wrong patch.

JohnAlbin’s picture

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. :-)

sun’s picture

Status: Needs work » Needs review
FileSize
8.84 KB

Thank you! :)

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

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

webchick’s picture

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.

Jacine’s picture

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! ;)

webchick’s picture

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.

sun’s picture

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:

$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.

catch’s picture

Priority: Critical » Normal
asiby’s picture

Status: Needs work » Needs review

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

sun’s picture

Assigned: sun » Unassigned
jhodgdon’s picture

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

jhodgdon’s picture

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 documentation updates

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