Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

319 accessibility and narrator issues for overlay #362

Closed
wants to merge 3 commits into from
Closed

319 accessibility and narrator issues for overlay #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aae4a14
Using dialog html element for the overlay
gsproston-scottlogic Oct 5, 2023
eeba251
Can now click outside of the dialog to close
gsproston-scottlogic Oct 6, 2023
4a6f16e
Using theme colour for overlay backdrop
gsproston-scottlogic Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions frontend/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ function App({ isNewUser }: { isNewUser: boolean }) {
);
const [numCompletedLevels, setNumCompletedLevels] = useState<number>(0);

const [showOverlay, setShowOverlay] = useState<boolean>(isNewUser);

const [defencesToShow, setDefencesToShow] =
useState<DefenceInfo[]>(DEFENCE_DETAILS_ALL);

Expand Down Expand Up @@ -67,6 +65,11 @@ function App({ isNewUser }: { isNewUser: boolean }) {
console.error(err);
});
void setNewLevel(currentLevel);
// show the welcome overlay if this is a new user
if (isNewUser) {
const modalDialog = document.querySelector("dialog");
modalDialog?.showModal();
}
}, []);

useEffect(() => {
Expand All @@ -75,12 +78,14 @@ function App({ isNewUser }: { isNewUser: boolean }) {
}, [currentLevel]);

function closeOverlay() {
setShowOverlay(false);
const modalDialog = document.querySelector("dialog");
Copy link
Member

Choose a reason for hiding this comment

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

As before, if you find yourself querying elements from document, you're not doing it React style.

In this case, you could use a forward ref on the HandbookOverlay component. However, it's probably far simpler to pass a prop to HandbookOverlay telling it whether we want it to be open or not, and then use a state var in this App component to store and update that state on clicking the icon.

const [overlayVisible, setOverlayVisible] = useState(false);

function openHandbook() {
  setOverlayType(OVERLAY_TYPE.HANDBOOK);
  setOverlayVisible(true);
}
function closeOverlay() {
  setOverlayVisible(false);
}

<HandbookOverlay {...{ currentLevel, overlayType, closeOverlay }} isOpen={overlayVisible} />

That's not the end of the story though, as you'll need to use the dialog.showModal() function inside HandbookOverlay component, in order to get a modal popup on-demand. So you'll still need a standard React ref to grab a DOM reference to the dialog element. You'll need to use that ref in a useEffect, to fire showModal or closeModal whenever isOpen changes its value.

modalDialog?.close();
}

function openHandbook() {
setOverlayType(OVERLAY_TYPE.HANDBOOK);
setShowOverlay(true);
const modalDialog = document.querySelector("dialog");
modalDialog?.showModal();
}

// methods to modify messages
Expand Down Expand Up @@ -223,13 +228,11 @@ function App({ isNewUser }: { isNewUser: boolean }) {

return (
<div id="app-content">
{showOverlay && (
<HandbookOverlay
currentLevel={currentLevel}
overlayType={overlayType}
closeOverlay={closeOverlay}
/>
)}
<HandbookOverlay
currentLevel={currentLevel}
overlayType={overlayType}
closeOverlay={closeOverlay}
/>
<header id="app-content-header">
<MainHeader
currentLevel={currentLevel}
Expand Down
13 changes: 8 additions & 5 deletions frontend/src/Theme.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,6 @@
);
--control-body-background-colour: #3a3a3a;

--overlay-hidden-colour: #0008;
--overlay-background-colour: #8ad5da;
--overlay-attack-background-colour: #d6d6ff;
--overlay-text-colour: #313131;

--chat-bot-colour: #d482fb;
--chat-bot-background: linear-gradient(var(--chat-bot-colour), #f3dbfe);
--chat-blocked-colour: var(--main-error-colour);
Expand All @@ -59,4 +54,12 @@
--email-background-colour: #93ffcb;
--email-divider-colour: var(--email-text-colour);
--email-text-colour: #000;

--overlay-background-colour: #8ad5da;
--overlay-attack-background-colour: #d6d6ff;
--overlay-text-colour: #313131;
}

::backdrop {
--overlay-hidden-colour: #0008;
}
27 changes: 7 additions & 20 deletions frontend/src/components/HandbookOverlay/HandbookOverlay.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,15 @@
.handbook-overlay-screen {
background-color: var(--overlay-hidden-colour);

position: fixed;
top: 0;
left: 0;
width: 100%;
height: 100%;
z-index: 100;

display: flex;
justify-content: center;
align-items: center;
}

.handbook-overlay {
position: relative;
border: none;

background-color: var(--overlay-background-colour);
color: var(--overlay-text-colour);

display: flex;
justify-content: center;
align-items: center;
overflow-y: auto;

width: 50%;
height: 50%;
border-radius: 10px;
padding: 0;
overflow-y: auto;
}

.handbook-overlay > .close-button {
Expand All @@ -49,6 +32,10 @@
cursor: pointer;
}

.handbook-overlay::backdrop {
background-color: var(--overlay-hidden-colour);
}

.handbook-overlay-content {
height: 100%;
padding: 0 5%;
Expand Down
33 changes: 22 additions & 11 deletions frontend/src/components/HandbookOverlay/HandbookOverlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
/* eslint-disable jsx-a11y/no-static-element-interactions */
/* eslint-disable jsx-a11y/no-noninteractive-element-interactions */
/* eslint-disable jsx-a11y/click-events-have-key-events */
// The above eslint rules are disabled to allow the user to click outside of the dialog to close it.
// Keyboard users are able to close the dialog by pressing the escape key, or tabbing to the close icon.

import { LEVEL_NAMES } from "../../models/level";
import { OVERLAY_TYPE } from "../../models/overlay";
import HandbookAttacks from "./HandbookAttacks";
Expand All @@ -24,18 +30,23 @@ function HandbookOverlay({
}

return (
<div className="handbook-overlay-screen">
<div className="handbook-overlay">
<button
className="prompt-injection-min-button close-button"
onClick={closeOverlay}
aria-label="close handbook overlay"
>
X
</button>
<div className="handbook-overlay-content">{showOverlayByType()}</div>
<dialog className="handbook-overlay" onClick={closeOverlay}>
Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if we should be binding the click to the window, instead of directly to the dialog element, and then determining from the click event whether the click was outside the content box. Then you'd likely not need to disable those linter rules. I'm going to check out your branch and see...

Copy link
Member

Choose a reason for hiding this comment

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

Yes that works 🎉

<button
className="prompt-injection-min-button close-button"
onClick={closeOverlay}
aria-label="close handbook overlay"
>
X
</button>
<div
className="handbook-overlay-content"
onClick={(event) => {
event.stopPropagation();
}}
>
{showOverlayByType()}
</div>
</div>
</dialog>
);
}

Expand Down