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.Button elements that should be hidden with the js.hidden CSS class are not being hidden in the off-canvas dialog. In particular the image field's 'Upload' button.
Here's how we found it:
1. Created a custom block type with an image field.
2. Set a content type to use Layout Builder.
3. Used the Layout Builder to 'Add Custom Block' and chose the custom block type.
4. The image field shows an Upload button that should be hidden.
The problem is a CSS rule in core/misc/dialog/off-canvas.reset.css on line 248. Core correctly applies a class (js-hide) to set display: none, but the drupal-off-canvas css overrides it so it shows up.
from js.module.css:10
.js .js-hide {
display: none;
}
from off-canvas-reset.css:244
#drupal-off-canvas button,
#drupal-off-canvas input[type="reset"],
#drupal-off-canvas input[type="submit"],
#drupal-off-canvas input[type="button"] {
display: inline-block;
overflow: visible;
cursor: pointer;
vertical-align: middle;
text-decoration: none;
border: 0;
outline: 0;
background-image: none;
text-shadow: none;
-webkit-appearance: none;
-moz-appearance: none;
}display: inline-block is overriding display: none in core/modules/system/css/components/js.module.css
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | raw-interdiff-26-35.txt | 221 bytes | jungle |
| #14 | 3070375-14.patch | 868 bytes | jungle |
Comments
Comment #3
pyrello CreditAttribution: pyrello at The University of Iowa commentedI can confirm this is an issue. With the media library widget, there is a button that is supposed to be hidden labeled "Update widget", but the button shows.
It seems like this is maybe a matter of the order in which the CSS files are being loaded.
Comment #4
lauriiiI know using
!importantis an anti-pattern most of the time. However, I think this could be a valid place to use it. We don't want element styles accidentally overriding the.js-hidestyles since if the class exists and the browser doesn't have JavaScript, the element should be always hidden. If they don't want to hide the element based on that rule, they should remove the class from the element.Before:
After:
Comment #6
komlenic CreditAttribution: komlenic commentedI can confirm that the patch in #4 fixes this, and I have encountered no unexpected side effects.
In my use case, this issue is encountered with adding a Media Library Form API Element to the off-canvas dialog. An "Update widget" button is incorrectly shown that is otherwise hidden on any other forms where this form element is used.
Since this is such a simple fix that is unlikely to cause any issues elsewhere, moving to RTBC.
Comment #7
lauriiiExtending changes to stable9 because it didn't exist at the time I worked on #4.
Comment #8
junglePatch #7 failed to apply. But it seems valid to 9.1.x, queued against 9.1.x. Should we change the target version to 9.1.x? Changing the target version, feel free to change back.
Comment #9
lauriiiMoving back to RTBC because the patch applies against 9.1.x
Comment #10
larowlanCrediting @sacarney who provided a detailed issue summary when reporting this issue 🏆 - thanks!
Comment #13
larowlanI agree that using
!importantstinks, but given a) @lauriii is the FE FM and b) we don't have many other options because so much of the CSS in off-canvas is way too specific, committing this is.As someone who has fought against the css in off-canvas on every project, I feel like we need a follow-up here to make it use BEM instead of IDs (which is the source of our specificity woes I suspect). Is there an existing issue? If not can you create one @lauriii?
I kid you not, our starter themes at PNX have a
libraries-overridedefinition to remove all of the core CSS for that component and a reasonable starter CSS file for something that doesn't clobber all of your styles and allows easy extension when things go wrong - nice to see this button issue fixed in core.Committed 52fedda and pushed to 9.1.x. Thanks!
Because the risk of disruption here is low, backported to 9.0.x - 8.9x doesn't apply - reassigning and tagging for that.
Comment #14
jungle@larowlan, thanks for commiting!
Assuming testing passes.
Comment #15
jungleraw-interdiff-26-35.txt should not be in #14, sorry!
Comment #16
lauriiiRe #13: CSS in off-canvas was deliberately made to use way too specific CSS in #2826722: Add a 'fence' around settings tray with aggressive CSS reset. to make it compatible with as many pre-existing themes as possible. I agree that the current solution is not ideal. We've been planning a new strategy in #2195695: Admin UIs on the front-end are difficult to theme. However, it doesn't seem like the proposed plan there would be something that could be implemented overnight.
Comment #18
larowlanBackported this to 8.9.x
Committed 3d7f169 and pushed to 8.9.x. Thanks!
Thanks @lauriii for the linked issue
Comment #19
jungleComment #20
chr.fritschFYI: This breaks #3168733: Modal widget is hidden since #3070375
Comment #22
lauriiiIt seems like we should use similar approach to #3166068: Autocomplete "loading" message not properly hidden in inline forms here because of this runs into some issues with the way jQuery .show and .hide works. We could reconsider this approach after #3167377: Rewrite jQuery .show() and .hide().
Comment #26
lauriiiI'm wondering if we have a reliable way to solve this until #3167377: Rewrite jQuery .show() and .hide() has been resolved.