fix(utils): replace randomIdString() with deterministic createElementId()#936
fix(utils): replace randomIdString() with deterministic createElementId()#936
randomIdString() with deterministic createElementId()#936Conversation
|
c2d9952 was deployed to: https://fred-pr936.review.mdn.allizom.net/ |
caugner
changed the title
randomIdString() with deterministicIdString()randomIdString() with deterministic createElementId()
LeoMcA
left a comment
LeoMcA
left a comment
There was a problem hiding this comment.
I think this fixes everything in the wrong place:
We're sacrificing the guarantee of ID-uniqueness in a number of places, which would result in invalid HTML, and a confused accessibility tree, for an issue which IIRC we already have a fix for: just upload everything on each build, no rsync/diff - that's the slow bit.
Regardless, having deterministic output is a good goal, but I don't think this is at all the correct approach: the new function does very little that manually adding ids would do, and the addition of name in dropdowns isn't enforced, nor is necessary - we can just add ids to the slotted elements directly in the DOM.
I'd suggest we keep the random ID fallback where we can't be sure that manual IDs have been set, and enforce this deterministic requirement in CI so we catch future fallbacks to the random ID and fix.
| let id = this._dropdownSlotElements.find((element) => element.id)?.id; | ||
| if (!id) { | ||
| id = randomIdString("uid_"); | ||
| id = createElementId("dropdown", this.name); |
There was a problem hiding this comment.
This seems unnecessary, why not construct the string directly?
| * @param {string} str - String to hash | ||
| * @returns {string} - Hexadecimal hash string | ||
| */ | ||
| function hashString(str) { |
There was a problem hiding this comment.
Not sure what hashing gives us here: if the input isn't unique, the output won't be either - this seems unnecessary
| */ | ||
| export function randomIdString(prefix = "id-") { | ||
| return Math.random().toString(36).replace("0.", prefix); | ||
| export function createElementId(prefix, content) { |
There was a problem hiding this comment.
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.
| static styles = styles; | ||
|
|
||
| static properties = { | ||
| name: { type: String, required: true }, |
There was a problem hiding this comment.
required: true doesn't do anything
| const labelId = randomIdString("label-"); | ||
| const labelId = createElementId( | ||
| "label", | ||
| `${typeof label === "string" ? label : "btn"}-${href || ""}`, |
There was a problem hiding this comment.
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-labelledby for icon-only buttons. Perhaps we set aria-label for icon-only buttons instead, and enforce that label is a string with types when iconOnly is true?
3 participants
Description
Remove
randomIdString()in favor of a deterministiccreateElementId()function.Motivation
Increase chances of HTML files being identical on subsequent builds, speeding up the sync part of the deployment.
Additional details
Tested locally with the following script:
Only the homepage and the Playground had differences:
Homepage differences:
Related issues and pull requests
Fixes #913.