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

Use button element for entire input replacement #5609

Merged
merged 4 commits into from
Jan 23, 2025
Merged

Use button element for entire input replacement #5609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54a86c3
Replace the native file upload with a button
patrickpatrickpatrick Jan 15, 2025
89aee09
Add aria-label to file upload button replacement
patrickpatrickpatrick Jan 20, 2025
994920a
Update tests for new file upload changes
patrickpatrickpatrick Jan 21, 2025
5d7fb7f
Correct focus for file upload in error summary
patrickpatrickpatrick Jan 21, 2025
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
51 changes: 49 additions & 2 deletions packages/govuk-frontend/src/govuk/components/file-upload/_index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
border: $govuk-border-width-form-element dashed $govuk-input-border-colour;
background-color: $govuk-body-background-colour;

.govuk-file-upload__button,
.govuk-file-upload__pseudo-button,
.govuk-file-upload__status {
// When the dropzone is hovered over, make these aspects not accept
// mouse events, so dropped files fall through to the input beneath them
Expand All @@ -85,7 +85,7 @@
opacity: 0;
}

.govuk-file-upload__button {
.govuk-file-upload__pseudo-button {
width: auto;
margin-bottom: 0;
flex-grow: 0;
Expand All @@ -97,3 +97,50 @@
margin-left: govuk-spacing(2);
}
}

.govuk-file-upload__button:focus {
outline: none;
}

.govuk-file-upload__button:focus .govuk-file-upload__pseudo-button {
outline: 3px solid transparent;
background-color: $govuk-focus-colour;
box-shadow: 0 2px 0 govuk-colour("black");
}

.govuk-file-upload__button:focus .govuk-file-upload__pseudo-button:hover {
border-color: $govuk-focus-colour;
outline: 3px solid transparent;
background-color: govuk-colour("light-grey");
box-shadow: inset 0 0 0 1px $govuk-focus-colour;
}

.govuk-file-upload__button:active .govuk-file-upload__pseudo-button:hover {
background-color: govuk-shade(govuk-colour("light-grey"), 20%);
}

.govuk-file-upload__button {
align-items: center;
display: flex;
padding: 0;
border: 0;
background-color: transparent;
}

.govuk-file-upload:disabled + .govuk-file-upload__button {
pointer-events: none;
}

.govuk-file-upload:disabled + .govuk-file-upload__button .govuk-file-upload__pseudo-button {
opacity: (0.5);

&:hover {
background-color: govuk-colour("light-grey");
cursor: not-allowed;
}

&:active {
top: 0;
box-shadow: 0 $govuk-border-width-form-element 0 govuk-shade(govuk-colour("white"), 60%); // s0
}
}
Comment on lines +130 to +146
Copy link
Member

Choose a reason for hiding this comment

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

@patrickpatrickpatrick One for the PR about the styles, but given we set the disabled attribute on the <button>, we should directly target the <button> rather than use the state of the <input>.

52 changes: 44 additions & 8 deletions packages/govuk-frontend/src/govuk/components/file-upload/file-upload.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export class FileUpload extends ConfigurableComponent {
/** @private */
i18n

/** @private */
id

/**
* @param {Element | null} $root - File input element
* @param {FileUploadConfig} [config] - File Upload config
Expand All @@ -45,38 +48,66 @@ export class FileUpload extends ConfigurableComponent {
)
}

if (!this.$root.id.length) {
throw new ElementError(
formatErrorMessage(FileUpload, 'Form field must specify an `id`.')
)
}

this.id = this.$root.id

this.i18n = new I18n(this.config.i18n, {
// Read the fallback if necessary rather than have it set in the defaults
locale: closestAttributeValue(this.$root, 'lang')
})

this.$label = this.findLabel()

// we need to copy the 'id' of the root element
// to the new button replacement element
// so that focus will work in the error summary
this.$root.id = `${this.id}-input`

// Wrapping element. This defines the boundaries of our drag and drop area.
const $wrapper = document.createElement('div')
$wrapper.className = 'govuk-file-upload-wrapper'

// Create the file selection button
const $button = document.createElement('button')
$button.className =
'govuk-button govuk-button--secondary govuk-file-upload__button'
$button.classList.add('govuk-file-upload__button')
$button.type = 'button'
$button.innerText = this.i18n.t('selectFilesButton')
$button.id = this.id
romaricpascal marked this conversation as resolved.
Show resolved Hide resolved

const buttonSpan = document.createElement('span')
buttonSpan.className =
'govuk-button govuk-button--secondary govuk-file-upload__pseudo-button'
buttonSpan.innerText = this.i18n.t('selectFilesButton')
buttonSpan.setAttribute('aria-hidden', 'true')

$button.appendChild(buttonSpan)
$button.addEventListener('click', this.onClick.bind(this))

// Create status element that shows what/how many files are selected
const $status = document.createElement('span')
$status.className = 'govuk-body govuk-file-upload__status'
$status.innerText = this.i18n.t('filesSelectedDefault')
$status.setAttribute('role', 'status')
$status.setAttribute('aria-hidden', 'true')

$button.appendChild($status)
$button.setAttribute(
'aria-label',
`${this.$label.innerText}, ${this.i18n.t('selectFilesButton')}, ${this.i18n.t('filesSelectedDefault')}`
)

// Assemble these all together
$wrapper.insertAdjacentElement('beforeend', $button)
$wrapper.insertAdjacentElement('beforeend', $status)

// Inject all this *after* the native file input
this.$root.insertAdjacentElement('afterend', $wrapper)

this.$root.setAttribute('tabindex', '-1')
this.$root.setAttribute('aria-hidden', 'true')

// Move the native file input to inside of the wrapper
$wrapper.insertAdjacentElement('afterbegin', this.$root)

Expand All @@ -85,8 +116,8 @@ export class FileUpload extends ConfigurableComponent {
this.$button = $button
this.$status = $status

// Prevent the hidden input being tabbed to by keyboard users
this.$root.setAttribute('tabindex', '-1')
// Bind change event to the underlying input
this.$root.addEventListener('change', this.onChange.bind(this))

// Syncronise the `disabled` state between the button and underlying input
this.updateDisabledState()
Expand Down Expand Up @@ -213,6 +244,11 @@ export class FileUpload extends ConfigurableComponent {
count: fileCount
})
}

this.$button.setAttribute(
'aria-label',
`${this.$label.innerText}, ${this.i18n.t('selectFilesButton')}, ${this.$status.innerText}`
)
}

/**
Expand Down Expand Up @@ -240,7 +276,7 @@ export class FileUpload extends ConfigurableComponent {
* When the button is clicked, emulate clicking the actual, hidden file input
*/
onClick() {
this.$label.click()
this.$root.click()
}

/**
Expand Down
105 changes: 56 additions & 49 deletions packages/govuk-frontend/src/govuk/components/file-upload/file-upload.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const inputSelector = '.govuk-file-upload'
const wrapperSelector = '.govuk-file-upload-wrapper'
const buttonSelector = '.govuk-file-upload__button'
const statusSelector = '.govuk-file-upload__status'
const pseudoButtonSelector = '.govuk-file-upload__pseudo-button'

describe('/components/file-upload', () => {
let examples
Expand Down Expand Up @@ -93,66 +94,59 @@ describe('/components/file-upload', () => {
})

it('renders the button with default text', async () => {
const buttonElementText = await page.$eval(buttonSelector, (el) =>
el.innerHTML.trim()
const buttonElementText = await page.$eval(
Copy link
Member

Choose a reason for hiding this comment

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

suggestion Would it be worth checking that not only the elements this test looks for are on the page, but also inside the button itself?

`${buttonSelector} ${pseudoButtonSelector}`,
(el) => el.innerHTML.trim()
)

expect(buttonElementText).toBe('Choose file')
})
})

describe('status element', () => {
it('renders the status element', async () => {
const statusElement = await page.$eval(statusSelector, (el) => el)

expect(statusElement).toBeDefined()
})

it('renders the status element with role', async () => {
const statusElementRole = await page.$eval(statusSelector, (el) =>
el.getAttribute('role')
const statusElementText = await page.$eval(
`${buttonSelector} ${statusSelector}`,
(el) => el.innerHTML.trim()
)

expect(statusElementRole).toBe('status')
})

it('renders the status element with default text', async () => {
const statusElementText = await page.$eval(statusSelector, (el) =>
el.innerHTML.trim()
const buttonAriaText = await page.$eval(buttonSelector, (el) =>
el.getAttribute('aria-label')
)

expect(buttonElementText).toBe('Choose file')
expect(statusElementText).toBe('No file chosen')
expect(buttonAriaText).toBe(
'Upload a file, Choose file, No file chosen'
)
})
})
})

describe('when clicking the choose file button', () => {
it('opens the file picker', async () => {
// It doesn't seem to be possible to check if the file picker dialog
// opens as an isolated test, so this test clicks the button, tries to
// set a value in the file chooser, then checks if that value was set
// on the input as expected.
const testFilename = 'test.gif'
await render(page, 'file-upload', examples.default)
it.each([buttonSelector, pseudoButtonSelector, statusSelector])(
'opens the file picker',
async (selector) => {
// It doesn't seem to be possible to check if the file picker dialog
// opens as an isolated test, so this test clicks the button, tries to
// set a value in the file chooser, then checks if that value was set
// on the input as expected.
const testFilename = 'test.gif'
await render(page, 'file-upload', examples.default)

const [fileChooser] = await Promise.all([
page.waitForFileChooser(),
page.click(buttonSelector)
])
const [fileChooser] = await Promise.all([
page.waitForFileChooser(),
page.click(selector)
])

await fileChooser.accept([testFilename])
await fileChooser.accept([testFilename])

const inputElementValue = await page.$eval(
inputSelector,
(el) =>
// @ts-ignore
el.value
)
const inputElementValue = await page.$eval(
inputSelector,
(el) =>
// @ts-ignore
el.value
)

// For Windows and backward compatibility, the values of file inputs
// are always formatted starting with `C:\\fakepath\\`
expect(inputElementValue).toBe(`C:\\fakepath\\${testFilename}`)
})
// For Windows and backward compatibility, the values of file inputs
// are always formatted starting with `C:\\fakepath\\`
expect(inputElementValue).toBe(`C:\\fakepath\\${testFilename}`)
}
)
})

describe('when selecting a file', () => {
Expand All @@ -163,7 +157,7 @@ describe('/components/file-upload', () => {

const [fileChooser] = await Promise.all([
page.waitForFileChooser(),
page.click(inputSelector)
page.click(buttonSelector)
romaricpascal marked this conversation as resolved.
Show resolved Hide resolved
])
await fileChooser.accept([testFilename])
})
Expand Down Expand Up @@ -206,7 +200,7 @@ describe('/components/file-upload', () => {

const [fileChooser] = await Promise.all([
page.waitForFileChooser(),
page.click(inputSelector)
page.click(buttonSelector)
])
await fileChooser.accept(['testfile1.txt', 'testfile2.pdf'])
})
Expand Down Expand Up @@ -354,11 +348,24 @@ describe('/components/file-upload', () => {
})

it('uses the correct translation for the choose file button', async () => {
const buttonText = await page.$eval(buttonSelector, (el) =>
const buttonElementText = await page.$eval(
pseudoButtonSelector,
(el) => el.innerHTML.trim()
)

const statusElementText = await page.$eval(statusSelector, (el) =>
el.innerHTML.trim()
)

expect(buttonText).toBe('Dewiswch ffeil')
const buttonAriaText = await page.$eval(buttonSelector, (el) =>
el.getAttribute('aria-label')
)

expect(buttonElementText).toBe('Dewiswch ffeil')
expect(statusElementText).toBe("Dim ffeiliau wedi'u dewis")
expect(buttonAriaText).toBe(
"Llwythwch ffeil i fyny, Dewiswch ffeil, Dim ffeiliau wedi'u dewis"
)
})

describe('status element', () => {
Expand All @@ -373,7 +380,7 @@ describe('/components/file-upload', () => {
it('uses the correct translation when multiple files are selected', async () => {
const [fileChooser] = await Promise.all([
page.waitForFileChooser(),
page.click(inputSelector)
page.click(buttonSelector)
])
await fileChooser.accept(['testfile1.txt', 'testfile2.pdf'])

Expand Down