Skip to content

Conversation

@carylwyatt
Copy link
Member

@carylwyatt carylwyatt commented Jan 15, 2026

This was a tricky one!

The desired user flow looks like:

  • user with some kind of elevated access logs in
  • user logs in via their institution's login process
  • when user is shuffled back to whichever HT page they initiated login from, they are immediately presented with the role switch modal
  • regardless of the user's choice in this modal, a session cookie is set so they don't get this prompt again until they log in again
  • IF the user also has a new notification, either:
    • the notification modal should appear as soon as the role switch modal is closed
    • or if they chose to switch their role, the site will reload with their new role then they will see the notification modal

UX problems with multiple open modals

Before handling the opening/closing of these modals, the notifications modal was stacked on top of the role switch modal. There isn't anything inherently wrong or inaccessible about "nested" modals/dialogs, but if we added the cookie banner in to the mix, it was just too many floating windows to handle all at once for me to be comfortable calling that good UX.

Handling the sequence of auto-opening modals

Generally, we want modals to be ignorant of data from other modals. Generally, we want the navbar to tell those modals when to open, but we want the modals themselves to be aware of when they close. These principals have been pretty easy to enforce up until this point.

The hierarchy of the components involved in this situation looks kinda like

<Navbar>
  <RoleSwitchModal>
    <Modal>
  <NotificationsModal>
    <Modal>

The Navbar holds the data about loginStatus which includes role information that gets passed down to Role Switch Modal. It also creates the notificationManager class that handles all the logic for notifications that gets passed down into the Notifications Modal. Each of those components then passes other information into the generic Modal component to display the relevant information to the user.

The problem I ran into (or keep running into really) is that the modals have a variety of ways to trigger the hide() function that closes the modal. The buttons at the bottom of the modal will close the modal but usually have some kind of "submit" or "cancel" functionality associated with them and the markup/logic for those buttons lives at the specific component level (like Role Switch Modal or Feedback Form Modal). The "close" button/X icon at the top of the modal simply dismisses the modal without taking any kind of action.

Where this comes into play here is that I needed the Navbar to tell the NotificationsModal to open only after the RoleSwitchModal was closed. This sounds like it shouldn't be that hard to pull off, and it really isn't if we're using the Cancel/Submit buttons at the bottom of the modal. But because the user could also use the X button to close the modal, now we have to find a way to pass the close button info from the generic Modal inside the Role Switch modal up to the Navbar to pass that data back down the Notifications Modal. 😵‍💫

I ended up using callback props that are a sort of event dispatcher so that parent and grandparent components can catch events that bubble up from children components. In this case, the Modal and RoleSwitchModal have a callback prop named "checkForNotifications" that is handled during the click event on Close (Modal) or Cancel (RoleSwitchModal) that then is executed at the Navbar level to check for notifications and open the NotificationsModal if necessary.

Storybook

The new stories for this are in the Navbar stories section. They check for dismissing the role switch modal with and without notifications and cancelling the role switch modal with notifications. All the previous notifications modal and role switch modal stories/tests still pass.

For mysterious reasons (maybe something changed in Chromatic?), the AdvancedSearch stories component/interaction tests started failing (only in the CLI! they were passing in the browser) with an error about the global HT variable not being defined even though it is definitely defined in the related decorator. I tried a bunch of stuff to fix it, but what did the trick was instantiating the AdvancedSearchDecorator following the method outlined in the docs (maybe I was using the wrong version? but then why did it pass before and only now decide to fail?? I don't know).

Role switch prompt cookie

In my limited testing, I haven't gotten the session cookie set by the role switch prompt to actually expire in Chrome. I haven't logged into dev-3 since Friday, but when I logged in today, I didn't get the role switch modal because my "session" cookie was still set. After some searching, it seems that Chrome settings might retain some client-side session cookies when the user has a "pick up where I left off" setting enabled. I double checked this by trying closing the browser after logging in in Firefox and closing the browser, reopening it, and the cookie was gone. If a client-side session cookie isn't going to be as reliable as we hoped, is there a general amount of time our sessions last so I can set the cookie to expire about the same time? Or is it variable?

Update: Ange decided this cookie should expire after 4 hours instead of relying on a session cookie. We'll see how that works and adjust later if necessary.

To test

Anywhere on dev-3: clear your cookies, then log in. Since we all have some kind of elevated access, after you log in, you should be immediately prompted to switch your role. If you switch your role and select "Submit," the page will reload and you will then be presented with your notifications (if you have any... maybe only @aelkiss will have any?). If you don't click submit but instead close the modal with Cancel or the X/Close button at the top of the modal, the page will not reload and you should see the notifications immediately. The HT-role-prompt cookie has been set, and you should not be prompted again for 4 hours or unless you use the "Log out" button from the account dropdown (which removes the cookie).

Page turner playwright tests

I just realized I probably need to update the resource sharing playwright tests to dismiss this modal? I'll check on that now.

Update: Yes, I will fix those tests. With zero cookies, the role switch modal sits on top of the cookie banner, so that will need to be dismissed first. Or have the browser set that cookie before the page loads? I'll figure it out.

Does catalog have any elevated access-related tests that would be affected by this?

Related pull requests

hathitrust/wordpress-hathitrust#51 cleans up some mismatched button styles in modals
hathitrust/babel#179 updates resource sharing user playwright tests to dismiss this modal before testing

title: 'AdvancedSearchForm',
component: AdvancedSearchForm,
decorators: [() => AdvancedSearchDecorator],
decorators: [
Copy link
Member Author

@carylwyatt carylwyatt Jan 15, 2026

Choose a reason for hiding this comment

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

Most of the changes in this file are linting, but this is the important change here. Changing how this decorator is instantiated is what fixed the mysterious failing tests.

focusMyAccountOnClose = false,
focusButtonOnClose = false,
focusDownloadOnClose = false,
setPromptCookie = false,
Copy link
Member Author

@carylwyatt carylwyatt Jan 15, 2026

Choose a reason for hiding this comment

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

We don't normally want a modal to set this cookie, but if we pass the setPromptCookie prop in when we call the component, then we do want it to set the cookie.

data-bs-dismiss="modal"
onclick={() => {
hide();
if (setPromptCookie) {
Copy link
Member Author

Choose a reason for hiding this comment

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

A roundabout way of telling the modal that we only want to check for notifications if this button is clicked from the RoleSwitchModal (the only modal component that passes in the setPromptCookie prop).

}
function openNotificationsModal(event) {
event.preventDefault();
if (event) {
Copy link
Member Author

Choose a reason for hiding this comment

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

While reconfiguring how the notifications modal opens, I had to add a check for there being an event here. Now that the navbar is auto-opening the NotificationsModal sometimes, there is a possibility that there is no event related with that because the user didn't click anything, and there's no need to preventDefault on a click that didn't happen.

let notificationsOpen = $state(false);
let canAutoOpenNotifications = $state(false);
function shouldShowRoleSwitch() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This function determines when to show the role switch modal. If a user is logged in, has a switchable role, and doesn't have the role prompt cookie set, we should see the role switch modal.

<RoleSwitchModal
bind:this={roleSwitchModal}
bind:isOpen={roleSwitchOpen}
checkForNotifications={() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the callback prop in action. The prop is defined in the Modal and RoleSwitchModal and assigned to a click handler, but the function itself is here in the component when it's created and passed in as a callback.

{/if}
<li>
<a
onclick={() => HT.cookieJar.removeItem('HT-role-prompt')}
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove role prompt cookie on logout.

loggedIn: true,
role: 'resourceSharing',
hasActivatedRole: true,
cookieData: {
Copy link
Member Author

Choose a reason for hiding this comment

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

Because we're mocking the cookie, the role switch modal does not appear during this story.

manager.updateTimestamp();
};
onMount(() => {
Copy link
Member Author

@carylwyatt carylwyatt Jan 15, 2026

Choose a reason for hiding this comment

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

onMount was deprecated in Svelte 5 in favor of $effect, and there was conflict between this onMount and the following effect anyway, so I rewrote the whole thing to be more succinct. When I go back to take out the $effect clause I left in the Navbar, I'll clean up the commented out bits that aren't needed anymore. Edit: removed during rebase

isOpen: true,
},
decorators: [() => NotificationModalDecorator],
decorators: [
Copy link
Member Author

Choose a reason for hiding this comment

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

Same fix for the same issue that what was happening with AdvancedSearch stories.

value="cancel"
onclick={() => {
hide();
checkForNotifications();
Copy link
Member Author

Choose a reason for hiding this comment

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

The click handler for the callback prop, but in the RoleSwitchModal component "Cancel" button this time.

this.data[key] = value;
}

hasItem(key) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the hasItem method for the TestCookieJar class so I could test it in storybook.

expires.setDate(expires.getDate() + duration);

var sExpires = '; expires=' + expires.toUTCString();
const sExpires = duration !== 0 ? '; expires=' + expires.toUTCString() : '';
Copy link
Member Author

Choose a reason for hiding this comment

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

To create a client-side session cookie, you omit the expiration when creating a cookie. This skips the "expiration" part of the cookie if you pass in '0' as an duration number.

}
var expires = new Date();
expires.setDate(expires.getDate() + duration);
if (unit === 'hours') {
Copy link
Member Author

Choose a reason for hiding this comment

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

After realizing the role prompt session cookie wasn't working as expected, Ange gave the go-ahead to change this cookie to expire after 4 hours. Our set cookie function had a default setting of days, so I refactored this to also accept hours as an optional "unit" to be passed in.

Copy link

@moseshll moseshll left a comment

Choose a reason for hiding this comment

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

Behaves as described on dev-3. HT-role-prompt has the right TTL and appears and disappears at the right times. Alas, I have no notifications to test this with.

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