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

fix(a11y): always enable continue button on FindProperty select page #3143

Closed
wants to merge 4 commits into from
Closed

fix(a11y): always enable continue button on FindProperty select page #3143

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48a4be0
always enable continue button on findproperty map page
jessicamcinchak May 13, 2024
b2b0e03
wip
jessicamcinchak May 13, 2024
9a5ab94
fix default error msg
jessicamcinchak May 13, 2024
a6de4ca
rebase
jessicamcinchak May 14, 2024
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
42 changes: 27 additions & 15 deletions editor.planx.uk/src/@planx/components/FindProperty/Public/Autocomplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import find from "lodash/find";
import { parse, toNormalised } from "postcode";
import React, { useEffect, useState } from "react";
import InputLabel from "ui/public/InputLabel";
import ErrorWrapper from "ui/shared/ErrorWrapper";
import Input from "ui/shared/Input";

import type { SiteAddress } from "../model";
Expand All @@ -25,6 +26,12 @@ interface PickOSAddressProps {
initialSelectedAddress?: Option;
id?: string;
description?: string;
showPostcodeError: boolean;
setShowPostcodeError: React.Dispatch<React.SetStateAction<boolean>>;
addressAutocompleteError?: string;
setAddressAutocompleteError: React.Dispatch<
React.SetStateAction<string | undefined>
>;
}

const AutocompleteWrapper = styled(Box)(({ theme }) => ({
Expand Down Expand Up @@ -55,7 +62,6 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
const [selectedOption, setSelectedOption] = useState<Option | undefined>(
props.initialSelectedAddress ?? undefined,
);
const [showPostcodeError, setShowPostcodeError] = useState<boolean>(false);

// Fetch blpu_codes records so that we can join address CLASSIFICATION_CODE to planx variable
const { data: blpuCodes } = useQuery(FETCH_BLPU_CODES, {
Expand Down Expand Up @@ -109,6 +115,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
), // display value shown on PropertyInformation, should match <address-autocomplete /> options formatting
source: "os",
});
props.setAddressAutocompleteError(undefined);
}
};

Expand All @@ -124,7 +131,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
}, [sanitizedPostcode, selectedOption]);

const handleCheckPostcode = () => {
if (!sanitizedPostcode) setShowPostcodeError(true);
if (!sanitizedPostcode) props.setShowPostcodeError(true);
};

// XXX: If you press a key on the keyboard, you expect something to show up on the screen,
Expand All @@ -135,7 +142,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
if (selectedOption) {
// Reset the selected address on change of postcode to ensures no visual mismatch between address and postcode
setSelectedOption(undefined);
// Disable the "Continue" button if changing postcode before selecting new address after having come "back"
// Invalidate the "Continue" button if changing postcode before selecting new address after having come "back"
props.setAddress(undefined);
}

Expand All @@ -160,7 +167,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
id="postcode-input"
value={postcode || ""}
errorMessage={
showPostcodeError && !sanitizedPostcode
props.showPostcodeError && !sanitizedPostcode
? "Enter a valid UK postcode"
: ""
}
Expand All @@ -174,7 +181,7 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
maxLength: 8,
"aria-describedby": [
props.description ? DESCRIPTION_TEXT : "",
showPostcodeError && !sanitizedPostcode
props.showPostcodeError && !sanitizedPostcode
? `${ERROR_MESSAGE}-${props.id}`
: "",
]
Expand All @@ -184,16 +191,21 @@ export default function PickOSAddress(props: PickOSAddressProps): FCReturn {
/>
</InputLabel>
{sanitizedPostcode && (
/* @ts-ignore */
<address-autocomplete
id="address-autocomplete"
data-testid="address-autocomplete-web-component"
postcode={sanitizedPostcode}
initialAddress={selectedOption?.title || ""}
osProxyEndpoint={`${process.env.REACT_APP_API_URL}/proxy/ordnance-survey`}
arrowStyle="light"
labelStyle="static"
/>
<ErrorWrapper
error={props.addressAutocompleteError}
id={"address-autocomplete"}
>
{/* @ts-ignore */}
<address-autocomplete
id="address-autocomplete"
data-testid="address-autocomplete-web-component"
postcode={sanitizedPostcode}
initialAddress={selectedOption?.title || ""}
osProxyEndpoint={`${process.env.REACT_APP_API_URL}/proxy/ordnance-survey`}
arrowStyle="light"
labelStyle="static"
/>
</ErrorWrapper>
Copy link
Member Author

@jessicamcinchak jessicamcinchak May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Wrapping the web component in our error wrapper isn't perfect because it means the error message & select label will be out of normal order, with the error message above the label rather than below. I suspect the underlying markup may be a bit imperfect and disconnected too - but haven't inspected closely yet.

I see a couple possible ways forward:

  1. We choose not to address this "usability" feedback - on this page or this whole component?
  2. We always enable the continue button as suggested but ignore this style inconsistency for now if we confirm the markup is okay? It'll never be visible at the same time as the postcode error message, so hard to notice
  3. We update our web component code to implement an error wrapper directly there (and then create/pass a new error prop here?)
    • Looking through all the alpha gov examples of this component, none of them have error handling 🫠 so we'll have to make a number of decisions ourselves here about how to implement https://alphagov.github.io/accessible-autocomplete/examples/
    • This is probably ~1 more day of work. So feasible to still get across the line this week, but not trivial !
  4. Other suggestions?

Screenshot from 2024-05-13 20-36-07

Copy link
Member Author

Choose a reason for hiding this comment

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

Notes from dev call:

  • The right move will be to add error handling & the error wrapper directly in the web component
    • We'll want to additionally consider what this means for the autocomplete's current error state which is an icon & message like "No addresses in this postcode" (eg should it be a disabled select?)
  • As this has been flagged as usability only, and not an A-level issue, we are not going to try to implement this before our re-test next week, but rather before our next annual audit. Since there isn't a clear implementation example of this in the Gov UK alpha docs, we don't want to risk introducing an actual A-level issue at the re-test stage.
  • I'll close this PR and the Trello ticket will go to top of the icebox with these notes!

)}
</AutocompleteWrapper>
);
Expand Down
133 changes: 79 additions & 54 deletions editor.planx.uk/src/@planx/components/FindProperty/Public/Map.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ import {
MapContainer,
MapFooter,
} from "@planx/components/shared/Preview/MapContainer";
import { GeoJSONObject } from "@turf/helpers";
import { useStore } from "pages/FlowEditor/lib/store";
import React, { useEffect, useState } from "react";
import React, { useEffect, useRef, useState } from "react";
import InputLabel from "ui/public/InputLabel";
import ErrorWrapper from "ui/shared/ErrorWrapper";
import Input from "ui/shared/Input";

import { DEFAULT_NEW_ADDRESS_LABEL, SiteAddress } from "../model";
Expand All @@ -23,10 +23,15 @@ interface PlotNewAddressProps {
setAddress: React.Dispatch<React.SetStateAction<SiteAddress | undefined>>;
setPage: React.Dispatch<React.SetStateAction<"os-address" | "new-address">>;
initialProposedAddress?: SiteAddress;
boundary?: GeoJSONObject | undefined;
id?: string;
description?: string;
descriptionLabel?: string;
mapValidationError?: string;
setMapValidationError: React.Dispatch<
React.SetStateAction<string | undefined>
>;
showSiteDescriptionError: boolean;
setShowSiteDescriptionError: React.Dispatch<React.SetStateAction<boolean>>;
}

type Coordinates = {
Expand Down Expand Up @@ -55,8 +60,6 @@ export default function PlotNewAddress(props: PlotNewAddressProps): FCReturn {
const [siteDescription, setSiteDescription] = useState<string | null>(
props.initialProposedAddress?.title ?? null,
);
const [showSiteDescriptionError, setShowSiteDescriptionError] =
useState<boolean>(false);

const [environment, boundaryBBox] = useStore((state) => [
state.previewEnvironment,
Expand Down Expand Up @@ -85,6 +88,17 @@ export default function PlotNewAddress(props: PlotNewAddressProps): FCReturn {
};
}, [setCoordinates]);

const mapValidationErrorRef = useRef(props.mapValidationError);
useEffect(() => {
mapValidationErrorRef.current = props.mapValidationError;
}, [props.mapValidationError]);

useEffect(() => {
if (mapValidationErrorRef.current) {
props.setMapValidationError(undefined);
}
}, [coordinates]);

useEffect(() => {
// when we have all required address parts, call setAddress to enable the "Continue" button
if (siteDescription && coordinates) {
Expand All @@ -99,7 +113,7 @@ export default function PlotNewAddress(props: PlotNewAddressProps): FCReturn {
}, [coordinates, siteDescription]);

const handleSiteDescriptionCheck = () => {
if (!siteDescription) setShowSiteDescriptionError(true);
if (!siteDescription) props.setShowSiteDescriptionError(true);
};

const handleSiteDescriptionInputChange = (
Expand All @@ -111,54 +125,65 @@ export default function PlotNewAddress(props: PlotNewAddressProps): FCReturn {

return (
<>
<MapContainer environment={environment} size="large">
<p style={visuallyHidden}>
An interactive map centred on the local authority district, showing
the Ordnance Survey basemap. Click to place a point representing your
proposed site location.
</p>
{/* @ts-ignore */}
<my-map
id="plot-new-address-map"
ariaLabelOlFixedOverlay="An interactive map for providing your site location"
data-testid="map-web-component"
zoom={14}
drawMode
drawType="Point"
geojsonData={JSON.stringify(props?.boundary)}
geojsonColor="#efefef"
geojsonBuffer={10}
resetControlImage="trash"
showScale
showNorthArrow
osProxyEndpoint={`${process.env.REACT_APP_API_URL}/proxy/ordnance-survey`}
clipGeojsonData={JSON.stringify(boundaryBBox)}
collapseAttributions={window.innerWidth < 500 ? true : undefined}
/>
<MapFooter>
<ErrorWrapper error={props.mapValidationError} id="plot-new-address-map">
<MapContainer environment={environment} size="large">
<p style={visuallyHidden}>
An interactive map centred on the local authority district, showing
the Ordnance Survey basemap. Click to place a point representing
your proposed site location.
</p>
{/* @ts-ignore */}
<my-map
id="plot-new-address-map"
ariaLabelOlFixedOverlay="An interactive map for providing your site location"
data-testid="map-web-component"
zoom={14}
drawMode
drawType="Point"
drawGeojsonData={
coordinates &&
JSON.stringify({
type: "Feature",
geometry: {
type: "Point",
coordinates: [coordinates?.longitude, coordinates?.latitude],
},
properties: {},
})
}
drawGeojsonDataBuffer={20}
resetControlImage="trash"
showScale
showNorthArrow
osProxyEndpoint={`${process.env.REACT_APP_API_URL}/proxy/ordnance-survey`}
clipGeojsonData={JSON.stringify(boundaryBBox)}
collapseAttributions={window.innerWidth < 500 ? true : undefined}
/>
</MapContainer>
</ErrorWrapper>
<MapFooter>
<Typography variant="body2">
The coordinate location of your address point is:{" "}
<strong>
{`${
(coordinates?.x && Math.round(coordinates.x)) || 0
} Easting (X), ${
(coordinates?.y && Math.round(coordinates.y)) || 0
} Northing (Y)`}
</strong>
</Typography>
<Link
component="button"
onClick={() => {
props.setPage("os-address");
props.setAddress(undefined);
}}
>
<Typography variant="body2">
The coordinate location of your address point is:{" "}
<strong>
{`${
(coordinates?.x && Math.round(coordinates.x)) || 0
} Easting (X), ${
(coordinates?.y && Math.round(coordinates.y)) || 0
} Northing (Y)`}
</strong>
I want to select an existing address
</Typography>
<Link
component="button"
onClick={() => {
props.setPage("os-address");
props.setAddress(undefined);
}}
>
<Typography variant="body2">
I want to select an existing address
</Typography>
</Link>
</MapFooter>
</MapContainer>
</Link>
</MapFooter>
<DescriptionInput data-testid="new-address-input" mt={2}>
<InputLabel
label={props.descriptionLabel || DEFAULT_NEW_ADDRESS_LABEL}
Expand All @@ -171,7 +196,7 @@ export default function PlotNewAddress(props: PlotNewAddressProps): FCReturn {
id={`${props.id}-siteDescription`}
value={siteDescription || ""}
errorMessage={
showSiteDescriptionError && !siteDescription
props.showSiteDescriptionError && !siteDescription
? `Enter a site description such as "Land at..."`
: ""
}
Expand All @@ -183,7 +208,7 @@ export default function PlotNewAddress(props: PlotNewAddressProps): FCReturn {
inputProps={{
"aria-describedby": [
props.description ? DESCRIPTION_TEXT : "",
showSiteDescriptionError && !siteDescription
props.showSiteDescriptionError && !siteDescription
? `${ERROR_MESSAGE}-${props.id}`
: "",
]
Expand Down
42 changes: 34 additions & 8 deletions editor.planx.uk/src/@planx/components/FindProperty/Public/Public.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,14 @@ describe("render states", () => {
expect(autocomplete.getAttribute("postcode")).toEqual("SE5 0HU");
expect(autocomplete.getAttribute("initialAddress")).toBeFalsy();

// expect continue to be disabled because an address has not been selected
expect(screen.getByTestId("continue-button")).toBeDisabled();
// user is unable to continue and to submit incomplete data
const continueButton = screen.getByTestId("continue-button");
expect(continueButton).toBeEnabled();
await user.click(continueButton);

expect(
screen.getByTestId("error-message-address-autocomplete"),
).toBeInTheDocument();
expect(handleSubmit).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -198,8 +204,9 @@ describe("render states", () => {
const descriptionInput = screen.getByTestId("new-address-input");
expect(descriptionInput).toBeInTheDocument();

// expect continue to be disabled because an address has not been selected
expect(screen.getByTestId("continue-button")).toBeDisabled();
// Continue button is always enabled, but validation prevents submit until we have complete address details
expect(screen.getByTestId("continue-button")).toBeEnabled();
await user.click(screen.getByTestId("continue-button"));
expect(handleSubmit).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -232,7 +239,11 @@ describe("render states", () => {
await screen.findByText("Type your postal code"),
).toBeInTheDocument();

expect(screen.getByTestId("continue-button")).toBeDisabled();
// user is unable to continue and submit incomplete data
expect(screen.getByTestId("continue-button")).toBeEnabled();
await user.click(screen.getByTestId("continue-button"));

expect(screen.getByText("Enter a valid UK postcode")).toBeInTheDocument();
expect(handleSubmit).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -315,11 +326,14 @@ describe("picking an OS address", () => {
});

it("updates the address-autocomplete props when the postcode is changed", async () => {
const handleSubmit = jest.fn();

const { user } = setup(
<MockedProvider mocks={findAddressReturnMock} addTypename={false}>
<FindProperty
description="Find your property"
title="Type your postal code"
handleSubmit={handleSubmit}
/>
</MockedProvider>,
);
Expand Down Expand Up @@ -350,7 +364,13 @@ describe("picking an OS address", () => {

// User is unable to continue and to submit incomplete data
const continueButton = screen.getByTestId("continue-button");
expect(continueButton).toBeDisabled();
expect(continueButton).toBeEnabled();
await user.click(continueButton);

expect(
screen.getByTestId("error-message-address-autocomplete"),
).toBeInTheDocument();
expect(handleSubmit).not.toHaveBeenCalled();
});

it("recovers previously submitted address when clicking the back button", async () => {
Expand Down Expand Up @@ -416,9 +436,15 @@ describe("plotting a new address that does not have a uprn yet", () => {
screen.getByText(`Enter a site description such as "Land at..."`),
).toBeInTheDocument();

// expect continue to be disabled because we have incomplete address details
expect(screen.getByTestId("continue-button")).toBeDisabled();
// Continue button is always enabled, but validation prevents submit until we have complete address details
expect(screen.getByTestId("continue-button")).toBeEnabled();
await user.click(screen.getByTestId("continue-button"));
expect(handleSubmit).not.toHaveBeenCalled();

// continue should trigger map error wrapper too
expect(
screen.getByTestId("error-message-plot-new-address-map"),
).toBeInTheDocument();
});

it("recovers previously submitted address when clicking the back button and lands on the map page", async () => {
Expand Down
Loading
Loading