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

Throw ElementError for missing elements during Character count instantiation #4261

Merged
merged 6 commits into from
Sep 27, 2023
Merged

Throw ElementError for missing elements during Character count instantiation #4261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ac66ad5
Add throwing of error when textarea is not of the right type
romaricpascal Sep 25, 2023
1b7d2f6
Add throwing of error when the textarea description is missing
romaricpascal Sep 25, 2023
0c3fb2d
Fix Character count's JSDOM tests
romaricpascal Sep 26, 2023
adc0380
Expand the use of `textContent` rather than `innerText` to the other …
romaricpascal Sep 26, 2023
f28eee0
Remove quotes around type in ElementError message
romaricpascal Sep 26, 2023
808f7ec
Remove need for cloning helper in Character Count JSDOM tests
romaricpascal Sep 26, 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
8 changes: 4 additions & 4 deletions packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ export class Accordion extends GOVUKFrontendComponent {
? this.i18n.t('hideSection')
: this.i18n.t('showSection')

$showHideText.innerText = newButtonText
$showHideText.textContent = newButtonText
$button.setAttribute('aria-expanded', `${expanded}`)

// Update aria-label combining
Expand All @@ -420,12 +420,12 @@ export class Accordion extends GOVUKFrontendComponent {
`.${this.sectionHeadingTextClass}`
)
if ($headingText instanceof HTMLElement) {
ariaLabelParts.push($headingText.innerText.trim())
ariaLabelParts.push($headingText.textContent.trim())
}

const $summary = $section.querySelector(`.${this.sectionSummaryClass}`)
if ($summary instanceof HTMLElement) {
ariaLabelParts.push($summary.innerText.trim())
ariaLabelParts.push($summary.textContent.trim())
}

const ariaLabelMessage = expanded
Expand Down Expand Up @@ -497,7 +497,7 @@ export class Accordion extends GOVUKFrontendComponent {
: this.i18n.t('showAllSections')

this.$showAllButton.setAttribute('aria-expanded', expanded.toString())
this.$showAllText.innerText = newButtonText
this.$showAllText.textContent = newButtonText

// Swap icon, toggle class
if (expanded) {
Expand Down
2 changes: 1 addition & 1 deletion packages/govuk-frontend/src/govuk/components/accordion/accordion.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ describe('/components/accordion', () => {
})
).rejects.toEqual({
name: 'ElementError',
message: 'Accordion: $module is not an instance of "HTMLElement"'
message: 'Accordion: $module is not an instance of HTMLElement'
})
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/govuk-frontend/src/govuk/components/button/button.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ describe('/components/button', () => {
})
).rejects.toEqual({
name: 'ElementError',
message: 'Button: $module is not an instance of "HTMLElement"'
message: 'Button: $module is not an instance of HTMLElement'
})
})
})
Expand Down
47 changes: 24 additions & 23 deletions packages/govuk-frontend/src/govuk/components/character-count/character-count.jsdom.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,31 @@
import { getExamples, renderComponent } from '@govuk-frontend/lib/components'

import { CharacterCount } from './character-count.mjs'

describe('CharacterCount', () => {
let $container
let $textarea

beforeAll(() => {
$container = document.createElement('div')
$textarea = document.createElement('textarea')

// Component checks that GOV.UK Frontend is supported
document.body.classList.add('govuk-frontend-supported')
let html

beforeAll(async () => {
const examples = await getExamples('character-count')
html = renderComponent(
'character-count',
examples['to configure in JavaScript']
)
})

// Component checks that required elements are present
$textarea.classList.add('govuk-js-character-count')
$container.appendChild($textarea)
beforeEach(async () => {
// Some tests add attributes to `document.body` so we need
// to reset it alongside the component's markup
document.body.outerHTML = `<body class="govuk-frontend-supported">${html}</body>`
})

describe('formatCountMessage', () => {
describe('default configuration', () => {
let componentWithMaxLength
let componentWithMaxWords

beforeAll(() => {
const $div = $container.cloneNode(true)
beforeEach(() => {
const $div = document.querySelector('[data-module]')
componentWithMaxLength = new CharacterCount($div, { maxlength: 100 })
componentWithMaxWords = new CharacterCount($div, { maxwords: 100 })
})
Expand Down Expand Up @@ -87,7 +90,7 @@ describe('CharacterCount', () => {
describe('i18n', () => {
describe('JavaScript configuration', () => {
it('overrides the default translation keys', () => {
const $div = $container.cloneNode(true)
const $div = document.querySelector('[data-module]')
const component = new CharacterCount($div, {
maxlength: 100,
i18n: {
Expand All @@ -108,7 +111,7 @@ describe('CharacterCount', () => {
})

it('uses specific keys for when limit is reached', () => {
const $div = $container.cloneNode(true)
const $div = document.querySelector('[data-module]')
const componentWithMaxLength = new CharacterCount($div, {
maxlength: 100,
i18n: {
Expand Down Expand Up @@ -136,7 +139,7 @@ describe('CharacterCount', () => {

describe('lang attribute configuration', () => {
it('overrides the locale when set on the element', () => {
const $div = $container.cloneNode(true)
const $div = document.querySelector('[data-module]')
$div.setAttribute('lang', 'de')

const component = new CharacterCount($div, { maxwords: 20000 })
Expand All @@ -148,11 +151,9 @@ describe('CharacterCount', () => {
})

it('overrides the locale when set on an ancestor', () => {
const $parent = document.createElement('div')
$parent.setAttribute('lang', 'de')
document.body.setAttribute('lang', 'de')

const $div = $container.cloneNode(true)
$parent.appendChild($div)
const $div = document.querySelector('[data-module]')

const component = new CharacterCount($div, { maxwords: 20000 })

Expand All @@ -165,7 +166,7 @@ describe('CharacterCount', () => {

describe('Data attribute configuration', () => {
it('overrides the default translation keys', () => {
const $div = $container.cloneNode(true)
const $div = document.querySelector('[data-module]')
$div.setAttribute(
'data-i18n.characters-under-limit.one',
'Custom text. Count: %{count}'
Expand All @@ -187,7 +188,7 @@ describe('CharacterCount', () => {

describe('precedence over JavaScript configuration', () => {
it('overrides translation keys', () => {
const $div = $container.cloneNode(true)
const $div = document.querySelector('[data-module]')
$div.setAttribute(
'data-i18n.characters-under-limit.one',
'Custom text. Count: %{count}'
Expand Down
24 changes: 15 additions & 9 deletions packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ export class CharacterCount extends GOVUKFrontendComponent {
$textarea instanceof HTMLInputElement
)
) {
return this
throw new ElementError($textarea, {
componentName: 'Character count',
identifier: '.govuk-js-character-count',
Copy link
Contributor

Choose a reason for hiding this comment

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

With (potentially) multiple textarea components on the page, how much do we worry about scoping selectors against their module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Linking with #4263 (review)

For these "module child" selectors it could be brilliant to show a scope that the browser can inspect

Suggested change
identifier: '.govuk-js-character-count',
identifier: '.govuk-js-character-count',
scope: $module,

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 Yeah, that's probably the component most likely to be on a page multiple times. We recommend to start with a single question per page so that limits the risks. It doesn't completely avoids the possibility of multiple of those on the page (especially if we consider the radios/checkboxes potentially revealing fields). That said, there shouldn't be too many of them on a given page that it's tricky for developers to find back the one that's wonky (either by looking carefully at the page or breaking on error in the devtools). I would say it's not something we should not worry too hard about at the start. We can always dig deeper if we receive feedback.

If we have a simple solution, though, not against adding extra information, but it might be tricky to add that consistently. The element with data-module does not really have anything identifiable usually. Here, the $textarea could provide an id (but only if it is on the page so nothing if it doesn't exist).

Copy link
Member Author

@romaricpascal romaricpascal Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Whoops, there wasn't your second message when I started writing mine, sorry. How would users access the $module you propose passing as $scope (it's fine if that part still needs looking into)? Do browsers log extra fields from the errors? the cause maybe, in browsers that support it (that'd be neat)?

Copy link
Contributor

Choose a reason for hiding this comment

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

One to investigate. Would be amazing if you could hover over scope and it highlighted the element

Can leave them off for now if we don't need it

Copy link
Member Author

Choose a reason for hiding this comment

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

That'd speed debugging indeed, but let's see if people come back needing more pointers to where things happen as I think that'd need a good bit of investigation 😊 Had a quick glance at cause, Firefox logs it (only if the element itself, if an object, you get Object {...}, not expandable), not Chrome, nor Safari (at the time of this message). We could always override toString to log before the message gets computed for display in the console, but seems quite hacky anyway 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, not the clearest blurb. I'll (try to) clarify:

  • I'd prefer leaving it off until we have the need for it as it'd require more investigation to make it work
  • Got curious and gave a quick check at where Error's cause are logged (no luck there) and tried a quick hack by overriding toString to log before the error message gets computed (can log the element, but a bit hacky).

Copy link
Contributor

Choose a reason for hiding this comment

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

No don't worry, the confused face was wondering why cause wasn't logged

One for another day (and don't want to be Chrome specific) but some nice ideas in:
https://developer.chrome.com/docs/devtools/console/log/

expectedType: 'HTMLTextareaElement or HTMLInputElement'
})
}

// Read config set using dataset ('data-' values)
Expand Down Expand Up @@ -133,18 +137,20 @@ export class CharacterCount extends GOVUKFrontendComponent {
this.$module = $module
this.$textarea = $textarea

const $textareaDescription = document.getElementById(
`${this.$textarea.id}-info`
)
const textareaDescriptionId = `${this.$textarea.id}-info`
const $textareaDescription = document.getElementById(textareaDescriptionId)
if (!$textareaDescription) {
return
throw new ElementError($textareaDescription, {
componentName: 'Character count',
identifier: `#${textareaDescriptionId}`
})
}

// Inject a description for the textarea if none is present already
// for when the component was rendered with no maxlength, maxwords
// nor custom textareaDescriptionText
if ($textareaDescription.innerText.match(/^\s*$/)) {
$textareaDescription.innerText = this.i18n.t('textareaDescription', {
if ($textareaDescription.textContent.match(/^\s*$/)) {
$textareaDescription.textContent = this.i18n.t('textareaDescription', {
count: this.maxLength
})
}
Expand Down Expand Up @@ -318,7 +324,7 @@ export class CharacterCount extends GOVUKFrontendComponent {
}

// Update message
this.$visibleCountMessage.innerText = this.getCountMessage()
this.$visibleCountMessage.textContent = this.getCountMessage()
}

/**
Expand All @@ -336,7 +342,7 @@ export class CharacterCount extends GOVUKFrontendComponent {
}

// Update message
this.$screenReaderCountMessage.innerText = this.getCountMessage()
this.$screenReaderCountMessage.textContent = this.getCountMessage()
}

/**
Expand Down
46 changes: 45 additions & 1 deletion ...ges/govuk-frontend/src/govuk/components/character-count/character-count.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,10 +808,54 @@ describe('Character count', () => {
$module.outerHTML = `<svg data-module="govuk-character-count"></svg>`
}
})
).rejects.toEqual({
name: 'ElementError',
message: 'Character count: $module is not an instance of HTMLElement'
})
})

it('throws when the textarea is missing', async () => {
await expect(
renderAndInitialise(page, 'character-count', {
params: examples.default,
beforeInitialisation($module) {
$module.querySelector('.govuk-js-character-count').remove()
}
})
).rejects.toEqual({
name: 'ElementError',
message: 'Character count: .govuk-js-character-count not found'
})
})

it('throws when the textarea is not the right type', async () => {
await expect(
renderAndInitialise(page, 'character-count', {
params: examples.default,
beforeInitialisation($module) {
// Replace with a tag that's neither an `<input>` or `<textarea>`
$module.querySelector('.govuk-js-character-count').outerHTML =
'<div class="govuk-js-character-count"></div>'
}
})
).rejects.toEqual({
name: 'ElementError',
message:
'Character count: $module is not an instance of "HTMLElement"'
'Character count: .govuk-js-character-count is not of type HTMLTextareaElement or HTMLInputElement'
Copy link
Contributor

Choose a reason for hiding this comment

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

It works 🙌

})
})

it('throws when the textarea description is missing', async () => {
await expect(
renderAndInitialise(page, 'character-count', {
params: examples.default,
beforeInitialisation($module) {
$module.querySelector('#more-detail-info').remove()
}
})
).rejects.toEqual({
name: 'ElementError',
message: 'Character count: #more-detail-info not found'
})
})

Expand Down
2 changes: 1 addition & 1 deletion packages/govuk-frontend/src/govuk/components/checkboxes/checkboxes.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ describe('Checkboxes with multiple groups and a "None" checkbox and conditional
).rejects.toEqual({
name: 'ElementError',
message:
'Checkboxes: [data-module="govuk-checkboxes"] is not an instance of "HTMLElement"'
'Checkboxes: [data-module="govuk-checkboxes"] is not an instance of HTMLElement'
})
})

Expand Down
2 changes: 1 addition & 1 deletion packages/govuk-frontend/src/govuk/components/error-summary/error-summary.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ describe('Error Summary', () => {
})
).rejects.toEqual({
name: 'ElementError',
message: 'Error summary: $module is not an instance of "HTMLElement"'
message: 'Error summary: $module is not an instance of HTMLElement'
})
})
})
Expand Down
14 changes: 7 additions & 7 deletions packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export class ExitThisPage extends GOVUKFrontendComponent {
* @private
*/
exitPage() {
this.$updateSpan.innerText = ''
this.$updateSpan.textContent = ''

// Blank the page
// As well as creating an overlay with text, we also set the body to hidden
Expand All @@ -238,7 +238,7 @@ export class ExitThisPage extends GOVUKFrontendComponent {
// the element text after adding it means that screen readers pick up the
// announcement more reliably.
document.body.appendChild(this.$overlay)
this.$overlay.innerText = this.i18n.t('activated')
this.$overlay.textContent = this.i18n.t('activated')

window.location.href = this.$button.getAttribute('href')
}
Expand Down Expand Up @@ -301,9 +301,9 @@ export class ExitThisPage extends GOVUKFrontendComponent {
this.exitPage()
} else {
if (this.keypressCounter === 1) {
this.$updateSpan.innerText = this.i18n.t('pressTwoMoreTimes')
this.$updateSpan.textContent = this.i18n.t('pressTwoMoreTimes')
} else {
this.$updateSpan.innerText = this.i18n.t('pressOneMoreTime')
this.$updateSpan.textContent = this.i18n.t('pressOneMoreTime')
}
}

Expand Down Expand Up @@ -350,10 +350,10 @@ export class ExitThisPage extends GOVUKFrontendComponent {
this.keypressTimeoutId = null

this.keypressCounter = 0
this.$updateSpan.innerText = this.i18n.t('timedOut')
this.$updateSpan.textContent = this.i18n.t('timedOut')

this.timeoutMessageId = window.setTimeout(() => {
this.$updateSpan.innerText = ''
this.$updateSpan.textContent = ''
}, this.timeoutTime)

this.updateIndicator()
Expand Down Expand Up @@ -384,7 +384,7 @@ export class ExitThisPage extends GOVUKFrontendComponent {

// Ensure the announcement span's role is status, not alert and clear any text
this.$updateSpan.setAttribute('role', 'status')
this.$updateSpan.innerText = ''
this.$updateSpan.textContent = ''

// Sync the keypress indicator lights
this.updateIndicator()
Expand Down
2 changes: 1 addition & 1 deletion packages/govuk-frontend/src/govuk/components/exit-this-page/exit-this-page.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ describe('/components/exit-this-page', () => {
})
).rejects.toEqual({
name: 'ElementError',
message: 'Exit this page: $module is not an instance of "HTMLElement"'
message: 'Exit this page: $module is not an instance of HTMLElement'
})
})

Expand Down
4 changes: 2 additions & 2 deletions packages/govuk-frontend/src/govuk/components/header/header.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ describe('Header navigation', () => {
})
).rejects.toEqual({
name: 'ElementError',
message: 'Header: $module is not an instance of "HTMLElement"'
message: 'Header: $module is not an instance of HTMLElement'
})
})

Expand Down Expand Up @@ -244,7 +244,7 @@ describe('Header navigation', () => {
).rejects.toEqual({
name: 'ElementError',
message:
'Header: .govuk-js-header-toggle is not an instance of "HTMLElement"'
'Header: .govuk-js-header-toggle is not an instance of HTMLElement'
})
})

Expand Down
2 changes: 1 addition & 1 deletion ...k-frontend/src/govuk/components/notification-banner/notification-banner.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ describe('Notification banner', () => {
).rejects.toEqual({
name: 'ElementError',
message:
'Notification banner: $module is not an instance of "HTMLElement"'
'Notification banner: $module is not an instance of HTMLElement'
})
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/govuk-frontend/src/govuk/components/radios/radios.puppeteer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ describe('Radios', () => {
).rejects.toEqual({
name: 'ElementError',
message:
'Radios: [data-module="govuk-radios"] is not an instance of "HTMLElement"'
'Radios: [data-module="govuk-radios"] is not an instance of HTMLElement'
})
})

Expand Down
Loading