-
Notifications
You must be signed in to change notification settings - Fork 59
fix(utils): replace randomIdString() with deterministic createElementId()
#936
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
411203c
8bdc299
c035664
b72e907
82e1a6f
17c1e1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { LitElement, html } from "lit"; | ||
|
|
||
| import { randomIdString } from "../utils/index.js"; | ||
| import { createElementId } from "../utils/index.js"; | ||
|
|
||
| import styles from "./element.css?lit"; | ||
|
|
||
|
|
@@ -13,6 +13,7 @@ import styles from "./element.css?lit"; | |
| * | ||
| * @element mdn-dropdown | ||
| * | ||
| * @attr {string} name - Unique name used to generate a stable ID for the dropdown slot element (for aria-controls). Required when the dropdown slot element has no explicit id. | ||
| * @attr {boolean} open - Whether the dropdown is open or not. | ||
| * | ||
| * @slot button - The element used to toggle the dropdown. | ||
|
|
@@ -22,12 +23,14 @@ export class MDNDropdown extends LitElement { | |
| static styles = styles; | ||
|
|
||
| static properties = { | ||
| name: { type: String, required: true }, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| open: { type: Boolean }, | ||
| loaded: { type: Boolean, reflect: true }, | ||
| }; | ||
|
|
||
| constructor() { | ||
| super(); | ||
| this.name = ""; | ||
| this.open = false; | ||
| this.loaded = false; | ||
| } | ||
|
|
@@ -70,7 +73,7 @@ export class MDNDropdown extends LitElement { | |
| _setAriaAttributes() { | ||
| let id = this._dropdownSlotElements.find((element) => element.id)?.id; | ||
| if (!id) { | ||
| id = randomIdString("uid_"); | ||
| id = createElementId("dropdown", this.name); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems unnecessary, why not construct the string directly? |
||
| this._dropdownSlotElements[0]?.setAttribute("id", id); | ||
| } | ||
| for (const element of this._buttonSlotElements) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,31 @@ | ||
| /** | ||
| * Used to generate a random element id by combining a prefix with a random string. | ||
| * Simple FNV-1a hash implementation for generating deterministic IDs. | ||
| * @param {string} str - String to hash | ||
| * @returns {string} - Hexadecimal hash string | ||
| */ | ||
| function hashString(str) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what hashing gives us here: if the input isn't unique, the output won't be either - this seems unnecessary |
||
| let hash = 2_166_136_261; // FNV offset basis | ||
| for (let i = 0; i < str.length; i++) { | ||
| // eslint-disable-next-line unicorn/prefer-code-point -- charCodeAt is sufficient for hash | ||
| hash ^= str.charCodeAt(i); | ||
| hash = Math.imul(hash, 16_777_619); // FNV prime | ||
| } | ||
| // Convert to unsigned 32-bit and then to hex | ||
| return (hash >>> 0).toString(36); | ||
| } | ||
|
|
||
| /** | ||
| * Used to generate a deterministic element id by hashing the provided content. | ||
| * Falls back to random generation if no content is provided (for backwards compatibility). | ||
| * | ||
| * @param {string} prefix | ||
| * @param {string} prefix - Prefix for the ID | ||
| * @param {string} content - Content to hash for ID generation | ||
| * @returns {string} | ||
| */ | ||
| export function randomIdString(prefix = "id-") { | ||
| return Math.random().toString(36).replace("0.", prefix); | ||
| export function createElementId(prefix, content) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function itself seems unnecessary: where we can set a static, unique ID in call sites, let's do that instead of routing it through here. |
||
| if (!content) { | ||
| // Fallback to random for backwards compatibility when no content provided | ||
| return Math.random().toString(36).replace("0.", prefix); | ||
| } | ||
| return `${prefix}-${hashString(String(content))}`; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely to cause collisions: we can realistically have two buttons with the same href on the same page.
We only need this ID to set
aria-labelledbyfor icon-only buttons. Perhaps we setaria-labelfor icon-only buttons instead, and enforce thatlabelis a string with types wheniconOnlyis true?