Skip to content

Refactor the cookie consent implementation.#5163

Open
EreMaijala wants to merge 25 commits intovufind-org:devfrom
EreMaijala:dev-native-cookie-consent
Open

Refactor the cookie consent implementation.#5163
EreMaijala wants to merge 25 commits intovufind-org:devfrom
EreMaijala:dev-native-cookie-consent

Conversation

@EreMaijala
Copy link
Copy Markdown
Contributor

@EreMaijala EreMaijala commented Mar 23, 2026

Replaces the third party cookie consent component with a VuFind-native approach that combines typical server-side rendering with some client-side JavaScript to reduce the amount of JS initialization code. The actual consent overlay has also been streamlined to include everything in collapsible elements. This improves accessibility and avoids the need for navigating between the overlay and a separate dialog. Also the styling now conforms with the selected theme.

What is lost here is the choice of different layouts (that we didn't really support anyway), but what is gained is that the layout is now customizable with templates.

TODO:

  • Update changelog when merging (changes to popup_description_html language string; changes to CookieConsent view helper constructor and method signatures; significant rewrite of cookie consent internals; replacement of Helpers/cookie-consent.phtml template with CookieConsent/*.phtml files; removal of external library; loss of layout functionality)

Replaces the third party cookie consent component with a VuFind-native approach that combines typical server-side rendering with some client-side JavaScript to reduce the amount of JS initialization code. The actual consent overlay has also been streamlined to include everything in collapsible elements. This improves accessibility and avoids the need for navigating between the overlay and a separate dialog. Also the styling now conforms with the selected theme.
@EreMaijala EreMaijala requested a review from demiankatz March 23, 2026 12:12
@demiankatz demiankatz added the new language strings adds new text in need of translation label Mar 23, 2026
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @EreMaijala, I haven't had time to fully review this yet, but see below for a first round of thoughts. I've also asked @sturkel89 to do some hands-on testing this week (my own review is just based on reading code so far).

@EreMaijala EreMaijala requested a review from demiankatz March 24, 2026 11:43
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @EreMaijala, I've given this a more complete code review today (though no hands-on testing yet). See below for a few more comments.

Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the progress, @EreMaijala -- see below for a couple of things. Additionally, #5172 has been merged, so you should be able to simplify this PR by merging dev in.

}
try {
return JSON.parse(cookieData);
} catch (e) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESLint is complaining about this unused variable:

Suggested change
} catch (e) {
} catch {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but then it doesn't allow removal either. I added a console.warn as a workaround (which can be kept), but we may have a rule conflict.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the console.warn solution makes sense -- it probably is better to notify about problems rather than silently ignoring them. We can revisit investigating the rules if necessary, but maybe forcing ourselves to thoughtfully process exceptions is the right thing to do.

@EreMaijala EreMaijala requested a review from demiankatz March 27, 2026 11:16
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below for a couple more small things.

I think this is in good shape for @sturkel89 to begin her hands-on testing now; further significant code change seems unlikely.

}
try {
return JSON.parse(cookieData);
} catch (e) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the console.warn solution makes sense -- it probably is better to notify about problems rather than silently ignoring them. We can revisit investigating the rules if necessary, but maybe forcing ourselves to thoughtfully process exceptions is the right thing to do.

@EreMaijala EreMaijala requested a review from demiankatz March 27, 2026 13:03
@demiankatz
Copy link
Copy Markdown
Member

Thanks, @EreMaijala, code looks good to me now -- so I'll wait and see what @sturkel89 comes up with!

Copy link
Copy Markdown
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the test branch in Chrome and Firefox, and compared it with dev. I am not sure I fully understand all the implications of this feature, but I'll report on what I see!

I set cookie consent = true in config.ini. When I open localhost/vufind_test in the test branch, I see a cookie banner pop up. It's possible to see more and more details; the basic screen looks like this:

Image

The most expanded screen looks like this:

Image

I found that checking either option would dismiss the cookie banner, and it didn't come back even if I shut down the browser and then opened it again and went to localhost/vufind_test. I also checked Settings to see what was happening with cookies on localhost, and found that regardless of whether I accepted all cookies or just the essential cookies, the system reported that 5 cookies had been set after a bit of clicking around.

When I switched to dev, I got a much simpler screen with code instead of text for the title:

Image

In playing with this version, I also got 5 cookies after making either choice.
[UPDATE: Demian figured out that I hadn't cleared the language cache after switching to dev, which explains this simplified interface. Once I cleared the language cache, this problem went away and the correct version of the pop-up appeared.]

In terms of the user interface, I think I prefer the options shown on the brief screen of "Accept All Cookies" and "Accept Only Essential Cookies" -- it's clearer than the choices that are displayed on the extended screen of "Save Settings" and "Accept Only Essential Cookies." I don't see how you could change any settings, since the Essential Cookies checkbox in the extended screen is not uncheckable.

Please let me know if I'm missing something, or if there's something else specific you'd like me to test!

@sturkel89
Copy link
Copy Markdown
Contributor

Follow-up comment:

Demian and I figured out that there's a setting in config.ini to enable consentCategories, which expands the Details in the cookie popup to include Matomo (tracking) cookie options. The tracking cookie option is unchecked by default.

image

With this full information displayed, the buttons for Save Settings and Accept Only Essential Cookies make more sense.

Question: is it possible to only offer the Save Settings option when consentCategories is enabled? Otherwise, I think Accept All Cookies vs. Accept Only Essential Cookies makes more sense.

@EreMaijala
Copy link
Copy Markdown
Contributor Author

@sturkel89 Good point. I've now updated the code to change the buttons only when there are non-essential categories enabled.

@EreMaijala EreMaijala requested a review from sturkel89 March 30, 2026 12:27
@EreMaijala
Copy link
Copy Markdown
Contributor Author

I had to tweak the markup and consent ID generation after testing to fix issues with accessing the uncollapsed content with screen readers and crypto.randomUUID() only being available in secure contexts (localhost and HTTPS).

Copy link
Copy Markdown
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing my initial suggestion! I'll document what I see now under the two circumstances.

consentCategories commented out

Regardless of the level of "details" accordion the user opens, the same two buttons appear as options:

Image

I'm assuming that these buttons really do what they say they'll do; if the user clicks Accept All Cookies, tracking cookies will be saved.


consentCategories activated

The buttons look like this when the user has NOT expanded the "details" accordion:

Image

But with details expanded so the user can make a choice about the tracking cookies, the buttons look like this:

Image

New idea!

Now that I'm looking at it, it seems to me that in this final scenario, you really only need the Save Settings button! The user makes their choice by clicking or not clicking the checkbox for tracking cookies, and then they will save their settings. The Accept Only Essential Cookies button is redundant and potentially a little ambiguous. I think it could be removed. What do you think?

@EreMaijala
Copy link
Copy Markdown
Contributor Author

@sturkel89 When there are only essential cookies, the two buttons actually do the same thing. However, this is unlikely to be the case in real world where analytics and other external services are likely to be involved. We could only display one button, but that could confuse people who are accustomed to having a choice.

It seems that most people just choose to accept all or essential ones. Usually users just want to get rid of the consent banner and continue with their task at hand. Digging into the categories and individual cookies involved must be possible, but it's a really small number of people actually interested in those.

I think that the Accept Only Essential Cookies is important to have at all times even though the Save Settings button would be sufficient. It provides people comfort knowing that they can dig into the details while there's always the safe choice of backing out and only accepting the essential ones.

@sturkel89
Copy link
Copy Markdown
Contributor

Your explanation works for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new language strings adds new text in need of translation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants