fix: file-a-bug link, skip dfn in del, latestVersion IRI display, highlight textContent#5208
Open
marcoscaceres wants to merge 10 commits intomainfrom
Open
fix: file-a-bug link, skip dfn in del, latestVersion IRI display, highlight textContent#5208marcoscaceres wants to merge 10 commits intomainfrom
marcoscaceres wants to merge 10 commits intomainfrom
Conversation
3 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes several UX and correctness issues across ReSpec’s GitHub integration, definition registration, W3C header rendering, and code highlighting.
Changes:
- Update “File a Bug” to point to GitHub’s issue creation chooser (
/issues/new/choose) and exposenewIssuesURLon the normalized GitHub config. - Skip registering
<dfn>elements that are descendants of<del>so deleted content doesn’t create active definitions. - Improve display of
latestVersionin W3C headers by showing a decoded IRI as link text while keeping the encoded URL inhref. - Fix highlighting within closed
<details>by sendingtextContentto the highlighter instead ofinnerText.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/github.js |
Switches “File a Bug” link target to /issues/new/choose and hoists newIssuesURL for reuse. |
tests/spec/core/github-spec.js |
Adds/updates assertions for newIssuesURL and the updated “File a Bug” link. |
src/core/dfn.js |
Skips <dfn> processing when inside a <del>. |
tests/spec/core/dfn-spec.js |
Adds a test to verify <dfn> inside <del> is not processed. |
src/w3c/templates/headers.js |
Displays decoded latestVersion as anchor text while keeping encoded URL in href. |
tests/spec/w3c/headers-spec.js |
Adds a test for decoded-I RI display behavior in the header. |
src/core/highlight.js |
Uses textContent instead of innerText when sending code to the highlighter. |
tests/spec/core/highlight-spec.js |
Adds a test ensuring highlighting works inside a closed <details>. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/core/highlight.js:37
- If
sendHighlightRequest()rejects (e.g., timeout), the function returns from thecatchwithout clearingaria-busy, leaving the element in a permanently busy state for assistive tech. Consider ensuringaria-busyis reset in afinallyblock (or explicitly in thecatch) before returning.
htmlElem.setAttribute("aria-busy", "true");
const languages = getLanguageHint(htmlElem.classList);
let response;
try {
response = await sendHighlightRequest(htmlElem.textContent, languages);
} catch (err) {
console.error(err);
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
Author
|
@copilot can you take a look at CI and fix the issue? |
Contributor
Fixed in d952bda. I checked the failing Node CI run (24629133182), pulled the failing job logs, and fixed the CI break by stabilizing the brittle assertion in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
Author
|
@copilot rebase |
Contributor
Done in 234dc3c — branch is updated with latest Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/core/highlight.js:37
- If sendHighlightRequest() throws or times out, highlightElement() returns early while leaving
aria-busy="true"on the element. This can cause assistive tech to treat the element as perpetually busy after a highlight failure. Consider resetting/removingaria-busyin afinallyblock (or before returning in the catch) so it’s cleared on both success and failure paths.
htmlElem.setAttribute("aria-busy", "true");
const languages = getLanguageHint(htmlElem.classList);
let response;
try {
response = await sendHighlightRequest(htmlElem.textContent, languages);
} catch (err) {
console.error(err);
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
@copilot Can you confirm all your feedback has been addressed? Can you fix anything that was not addressed? |
Contributor
Confirmed and addressed in fc52f69: the remaining gap was the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…ght textContent - fix(core/github): "File a Bug" link now points to /issues/new/choose instead of the issue list; hoist newIssuesURL before otherLink to avoid duplicating the URL construction in normalizedGHObj - fix(core/dfn): skip <dfn> elements that are descendants of <del> (deleted content in updatable-rec specs) - fix(w3c/templates/headers): latestVersion link displays human-readable IRI via decodeURIComponent() while keeping encoded URL in href - fix(core/highlight): use textContent instead of innerText so <pre> inside closed <details> elements is still highlighted - tests: add/update tests for all four fixes
decodeURIComponent() is for URL components and throws on malformed input; decodeURI() is correct for full URLs. The deleted-dfn test was pre-setting id="deleted" on the <dfn> inside <del>, defeating regression coverage: if ReSpec ever started auto-assigning IDs to deleted dfns, the test would still pass because the element would be found by its hardcoded id. Remove the pre-set id and assert that no id was generated.
link.href returns the browser-resolved URL which may percent-encode
a trailing bare %. getAttribute('href') returns the raw attribute.
- headers: use getAttribute('href') instead of link.href for the valid
IRI test (same fix already applied to malformed URL test)
- highlight: assert details element is closed before checking that
highlighting worked (verifies the regression case)
marcoscaceres
force-pushed
the
fix/batch-eq-quick
branch
from
fc52f69 to
bdb82ed
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
builds/ should not be committed — CI regenerates them on release.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/issues/new/chooseinstead of the issue list.newIssuesURLis hoisted to avoid duplicating thenew URL()call innormalizedGHObj.<dfn>elements that are descendants of<del>are now skipped during definition registration (deleted content in updatable-rec specs, closes Updatable recs: ignore duplicate definitions when a definition is sitting inside a <del> #4721).latestVersionlink displays the human-readable IRI viadecodeURI()(wrapped insafeDecodeURI()for malformed input) as link text while thehrefattribute keeps the encoded URL (closeslatestVersionURLencodes IRIs making them unreadable by people #4910).textContentinstead ofinnerTextso<pre>elements inside a closed<details>are still highlighted (closes Highlight.js and <details> #4695).Test plan
pnpm start --browser ChromeHeadless --grep="Core - GitHub"pnpm start --browser ChromeHeadless --grep="Core - Definitions"pnpm start --browser ChromeHeadless --grep="W3C - Headers"pnpm start --browser ChromeHeadless --grep="Core - Highlight"pnpm start --browser FirefoxHeadless