Skip to content

Commit

Permalink
Change id validation on component wrapper
Browse files Browse the repository at this point in the history
- previous validation was too strict, HTML5 valid characters in id attributes are almost anything
- validation now only complains if an id contains spaces, newlines or `.` characters
- `.` is technically allowed, but can lead to problems in testing as an id of `#id.stillid` could be mistakenly matched to an element with an id of `#id` and a class of `stillid`
  • Loading branch information
andysellick committed Jan 23, 2025
1 parent 8e55b13 commit e91fa9d
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 5 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
useful summary for people upgrading their application, not a replication
of the commit log.

## Unreleased

* Change id validation on component wrapper ([PR #4584](https://github.com/alphagov/govuk_publishing_components/pull/4584))

## 50.0.0

* Remove margin top from search component ([PR #4581](https://github.com/alphagov/govuk_publishing_components/pull/4581))
Expand Down
2 changes: 1 addition & 1 deletion docs/component-wrapper-helper.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ These options can be passed to any component that uses the component wrapper.
To prevent breaking [component isolation](https://github.com/alphagov/govuk_publishing_components/blob/main/docs/component_principles.md#a-component-is-isolated-when), passed classes should only be used for JavaScript hooks and not styling. All component styling should be included only in the component itself. Any passed classes should be prefixed with `js-`. To allow for extending this option, classes prefixed with `gem-c-`, `govuk-`, `app-c-`, `brand--`, or `brand__` are also permitted, as well as an exact match of `direction-rtl`, but these classes should only be used within the component and not passed to it.
The helper checks that any passed `id` attribute is valid, specifically that it does not start with a number or contain whitespace or contain any characters other than letters, numbers, and `_` or `-`. It also checks that role and lang attribute values are valid, along with some other checks detailed below.
The helper checks that any passed `id` attribute is valid, specifically that it does not contain whitespace or `.` characters. It also checks that role and lang attribute values are valid, along with some other checks detailed below.
An example of passing data to a component with the component wrapper:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ def set_margin_bottom(margin_bottom)
def check_id_is_valid(id)
return if id.blank?

raise(ArgumentError, "Id (#{id}) cannot start with a number or contain whitespace and can only contain letters, digits, `_` and `-`") unless /\A[a-zA-Z][\w:-]*\z/.match?(id)
raise(ArgumentError, "Id (#{id}) cannot contain whitespace or `.` characters") if /[. \n]+/.match?(id)
end

def check_data_attributes_are_valid(attributes)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,16 @@

describe "setting an id" do
it "does not accept invalid ids" do
["1dstartingwithnumber", "id with spaces", "idwith.dot", "id\nwithnewline"].each do |id|
["id with spaces", "idwith.dot", "id\nwithnewline"].each do |id|
expect {
GovukPublishingComponents::Presenters::ComponentWrapperHelper.new(id:)
}.to raise_error(ArgumentError, / contain/)
end
end

it "accepts a valid id" do
helper = GovukPublishingComponents::Presenters::ComponentWrapperHelper.new(id: "valid")
expect(helper.all_attributes[:id]).to eql("valid")
helper = GovukPublishingComponents::Presenters::ComponentWrapperHelper.new(id: "valid[id]_attribute-value")
expect(helper.all_attributes[:id]).to eql("valid[id]_attribute-value")
end

it "can set an id, overriding a passed value" do
Expand Down

0 comments on commit e91fa9d

Please sign in to comment.