Page MenuHomePhabricator

Update CookieWarning so it doesn't use the SkinTemplateOutputPageBeforeExec hook
Closed, ResolvedPublic


The SkinTemplateOutputPageBeforeExec hook will be soft deprecated in T60137

Preferred hooks are listed in

In 1.36 the Vector skin will no longer run this hook and depending on migration efforts the hook may be hard deprecated.

Let me know if you have any questions or need any help with this migration. I'll be happy to help you!

Event Timeline

@Jdlrobson Hi, I'm wondering if you know what hook this extension should migrate? I haven't been able to find a hook that allows you to insert into the header element.

It's a banner so would SiteNoticeAfterHook or SiteNoticeBeforeHook be suitable?

@Jdlrobson thanks for the response! I found a replacement hook for the desktop version onSkinAfterContent, since it'll still shows the banner as if you did the code just under <body> though under MobileFrontend this shows in the footer thus this hook can only be used for the desktop. I've tried SiteNoticeAfterHook & SiteNoticeBeforeHook for mobile, the only issue is it displays where the site notice is, not at the top. Thus it's styles get lost (it's white the background, but because the background is already white it looks weird).

See screenshot linked:

Screenshot 2020-10-05 at 02.58.35.png (2×3 px, 873 KB)

compared to

Screenshot 2020-10-05 at 02.59.13.png (2×3 px, 1006 KB)

Is it possible to add a hook to add elements just right under <body>?

I've tried SiteNoticeAfterHook & SiteNoticeBeforeHook for mobile, the only issue is it displays where the site notice is, not at the top. T

This is a much better location for the cookie warning. Banners on Minerva above the header tend to break the UI as Minerva was not designed for anything displayed above the header.

What is your concern with this placement - is this based purely on how it used to work? This seems like a massive improvement to me.

@Jdlrobson I mean I am not sure if this looks nice

Screenshot 2020-10-05 at 17.10.27.png (2×1 px, 701 KB)
. Does this seem ok?

Also another concern is what if someone adds a site notice, the cookie notice would be shown under rather than on top, is that fine? The before hook only allowed you to override the site notice or allowed the configured one (not add one before the site notice you configured.).

Seems fine to me - this is consistent with how Minerva is now showing CentralNotice banners, although I would probably add a bottom border to the banner to distinguish it from the bottom with a color that matches the header background.

@Paladox I think 1px is all that's needed here. You'll also want to remove those margins to the left and right. I'm not sure where they are coming from.

Here's a CentralNotice banner for reference which is using border-bottom: 1px solid #c8ccd1;:

Screen Shot 2020-10-05 at 2.28.14 PM.png (594×1 px, 104 KB)

@Jdlrobson that looks nice! How was that managed (as in it spread out like that, and the "dismissible" button hidden?

Am not sure why the dismissable button is showing up for you can you share the HTML of that page or even better a test URI?

(I would guess there is a compatibility problem with the DismissableSiteNotice extension.)

I am not seeing the cookie warning on the given URL.

Oh, I think your US based. Which we've switched it off for. I'll switch it on so you can see. Please try again. @Jdlrobson

I'm not sure what's going on here, but it seems that the CookieWArning extension is being wrapped in a DismissableSiteNotice banner. DismissableSiteNotice is not listed as a dependency of that extension. Can you disable that extension and confirm it works without that enabled?

I suspect there is a bug in that extension. When I apply the following rules CookieWarning looks fine:

.mw-cookiewarning-container { border-bottom: 1px solid #c8ccd1; }
.sitedir-ltr .mw-dismissable-notice-body { margin: 0; }

Please raise a bug for DismissableSiteNotice. We're currently moving this code into core (T259903) so should be able to fix styling in Minerva as part of that move.

Change 631814 had a related patch set uploaded (by Paladox; owner: Paladox):
[mediawiki/extensions/CookieWarning@master] Replace deprecated hook

@Jdlrobson Is what it looks like now with ( , which seems fine for now.

Screenshot 2020-10-06 at 00.45.34.png (2×3 px, 863 KB)


Screenshot 2020-10-06 at 00.45.47.png (2×1 px, 700 KB)

Doing should make it look like:

Screenshot 2020-10-06 at 00.46.53.png (2×3 px, 860 KB)


Screenshot 2020-10-06 at 00.47.04.png (2×1 px, 701 KB)

Change 631814 merged by jenkins-bot:
[mediawiki/extensions/CookieWarning@master] Replace deprecated hook

Jdlrobson claimed this task.

Suggested follow up: I would suggest reviewing the absolutely positioned banner given the upcoming changes to Vector detailed here:

Screen Shot 2020-10-14 at 1.58.02 PM.png (594×2 px, 207 KB)


I testet the new Master branch on my wiki. Now it works with the Vector skin. but now my mobile view with MinervaNeue doesn't work anymore. With the old REL1_35 branch it's just the other way round. I hope you could also fix that.

Change 643961 had a related patch set uploaded (by Paladox; owner: Paladox):
[mediawiki/extensions/CookieWarning@REL1_35] Replace deprecated hook

Change 643961 merged by jenkins-bot:
[mediawiki/extensions/CookieWarning@REL1_35] Replace deprecated hook