LG-15064: Countdown alerts for expiring phone OTP announces too frequently by jmdembe · Pull Request #11899 · 18F/identity-idp

jmdembe · 2025-02-19T16:09:00Z

🎫 Ticket

Link to the relevant ticket:
LG-15064

🛠 Summary of changes

This ticket addresses a bug where the alert component announces the timer countdown every second. The alert will now announce once every 30 seconds.

📜 Testing Plan

Optional: Set otp_valid_for value for anything between 3-5 minutes to make testing more managable.
Turn on VoiceOver to see the fix

Account creation

Follow steps to create an account
When prompted to select an MFA method, select "Text or voice message"
Enter a phone number
Land on the OTP verification screen. Wait 8 minutes for the announcement to begin
See the banner once OTP code expires

👀 Screenshots

finished.screen.recording.mov

changelog: Bug fixes, accessibility, slow down countdown frequency

aduth · 2025-02-19T18:04:34Z

app/components/countdown_alert_component.rb

+              :tag_options

  def initialize(
+    screen_reader_frequency: nil,


Rather than adding a new option here, did you consider using the approach that we use in the session timeout modal? Namely, having two separate countdowns, one of which is used for assistive technology announces at a slower interval:

identity-idp/app/views/session_timeout/_warning.html.erb

Lines 10 to 38 in 2b6458b



<%= t(

# i18n-tasks-use t('notices.timeout_warning.signed_in.message_html')

# i18n-tasks-use t('notices.timeout_warning.partially_signed_in.message_html')

'message_html',

scope: modal_presenter.translation_scope,

time_left_in_session_html: render(

CountdownComponent.new(

expiration: Time.zone.now,

start_immediately: false,

),

),

) %>



" role="timer" aria-live="polite" aria-atomic="true">

<%= t(

# i18n-tasks-use t('notices.timeout_warning.signed_in.live_region_message_html')

# i18n-tasks-use t('notices.timeout_warning.partially_signed_in.live_region_message_html')

'live_region_message_html',

scope: modal_presenter.translation_scope,

time_left_in_session_html: render(

CountdownComponent.new(

expiration: Time.zone.now,

update_interval: 30.seconds,

start_immediately: false,

),

),

) %>

I thought that I was going in this direction, but I am pivoting because I originally thought it was to use the one element.

aduth · 2025-02-21T16:08:46Z

app/views/two_factor_authentication/otp_verification/show.html.erb

+      <%= render CountdownComponent.new(
+        expiration: Time.zone.now,
+        update_interval: 30.seconds,
+        start_immediately: true,


One thing we probably want to be conscious of here is that I don't know that we want the timer to start announcing "9 minutes and 30 seconds remaining" to a screen reader user, and for every 30 seconds thereafter. The alert component above is configured to only start counting down once it gets closer to the timeout (2m30s IIRC).

Yeah the tricky thing that I think is happening is that we are dealing with two timers that need to tick at 2 different paces. Then when the banner shows up, it is counting down by the update_interval of one second while the countdown changes every 5 seconds. There has to be a way in which we silence the alert so that we only hear from the countdown, but I'm thinking of how to do that right now.

This feature works differently from the session timer because the popup shows once and the times are synced together.

aduth · 2025-02-21T16:10:22Z

app/views/two_factor_authentication/otp_verification/show.html.erb

+
+    <p class="usa-sr-only" role="timer" aria-live="polite" aria-atomic="true">
+      <%= render CountdownComponent.new(
+        expiration: Time.zone.now,


Not sure if this is temporary, but the expiration will need to be in the future matching @presenter.otp_expiration above.

…have space for countdown

…y countdown

…-idp into jd/LG-15064-countdown-timer

jmdembe · 2025-02-27T21:09:52Z

During testing, I found it interesting that the screen reader did not announce at the same time it appeared. It may be worth changing the expiration time on the countdown to match the banner. I don't think it is a good practice to have something visible for 30 seconds and after that 30 seconds is over, then the time is announced.

…ynced to announce.

…anner

jmdembe · 2025-02-28T17:18:01Z

What I attempted to do in 51a607e was to get the usa-sr-only banner to appear a configured amount of seconds before the visible banner so that the usa-sr-banner would announce in sync to the visible banner.

kevinsmaster5 · 2025-02-28T19:05:29Z

app/javascript/packages/countdown/countdown-alert-element.spec.ts

    document.body.innerHTML = `
-      <lg-countdown-alert ${showAtRemaining ? `show-at-remaining="${showAtRemaining}"` : ''}>
+      <lg-countdown-alert ${showAtRemaining ? `show-at-remaining="${showAtRemaining}"` : ''}
+      }>


Is there a stray close curly brace here? It's not failing or anything but just wondering about the change.

Yeah it looks like it was a stray. It was hard to discern because it was made to be text.

vrajmohan · 2025-02-28T19:23:26Z

config/application.yml.default

  max_mail_events: 2
  otp_delivery_blocklist_findtime: 1
  otp_delivery_blocklist_maxretry: 2
+


Any reason for this break?

ah, I had that change staged, but not committed. Fixed that in d261989

vrajmohan · 2025-02-28T19:24:19Z

app/components/countdown_alert_component.rb


 class CountdownAlertComponent < BaseComponent
-  attr_reader :show_at_remaining, :alert_options, :countdown_options, :tag_options
+  attr_reader :show_at_remaining, :alert_options, :countdown_options,


Any reason for this change?

That change was made by the linter. I don't think it needs to be linted in that way, so I will restore the change.

vrajmohan · 2025-02-28T19:41:20Z

app/controllers/two_factor_authentication/otp_verification_controller.rb

    include TwoFactorAuthenticatable
    include MfaSetupConcern
    include NewDeviceConcern
+    include SessionTimeoutWarningHelper


Are we using anything from that module?

vrajmohan · 2025-02-28T19:48:51Z

app/javascript/packages/countdown/countdown-alert-element.spec.ts

  function createElement({ showAtRemaining }: { showAtRemaining?: number } = {}) {
    document.body.innerHTML = `
-      <lg-countdown-alert ${showAtRemaining ? `show-at-remaining="${showAtRemaining}"` : ''}>
+      <lg-countdown-alert ${showAtRemaining ? `show-at-remaining="${showAtRemaining}"` : ''} >


Minor, is the space before the '>' conventional?

My mistake. Addressed this and your above comment in 64f1126

vrajmohan

Works as specified, looks good.

app/components/countdown_component.rb

Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>

aduth · 2025-02-28T20:12:11Z

app/components/alert_component.html.erb

@@ -1,4 +1,4 @@
-<%= tag.div(**tag_options, class: css_class, role:) do %>
+<%= tag.div(role:, **tag_options, class: css_class) do %>


It'd be good to include some test coverage for this change, specifically that it's possible to override the default role with another value.

kevinsmaster5

Looks good! 👍

jmdembe added 3 commits February 18, 2025 13:49

add update interval to info on otp expirateion screen

cfb0a6d

add class and value for screen reader frequency

bb0c9ac

remove update frequency

e704197

changelog: Bug fixes, accessibility, slow down countdown frequency

jmdembe force-pushed the jd/LG-15064-countdown-timer branch from 23a9f9a to e704197 Compare February 19, 2025 16:11

jmdembe added 4 commits February 19, 2025 11:31

use store value instead of hardcoded number

ee4abb0

update documentation and test setup

3b17adf

add test for new class

566a04b

first round of lintfixes for tests

2ddb808

aduth reviewed Feb 19, 2025

View reviewed changes

jmdembe added 5 commits February 21, 2025 09:07

udo changes

b78cb27

trying with the countdown component in screen reader container

c0c0bcc

clean up syntax

3ffc187

change start immediately

ad641ae

WIP: lintfix and creating untested modal

06a9f5c

aduth reviewed Feb 21, 2025

View reviewed changes

jmdembe added 14 commits February 21, 2025 11:16

clean up readme file

fd12c11

more changes to alerts

a49d509

positive step: turn aria-live to off when alert appears so that we …

49dc930

…have space for countdown

lintfix: remove console.log

c11b920

remove more screenReaderFrequency, try putting an attribute to dela…

04e4323

…y countdown

lintfixes

17ae171

add class to denote delay?

e535937

add class to denote delay?

e9bf0d2

Merge branch 'jd/LG-15064-countdown-timer' of github.com:18F/identity…

fa8b30f

…-idp into jd/LG-15064-countdown-timer

remove tag_options because that did not work

9533be7

remove delay_countdown_announcement, change to '<div>' tags

6b6e1d1

add new class, function with comment on where to go next

a5a61b4

remove added classes again, make change to countdown element

bf2c0c8

lintfix

ac114ec

jmdembe marked this pull request as ready for review February 27, 2025 21:06

jmdembe marked this pull request as draft February 28, 2025 15:14

jmdembe added 2 commits February 28, 2025 12:01

add modifier to time so that screenreader time and visible time are s…

2087cc6

…ynced to announce.

make update to try and get show_at_remaining to sync with visible b…

51a607e

…anner

jmdembe added 3 commits February 28, 2025 12:19

rename configuration and console.log

db97b5a

remove config because it does not need to be hard coded

ec3dda9

a11y test fix

f9a9c46

jmdembe marked this pull request as ready for review February 28, 2025 17:56

kevinsmaster5 reviewed Feb 28, 2025

View reviewed changes

vrajmohan reviewed Feb 28, 2025

View reviewed changes

address code review comments

d261989

vrajmohan reviewed Feb 28, 2025

View reviewed changes

address unused helper and > in html

64f1126

vrajmohan approved these changes Feb 28, 2025

View reviewed changes

aduth reviewed Feb 28, 2025

View reviewed changes

app/components/countdown_component.rb Outdated Show resolved Hide resolved

Update app/components/countdown_component.rb

aea01dd

Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>

aduth reviewed Feb 28, 2025

View reviewed changes

test to show that can be overriden

b17418f

kevinsmaster5 approved these changes Mar 3, 2025

View reviewed changes

jmdembe merged commit 2951b4d into main Mar 4, 2025
2 checks passed

jmdembe deleted the jd/LG-15064-countdown-timer branch March 4, 2025 14:59

jmhooper mentioned this pull request Mar 4, 2025

Deploy RC 457 to Production #11943

Merged

	<p>
	<%= t(
	# i18n-tasks-use t('notices.timeout_warning.signed_in.message_html')
	# i18n-tasks-use t('notices.timeout_warning.partially_signed_in.message_html')
	'message_html',
	scope: modal_presenter.translation_scope,
	time_left_in_session_html: render(
	CountdownComponent.new(
	expiration: Time.zone.now,
	start_immediately: false,
	),
	),
	) %>
	</p>
	<p class="usa-sr-only" id="<%= c.description_id %>" role="timer" aria-live="polite" aria-atomic="true">
	<%= t(
	# i18n-tasks-use t('notices.timeout_warning.signed_in.live_region_message_html')
	# i18n-tasks-use t('notices.timeout_warning.partially_signed_in.live_region_message_html')
	'live_region_message_html',
	scope: modal_presenter.translation_scope,
	time_left_in_session_html: render(
	CountdownComponent.new(
	expiration: Time.zone.now,
	update_interval: 30.seconds,
	start_immediately: false,
	),
	),
	) %>
	</p>

		@@ -1,4 +1,4 @@
		<%= tag.div(**tag_options, class: css_class, role:) do %>
		<%= tag.div(role:, **tag_options, class: css_class) do %>

Conversation

jmdembe commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdembe commented Feb 27, 2025

Uh oh!

jmdembe commented Feb 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrajmohan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinsmaster5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmdembe commented Feb 19, 2025 •

edited

Loading