Problem/Motivation

The gin_toolbar module references the claro/claro.drupal.dialog library to handle the dialog styling.

However adding this dependency without the css/base/variables.css defined in claro/global-styling results in an inconsistent UI, because its claro/claro.jquery.ui dependency still references variables which are only defined in that file.

Steps to reproduce

1. Install gin_toolbar
2. Using a non-claro front end theme, add an element which uses an autocomplete form element (e.g. webform).

/core/themes/claro/css/components/jquery.ui/theme.css will be loaded, but the autocomplete styles will be broken because the variables within the following rules fail to resolve:

.ui-autocomplete {
    color: var(--jui-dropdown-fg-color);
    border: var(--input-border-size) solid var(--jui-dropdown-border-color);
    border-top: 0;
    border-radius: 0 0 var(--input-border-radius-size) var(--input-border-radius-size);
    background: var(--jui-dropdown-bg-color);
    box-shadow: 0 0.125rem 0.25rem var(--jui-dropdown-shadow-color);
}

chrome dev tools with missing variables

Proposed resolution

The css/base/variables.css should be exposed as its own library definition, then have all the libraries which rely on it include it as a dependency.

That way individual components can be used without the global styles.

Remaining tasks

Provide MR

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

TBD

CommentFileSizeAuthor
#11 3554220-nr-bot_h096tb6s.txt91 bytesneeds-review-queue-bot
#9 3554220-nr-bot_m3ce71c7.txt91 bytesneeds-review-queue-bot
drupal-claro-missing-variables.png29.61 KBcodebymikey

Issue fork drupal-3554220

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

codebymikey created an issue. See original summary.

codebymikey’s picture

codebymikey commented

Ran grep -l 'var(--' . -R | sort | uniq | grep -v 'pcss' on the claro theme, and it shows the following files has having custom properties:

./css/base/elements.css
./css/base/variables.css
./css/components/accordion.css
./css/components/action-link.css
./css/components/ajax-progress.module.css
./css/components/autocomplete-loading.module.css
./css/components/breadcrumb.css
./css/components/button.css
./css/components/card.css
./css/components/ckeditor5.css
./css/components/content-header.css
./css/components/details.css
./css/components/dialog.css
./css/components/divider.css
./css/components/dropbutton.css
./css/components/entity-meta.css
./css/components/fieldset.css
./css/components/file.css
./css/components/form--checkbox-radio.css
./css/components/form--field-multiple.css
./css/components/form--managed-file.css
./css/components/form--password-confirm.css
./css/components/form--select.css
./css/components/form--text.css
./css/components/form.css
./css/components/icon-link.css
./css/components/image-preview.css
./css/components/image.admin.css
./css/components/jquery.ui/theme.css
./css/components/media-library.ui.css
./css/components/menus-and-lists.css
./css/components/messages.css
./css/components/modules-page.css
./css/components/page-title.css
./css/components/pager.css
./css/components/progress.css
./css/components/shortcut.css
./css/components/skip-link.css
./css/components/system-admin--admin-list.css
./css/components/system-admin--modules.css
./css/components/system-admin--panel.css
./css/components/system-status-counter.css
./css/components/system-status-report-general-info.css
./css/components/system-status-report.css
./css/components/table--file-multiple-widget.css
./css/components/tabledrag.css
./css/components/tables.css
./css/components/tableselect.css
./css/components/tablesort-indicator.css
./css/components/tabs.css
./css/components/vertical-tabs.css
./css/components/views-exposed-form.css
./css/components/views-ui.css
./css/components/views_ui.admin.css
./css/layout/breadcrumb.css
./css/layout/card-list.css
./css/layout/form-two-columns.css
./css/layout/local-actions.css
./css/theme/field-ui.admin.css
./css/theme/filter.theme.css
./css/theme/install-page.css
./css/theme/maintenance-page.css
./css/theme/media-library.css
./css/theme/tour.theme.css
./css/theme/views_ui.admin.theme.css

So I've gone through the individual libraries and added the variables.css as a dependency.

codebymikey changed the visibility of the branch 3554220-claro-variables-dependency-10.4.x to hidden.

codebymikey opened merge request !13565

codebymikey’s picture

codebymikey commented
Issue summary: View changes
Status: Active » Needs review
smustgrave’s picture

smustgrave commented

Isn't that a bug in gin_toolbar? That it allows you to use it on a non gin and in turn non claro theme.

codebymikey’s picture

codebymikey commented
Related issues: +#3253148: Remove IE from core's browserlist, remove non-essential CSS importing and recompile assets

Going by the maintainer's comments in #3181937-10: Remove dialog libraries, it appears it's a part of the feature set the module tries to support.

I agree it's probably not best to mix and match theme libraries, but given that the toolbar is shown on both the frontend and backend, some users might prefer having the consistent editing experience when using the gin_toolbar.

Given that it seemed to work before #3253148: Remove IE from core's browserlist, remove non-essential CSS importing and recompile assets, I think declaring the dependency explicitly seems like a viable solution here as there's no easy way for contrib to enforce the inclusion of variables.css otherwise, and its inclusion shouldn't break anything

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Alright the change does make sense, and applying locally seems non disruptive. Maybe can land in 11.3 but definitely 11.4

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3554220-nr-bot_m3ce71c7.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

codebymikey’s picture

codebymikey commented
Status: Needs work » Reviewed & tested by the community

Not sure why the bot flagged it as NW, but MR still applies.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3554220-nr-bot_h096tb6s.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

codebymikey’s picture

codebymikey commented
Status: Needs work » Needs review
Issue tags: +no-needs-review-bot

Disabling the review bot.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Restoring previous status. I reverted back to 11.x as main was failing and since there's no php changes here I don't think phpcs would be failing.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Backported to 11.3.x as an eligible bug fix, I don't think this will be disruptive.

Committed and pushed 0d046164d88 to main and 9b17226b4c7 to 11.x and a2ac2dc8228 to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !13565

  • longwave committed a2ac2dc8 on 11.3.x 
    fix: #3554220 Claro's libraries don't enforce the variables.css...

  • longwave committed 9b17226b on 11.x 
    fix: #3554220 Claro's libraries don't enforce the variables.css...

  • longwave committed 0d046164 on main 
    fix: #3554220 Claro's libraries don't enforce the variables.css...

Status: Fixed » Closed (fixed)

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