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.

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

avoid iterating a live nodelist #588

Merged
merged 2 commits into from
Mar 22, 2024
Merged

avoid iterating a live nodelist #588

merged 2 commits into from
Mar 22, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Mar 22, 2024

This step is still surprisingly slow but this makes it take 5 seconds instead of 10 on my machine.

@bakkot bakkot merged commit 3ddc70b into main Mar 22, 2024
2 checks passed
@bakkot bakkot deleted the avoid-live-nodelist branch March 22, 2024 23:30
@gibson042
Copy link
Contributor

It occurs to me that even more time could be saved by skipping this step entirely and instead updating the print CSS to use relevant attribute-value-starts-with selectors:

+/* Render non-mailto external link targets, cf. https://url.spec.whatwg.org/#special-scheme */
-a[data-print-href]::after {
+a:is([href^='ftp:' i], [href^='sftp:' i], [href^='http:' i], [href^='https:' i], [href^='ws:' i], [href^='wss:' i])::after {
   content: ' <' attr(href) '>';
   color: initial;
 }

This would miss new schemes until added to the CSS (which is lintable) and would be redundant for links whose text matches their target (which we should probably not have anyway—it's more accessible to name links and let user agents handle URIs).

@ljharb
Copy link
Member

ljharb commented Mar 23, 2024

also if you're going for speed, you should avoid for..of aggressively.

@michaelficarra
Copy link
Member

and would be redundant for links whose text matches their target (which we should probably not have anyway—it's more accessible to name links and let user agents handle URIs)

Yes but we do and that's why we can't use this CSS-only strategy.

also if you're going for speed, you should avoid for..of aggressively.

The for-of is only iterating 94 elements. It's not the slow part. The slow part, for some unexplainable reason, is setting an attribute.

@bakkot
Copy link
Contributor Author

bakkot commented Mar 25, 2024

The for-of is only iterating 94 elements.

(Well, no, it's iterating 30k elements, but it's still true that the iteration itself is now ~instant.)

@gibson042
Copy link
Contributor

and would be redundant for links whose text matches their target (which we should probably not have anyway—it's more accessible to name links and let user agents handle URIs)

Yes but we do and that's why we can't use this CSS-only strategy.

Having them doesn't preclude a CSS-only approach, it just means that such an approach would duplicate URLs. But to address the larger point, would you object to fixing such links by giving them non-URL text?

@michaelficarra
Copy link
Member

@gibson042 You're always welcome to send a PR to 262, but (without looking) I doubt you'll completely eliminate them.

gibson042 added a commit to gibson042/ecma262 that referenced this pull request Mar 26, 2024
To be used for suppressing automatic insertion of targets when printing,
cf. tc39/ecmarkup#578 and
tc39/ecmarkup#588 (comment)
@gibson042
Copy link
Contributor

ecma262 PR: tc39/ecma262#3301

gibson042 added a commit to gibson042/ecma262 that referenced this pull request Mar 26, 2024
To be used for suppressing automatic insertion of targets when printing,
cf. tc39/ecmarkup#578 and
tc39/ecmarkup#588 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants