Problem/Motivation

We have for some years now been overriding H5P styling in Drupal 8, 9, and 10 using a simple custom module that splices in some extra CSS and we have been able to re-style H5P interactions very successful significantly.

The custom module was very simple...

function custom_h5p_overrides_h5p_styles_alter(&$styles, $libraries, $mode) {
  $styles[] = (object) [
    // Path relative to drupal root
    'path' => \Drupal::service('extension.list.module')->getPath('custom_h5p_overrides') . '/css/custom-h5p-overrides.min.css','version' => '?ver=1'
  ];
}

For example, we re-style a multiple choice question like this...

h5p_after_module_update_BEFORE

But since updating to 2.0.0-alpha5, the custom CSS deems no longer to be applied and we are right back where we started...

h5p_after_module_update_AFTER

Can anyone please help us to understand what has changed here and why this very long-standing and reliable method no longer works?

Many thanks

CommentFileSizeAuthor
#14 mutiple-scripts-and-styles.patch1.11 KBpjotr.savitski
#6 styles_alter-3526331-6.patch1.4 KBanzi31
#5 styles_alter-3526331-5.patch1.74 KBanzi31
h5p_after_module_update_AFTER.png22.63 KBsirclickalot
h5p_after_module_update_BEFORE.png21.52 KBsirclickalot

Issue fork h5p-3526331

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

sirclickalot created an issue. See original summary.

filburt’s picture

filburt commented

Hi,

please try with added type declarations - worked for me:

function custom_h5p_overrides_h5p_styles_alter(array &$styles, array $libraries, string $mode): void {
...
}
anzi31’s picture

anzi31 commented

Have the same issue using D11. Neither 2.0.0-alpha5 nor 2.0.0-alpha6 works. Tried #2, but that did not help.
Debugging shows that the hook is not being called. Worked in D10 alpha4.

sirclickalot’s picture

sirclickalot
Primary language English
Location Somerset
commented

@anzi31, Agreed, the hook is simply not being called and #2 could not really make any difference.

We do love H5P for educational project but forward-compatibility of this module just does not seem to me to be going to happen so we are finding ourselves look around for other alternatives.

anzi31’s picture

anzi31 commented
StatusFileSize
new styles_alter-3526331-5.patch1.74 KB

Looks like \Drupal::moduleHandler()->alter() was removed from H5PDefaultFormatter.php.
If you embed a quiz, it works, but if it is an iframe, it does not.

anzi31’s picture

anzi31 commented
StatusFileSize
new styles_alter-3526331-6.patch1.4 KB

Uploaded the wrong file.

illeace’s picture

illeace commented
Status: Active » Reviewed & tested by the community

What I found using Drupal 11 and 2.0.0-alpha6 is that my H5P CSS customizations worked when viewing the H5P embed link, and also were visible when editing the H5P, but not when viewing the node directly. After applying this patch, I also see those CSS customizations when viewing the node. I'm marking this RTBC.

sim_1’s picture

sim_1
she/her
commented

sim_1 opened merge request !35

  • sim_1 committed 7707a518 on 2.0.x 
    Issue #3526331 by anzi31, sirclickalot, filburt, illeace: Can no longer...
sim_1’s picture

sim_1
she/her
commented
Status: Reviewed & tested by the community » Fixed

Merged and marking as fixed. Thanks everyone for working on this and testing!

Status: Fixed » Closed (fixed)

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

pjotr.savitski’s picture

pjotr.savitski commented

Only the first script and style would be added by the

$jsFilePaths[] = $aggregatedJS['scripts'][0][0];

and

$cssFilePaths[] = $aggregatedCSS['styles'][0][0];

Alter hooks could potentially add multiple scripts/styles and the solution should probably use array merge to add all the scripts/styles to existing ones.

The code should be

$jsFilePaths = array_merge($jsFilePaths, $aggregatedJS['scripts'][0]);

and

$cssFilePaths = array_merge($cssFilePaths, $aggregatedCSS['styles'][0]);

This is another issue altogether, but the absolute URL to the style and script does not include the version data. This means that browsers may not refresh the file contents even if the version data point is changed. That is how new aggregatedAssets method handled the provided data.

pjotr.savitski’s picture

pjotr.savitski commented
StatusFileSize
new mutiple-scripts-and-styles.patch1.11 KB

This should fix the issue with just one script/style being added.

sim_1’s picture

sim_1
she/her
commented
Status: Closed (fixed) » Needs review

Thank you so much, @pjotr.savitski for finding that issue. Reopening for testing.

illeace opened merge request !47

illeace’s picture

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

I've review the patch from @pjotr.savitski and confirmed:

  • Prior to the patch, when viewing a node with an H5P or editing an H5P, the first CSS/JS alter file was included, but not the 2nd or 3rd.
  • After the patch, additional alter files beyond the first are correctly included.
  • Note: this change does not affect the embed method of viewing an H5P, which continues to work correctly.

I've created a new feature branch / MR in the Issue fork, which is ready to merge.

  • sim_1 committed 1bef71c6 on 2.0.x authored by illeace 
    [#3526331] fix: Can no longer insert / override CSS since update to 2.0....
sim_1’s picture

sim_1
she/her
commented
Status: Reviewed & tested by the community » Fixed

Great, thanks! Merged second MR and re-marking as fixed.

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

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

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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