Skip to content

fix(inline): four small bugs around the inline-consent flow#484

Merged
skerbis merged 6 commits into
FriendsOfREDAXO:mainfrom
marcohanke:fix/inline-consent-multiple-issues
Jun 12, 2026
Merged

fix(inline): four small bugs around the inline-consent flow#484
skerbis merged 6 commits into
FriendsOfREDAXO:mainfrom
marcohanke:fix/inline-consent-multiple-issues

Conversation

@marcohanke

Copy link
Copy Markdown
Member

This PR fixes four issues I hit when integrating the inline consent flow
in a customer project on REDAXO 5.20.

Each commit is independent and can be cherry-picked.

1. inline_placeholder.php$consentId / $serviceKey never initialized

InlineConsent::renderPlaceholderHTML() passes both via $fragment->setVar()
but the template only retrieves $service, $options, $placeholderData
and $content via $this->getVar(). $consentId and $serviceKey are
used as bare variables (lines 58, 105 etc.) — REDAXO rex_fragment
doesn't extract() set vars, so both are null. With display_errors
on this produces PHP warnings inside the data-consent-id attribute,
which breaks the JS that reads that attribute.

2. consent_inline.js cookie name mismatch

setCookieData() writes the cookie as consent_manager=… (with
underscore). All read paths (getStorageValue, getCookie('…'),
sessionStorage.getItem) use consentmanager (no underscore).
Result: the consent cookie is set but never found again after reload —
the inline placeholder reappears every page load even when the user
already accepted.

This commit fixes the write name and also cleans up the old
underscore cookie so users who already hit the bug don't end up with
two stale cookies.

3. Documented css_class option is not implemented

docs/inline.md lists 'css_class' as a supported option (line 504,
565, 569, 741, with an example 'consent-theme-minimal') but
grep -r css_class lib/ fragments/ returns no hits. The option is
silently dropped.

This commit appends the option value as an extra class on the outer
.consent-inline-container element so users can theme individual
placeholders the way the docs already promise.

4. Default --consent-overlay-max-width: 90% makes the overlay not cover the placeholder

The placeholder defines --consent-overlay-max-width: 90% (95% on
mobile) on .consent-inline-placeholder, and .consent-inline-overlay
consumes it as max-width: var(--consent-overlay-max-width). In a
wrapping container with a locked aspect-ratio (e.g. a 16:9 video box),
the overlay stays 10% narrower than the thumbnail behind it — the
thumbnail peeks out at the sides and the placeholder looks broken.

There is no backend setting or theme_editor field for this. Overriding
the variable from a wrapper element doesn't work because the var is
re-declared on the placeholder (a descendant), so the wrapper-level
value never reaches the overlay.

This commit sets the default to 100%. Users who want inset spacing
can use the existing --consent-overlay-padding variable instead.


Tested against v5.6.6 in a live customer project. Happy to split into
separate PRs if you'd prefer.

InlineConsent::renderPlaceholderHTML() passes both via setVar() but
the fragment template only retrieves $service/$options/$content via
getVar() — $consentId and $serviceKey were used as bare variables
and ended up undefined. With display_errors on this produces PHP
warnings inside the data-consent-id attribute which breaks the
inline JS that reads that attribute.
setCookieData() wrote 'consent_manager' (with underscore) while
getStorageValue() and all other reads use 'consentmanager' (no
underscore). Result: the consent cookie is never found again
after reload — the inline placeholder reappears every page load
even when the user has clicked "Allow all".

Also adds a one-time cleanup for the legacy underscore cookie so
users who hit the bug don't see two cookies.
docs/inline.md lists 'css_class' as a supported option (line 504,
565, 569, 741 with an example) but no code path uses it. This
appends the value as an extra class on the outer
.consent-inline-container element so users can theme individual
placeholders as the docs already promise.
The default --consent-overlay-max-width: 90% (95% on mobile)
leaves the underlying thumbnail visible at the edges, which
looks broken in 16:9 video wrappers and similar
aspect-ratio-locked containers. There is no backend setting
or theme_editor field to change this — the value is
hardcoded in CSS.

Set the default to 100%. Users who want inset spacing can use
the existing --consent-overlay-padding variable instead.
Copilot AI review requested due to automatic review settings June 12, 2026 08:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR targets multiple small issues in the inline-consent flow of the REDAXO Consent Manager AddOn, improving placeholder rendering, persistence of inline consent decisions, and default overlay styling.

Changes:

  • inline_placeholder.php: initialize consentId / serviceKey via fragment vars and add support for a documented css_class option on the outer container.
  • consent_inline.js: fix the cookie name mismatch by writing consentmanager (and attempt to clean up the legacy consent_manager cookie).
  • consent_inline.css: change the default overlay max width to 100% (desktop + mobile) so the overlay fully covers the placeholder.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
fragments/ConsentManager/inline_placeholder.php Initializes missing fragment vars and applies optional extra CSS class for theming.
assets/consent_inline.js Writes the inline-consent cookie under the correct name and adds legacy-cookie cleanup logic.
assets/consent_inline.css Updates default overlay sizing variable to avoid visible “thumbnail peeking” at the sides.

Comment thread fragments/ConsentManager/inline_placeholder.php Outdated
Comment thread assets/consent_inline.js
Address review comment: $this->getVar('consentId', uniqid(...))
evaluates uniqid() on every render because PHP evaluates default
arguments eagerly. Fetch the var first, only generate a fallback id
when it's actually missing. Avoids unused id allocations and makes
placeholder ids easier to follow when debugging.
Address review comment: the previous patch only cleaned the legacy
'consent_manager' cookie on write. Users who already hit the bug
would still see the placeholder once after the update — until they
re-consent, at which point the write-side cleanup would kick in.

Now getStorageValue() falls back to the legacy cookie name when
'consentmanager' is missing, copies the value over, and expires the
legacy cookie — so the consent state is preserved across the update
without an extra user click.
@skerbis skerbis merged commit 4159687 into FriendsOfREDAXO:main Jun 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants