Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#13 | drupal.form-item-classes.13.patch | 8.84 KB | sun |
#11 | drupal.form-item-classes.10.patch | 8.84 KB | sun |
#9 | drupal.form-item-classes.9.patch | 8.18 KB | sun |
#8 | drupal.form-item-classes.8.patch | 8.14 KB | sun |
#5 | drupal.form-item-classes.patch | 6.07 KB | sun |
Comments
Comment #1
c960657 CreditAttribution: c960657 commentedMissed one.
Comment #2
c960657 CreditAttribution: c960657 commentedThe 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.
Comment #3
sunHm. Currently we do:
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.
Comment #4
sunBumping priority.
Comment #5
sunSo this is my recommendation:
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.
Comment #6
sunComment #8
sunThis 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.
Comment #9
sunDamn - wrong patch.
Comment #10
JohnAlbinI 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. :-)
Comment #11
sunThank you! :)
Comment #12
JohnAlbinOk. The new patch in #11 fixes the issue I mentioned.
Comment #13
sunJust in case... re-rolled for the registry removal
Comment #14
webchickBoy, 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.
Comment #15
JacineThis looks good to me. Thank you for working on this. One more theme function I wont have to override! ;)
Comment #16
webchickOk, 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.
Comment #17
sunTo 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:
then the resulting markup will be:
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.
Comment #18
catch#653068: Update documentation is incomplete
Comment #20
asiby CreditAttribution: asiby commentedopenid-css-1.patch queued for re-testing.
Comment #21
sunComment #22
jhodgdonchanging tag - looks like this is upgrade doc that is needed
Comment #23
jhodgdonI 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".