Redesign remember-my-choice checkbox in WAYF - #2095
Open
kayjoosten wants to merge 1 commit into
Open
kayjoosten wants to merge 1 commit into
kayjoosten wants to merge 1 commit into
Conversation
Update the WAYF "remember my choice" checkbox to match the new visual
design (checkbox before label, adjusted margins, plain black text
instead of bold blue) and add an accessible tooltip on the per-SP
variant that explains how long a choice is remembered and how to reset
it, using the existing tooltip pattern from consent's attribute list.
- rememberChoice.html.twig is reordered so the checkbox comes before
its label, and now branches on rememberChoicePerIdp: the per-SP
variant shows duration-aware copy ("remembered for 90 days") plus a
tooltip toggle, while the global variant keeps its existing text
unchanged. The tooltip reuses the same checkbox-toggle + CSS sibling
selector approach as Consent/Attributes/tooltip.html.twig, so no new
JavaScript is needed: wayf/mouseBehaviour.js's existing
handleAriaPressed() already runs on every WAYF page load.
- rememberChoicePerIdp and rememberChoiceDuration were already
threaded through WayfViewModel/WayfViewModelFactory/WayfRenderer for
#2065 but never reached the template: wayf.html.twig and
remainingIdps.html.twig now forward both values into
rememberChoice.html.twig.
- RememberChoiceDurationFormatter turns
wayf.remember_choice_per_idp_lifetime (seconds) into a translated
"N days"/"N minutes" string, computed once in WayfViewModelFactory
and only when rememberChoicePerIdp is true, so the translator isn't
invoked on every WAYF render for SPs that don't use the feature.
- New translation keys were added to languages/messages.{en,nl,pt}.php
rather than theme/*/translations, matching where the existing
remember_choice key already lives.
- WayfController's functional-testing fixture gained a
rememberChoicePerIdp query param (defaulting to false, preserving
existing behaviour) so both the per-SP and global variants can be
exercised manually and from Cypress.
- New Cypress coverage in wayf.general.spec.js checks the per-SP
checkbox and tooltip toggle render correctly and that the global
variant has no tooltip element.
Refs: #2066
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implements #2066, redesigning the WAYF "remember my choice" checkbox to
match the new visual design (checkbox before label, adjusted margins,
plain black text instead of bold blue) and adding an accessible tooltip
on the per-SP variant explaining how long a choice is remembered and
how to reset it.
Stacked on #2094
This branches from
wayf-reset-endpoint(#2094, not yet merged), sincethe tooltip copy links to the reset endpoint added there, and the
per-SP flag/config this ticket builds on comes from #2065 (#2091). The
diff shown here is the incremental change for #2066 only; once #2091
and #2094 merge to
main, this PR's base should be retargeted tomain.Details
rememberChoice.html.twigis reordered so the checkbox comes beforeits label, and now branches on
rememberChoicePerIdp: the per-SPvariant shows duration-aware copy ("remembered for 90 days") plus a
tooltip toggle, while the global variant keeps its existing text
unchanged.
pattern as
Consent/Attributes/tooltip.html.twig, so no newJavaScript is needed —
wayf/mouseBehaviour.js's existinghandleAriaPressed()already runs on every WAYF page load andhandles the
aria-pressed/aria-expandedtoggling generically.rememberChoicePerIdpandrememberChoiceDurationwere alreadythreaded through
WayfViewModel/WayfViewModelFactory/WayfRendererfor Remember choice for WAYF per SP #2065 but never reached the template — a real pre-existing gap.
wayf.html.twigandremainingIdps.html.twignow forward bothvalues into
rememberChoice.html.twig.RememberChoiceDurationFormatterturnswayf.remember_choice_per_idp_lifetime(seconds) into a translated"N days"/"N minutes" string.
WayfViewModelFactoryonly computes itwhen
rememberChoicePerIdpis true, so the translator isn't invokedon every WAYF render for SPs using the global (non-per-SP) cookie.
languages/messages.{en,nl,pt}.phprather than
theme/*/translations, matching where the existingremember_choicekey already lives.WayfController's functional-testing fixture gained arememberChoicePerIdpquery param (defaulting tofalse, preservingexisting behaviour for all current callers) so both variants can be
exercised manually and from Cypress.
wayf.general.spec.jschecks that the per-SPcheckbox and tooltip toggle render and function correctly, and that
the global variant has no tooltip element.
Note: pre-existing
it.onlyin wayf.general.spec.jsThis spec file has a pre-existing
it.only('Shows the global site notice', ...)(predates this branch) that currently suppresses everyother test in the file when run locally/in CI. I verified this is
unrelated to my changes: temporarily removing it against the
unmodified base branch reproduces the same 12 pre-existing failures
(IdP count/search assertions), and with it removed my 5 new tests all
pass cleanly. I left it in place per the minimal-diff policy, but
flagging it here since it means the new Cypress tests (and the rest of
this file) aren't currently exercised by CI as things stand — worth a
follow-up ticket to fix the underlying flakiness and remove
.only.Testing
All run inside the Docker dev container (PHP 8.5):
phpmd/phpcs/phpcs-legacy/docheader: clean.eb4: 244/244.unit: 1013/1013 (1003 baseline + 10 new).functional(APP_ENV=test): 121/121.integration: 105/105.defaultsuite: 300/300 scenarios, 5557/5557 steps.yarn lint: clean.wayf.general.spec.js, firefox headless, with thepre-existing
it.onlytemporarily removed for verification only):new tests pass; the 12 pre-existing failures are unchanged from the
base branch (see note above).
Refs: #2066