This is a hard one to explain.

We were having issues where a page where the dialog dependencies added by this module were needed so that the dialog rendered properly and they werent.

Locally it was working and after a lot of hours of debugging I found the library definitions were being cached on production but not locally. I figured this was because on production there was traffic and we were getting the libraries definitions wrongly cached.

Problem is that gin_toolbar_library_info_alter() is checking on _gin_toolbar_gin_is_active, so not adding the dependencies, but the definitions are indeed cached without them. When a logged in page where _gin_toolbar_gin_is_active() would actually be active and the dependencies expected to be added, they are not.

I don't think hook_info_alter() is supposed to be dynamic. That's something that changes globally once and not run again. If adding those dependencies depends on a lot of different issues, I think this needs to be done on some other hook/way.

As a workaround I will add this to my theme:

libraries-extend:
  core/drupal.dialog:
    - claro/claro.drupal.dialog
    - gin/dialog
  core/ckeditor:
    - gin/gin_ckeditor
    - gin/ckeditor
  core/drupal.ajax:
    - claro/ajax
    - gin/ajax
  media_library/widget:
    - claro/media_library.theme
    - gin/media_library
  media_library/view:
    - claro/media_library.theme
    - gin/media_library
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:

Comments

hanoii created an issue. See original summary.

webflo’s picture

I had the same problem and solved by overriding the LibraryDiscoveryCollector service. The cache needs to be aware of the users permissions (at least "access toolbar").

saschaeggi’s picture

@webflo if you have a solution to this issue, feel free to open a MR!

saschaeggi’s picture

Status: Active » Closed (outdated)

Closing as no progress has been made and no further reports have been made.

If this is still a problem feel free to reopen it.

andreasderijcke’s picture

Status: Closed (outdated) » Active
StatusFileSize
new2.11 KB
new56.85 KB
new46.84 KB
new81.07 KB
new262.18 KB

I made a mini module to demonstrate the issue. Find modal_test.zip attached.

I've tested this in a clean Drupal 10.1.3 install, using 'drush site:install' + enable gin and gin_toolbar for admin theme. No other changes than adding the 'modal_test' module. Of course caching is enabled.

After install of 'modal_test', as anonymous user, browse to page '/modal-test/page-with-modal' and click 'Open modal'. This should show:
Modal as anonymous

Next, as admin the same page and modal should show as
Modal as admin

Both cases are as expected, according to #3308200-7: Why do I see dialog.css in my theme?.

Now, clear the caches and view the page and modal again as admin. This should show the same as the previous images.
Now view again as anonymous, this should show the broken theming:
Modal as anonymous with broken theming

In the browser inspector you can see gin theming is applied and missing CSS variables:
Modal broken CSS in browser inspector

Clear the cache, refresh the page as anonymous and see the modal again as shown in the first image.

andreasderijcke’s picture

About solving this, removing

if ($extension == 'core' && isset($libraries['drupal.dialog'])) {
    $libraries['drupal.dialog']['dependencies'][] = 'claro/claro.drupal.dialog';
    $libraries['drupal.dialog']['dependencies'][] = 'gin/dialog';
 }

from gin_toolbar_library_info_alter() solves the problem for anonymous, proving that adding the dependencies in that hook is the cause.

Is extending those libraries in that hook still required?
Removing it entirely, refreshing the page as admin, the modal still shows in Gin theming.

andreasderijcke’s picture

andreasderijcke’s picture

Status: Active » Needs review

Since having the Gin dialog style bleed into the frontend theme is a design choice, as far as I can tell right now, making sure the Gin CSS variables are available in the frontend fixes the problem of the broken dialogs when the cache is 'wrong'.

For the real problem, that the additions in gin_toolbar_library_info_alter are not context aware, I have no suggestion at this time.
Depending on the choices made for Gin 4.x, the problem might become moot.

So, for now, see the MR for suggested workaround + additions in the README to point out the design choice and how to 'opt-out';

andreasderijcke’s picture

StatusFileSize
new1.84 KB

Patch attached, corresponding with the MR at moment of writing.

Liam Morland made their first commit to this issue’s fork.

ricovandevin’s picture

A combination of

libraries-extend:
  core/drupal.dialog:
    - claro/claro.drupal.dialog
    - gin/dialog

to force the dialog CSS to be loaded independent of how the cache was filled and the patch from #10 to force the CSS variables to be loaded provides an acceptable workaround for this issue.

For the long term solution I'd prefer a proper opt-in or opt-out for using the Gin dialog styling for both users with and without permission to see the Gin toolbar. In my opinion the styling should always be the same in both cases for consistency. I think I have a slight preference for an opt-in as it feels bit weird that enabling a toolbar module has an effect on dialog styling. On the other hand I think the Gin (toolbar) styling for dialogs is much prettier without any doubt. :-)

jurgenhaas’s picture

Assigned: Unassigned » jurgenhaas
Issue tags: +Barcelona2024
gurpreet@materialplus’s picture

Hi

Using the gin-toolbar module (8.x-1.x), I have produced this (popup) issue with Drupal versions 10 and 11. However, the issue has been resolved after applying MR #11. I have included both the before and after the video.
Before:- https://www.awesomescreenshot.com/video/32055676?key=9463acd25968bdd3506...
After:- https://www.awesomescreenshot.com/video/32055728?key=f41ee0d7125da876ff5...
Thanks

jurgenhaas’s picture

Note to self, here is a snippet from @webflo on how he addressed this before: https://gist.github.com/webflo/67a380250486f8ab2ccacc73efe21406

duaelfr’s picture

Assigned: jurgenhaas » Unassigned
Status: Needs review » Needs work

This issue is really annoying for websites using dialogs on front-end because gin_toolbar does not take permissions like view the administration theme into account when loading its CSS. The result is broken dialogs for non-admin users.
The MR suggests a workaround to disable all gin dialog-related styles on frontend but it kind of defeats the purpose of the module for admin users. Would it be possible to implement something like @webflo's suggestion?

klemendev’s picture

I agree with @duaelfr, something should be done as this breaks frontend of non-admin users which can be problematic

liam morland’s picture

I have re-rolled the merge request. The merge request should be switched to target 2.0.x.

hanoii’s picture

Title: Library information alter should not be context-aware » Library information alter should be context-aware

I landed here also having issues, in my case ajax.css was being leaked to anonymous users on the frontend breaking my FE dialog.

I think the title should be that the library information alter has to be context-aware, which is what weblo's approach on https://www.drupal.org/project/gin_toolbar/issues/3293209#comment-15892466 does. @jurgenhaas I am curious on how you got to this gist. Excellent! I used this approach and works perfectly and it's a great TIL.

I can see why this cannot be added as an MR to this module, as the context can really be anything particular to the use case of the user, and having a module that alters a core's services is not the best approach.

I went ahead and added #3516920: Make library discovery context aware as I think this can be a good addition to core.

hanoii’s picture

Funny enough, I was just made aware of the fact that I am the original reporter of this issue, go figure! Re-reading what I wrote 3 years ago allowed me to remember that what happened to be back then was the other side of the issue. Anonymous traffic was caching the library discovery, so gin_toolbar CSSs were not added, failing to work for authenticated users.

Either way, webflo's approach sorts out both things and maybe the core issue get some feedback or solution.

hanoii’s picture

Title: Library information alter should be context-aware » Library information alter can't be context-aware, other method is needed

And now I also understand my original title, changing yet once more to something better.

andreasderijcke’s picture

At our company, we are having issues again after our hosting moved to more complicated setup of multi webserver per websites and a shared assets directory.

So far, the problem seemed always to be differences in JS dependency, 'claro/ajax' included by this module, to be specific (as we know so far), that should be included or not depending on the user's role (admin versus editor).
An implementation of the gist https://gist.github.com/webflo/67a380250486f8ab2ccacc73efe21406 seemed to have fixed it on a few projects.

However, a colleague reported the problem again on a layout builder route. Have not been able to pinpoint the mismatch in that case and which permission/context it is related to.

The hosting setup seems to be relevant in some way, as locally, we cannot reproduce this.
In the few occurrences that I managed to debug a bit, what seems odd is that both admin and editor got the same JS inclusions in cached responses, which are correct for one user but not there other. I'd expect these responses to be different (dynamic page cache) for these users, containing the proper JS inclusions for their role.
No clue yet if this is related to this or just a consequence of something else interfering or wrong in the base setup we use.