fix(utils): resolve @import url against the stylesheet that contains it - #5269
fix(utils): resolve @import url against the stylesheet that contains it#5269jsakamoto wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes CSSOM preloading of @import stylesheets by resolving CSSImportRule.href relative URLs against the containing stylesheet URL (sheet.href) instead of the document URL, preventing incorrect XHR requests (404s) when stylesheets live in different directories. It also updates the cross-origin flag computation to compare URL origins after resolution and adds unit tests to cover both stylesheet-based resolution and the inline-style (sheet.href === null) fallback.
Changes:
- Resolve
@importURLs usingnew URL(rule.href, sheet.href || document.baseURI).hrefbefore requesting imported stylesheets. - Recompute
isCrossOriginRequestby comparing the resolved import URL origin to the document origin. - Add unit tests verifying resolution against stylesheet URL and document URL fallback for inline styles.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/core/utils/parse-sameorigin-stylesheet.js | Resolves @import hrefs against the correct base and adjusts cross-origin detection based on resolved URL origins. |
| test/core/utils/parse-sameorigin-stylesheet.js | Adds unit tests for stylesheet-relative and document-relative @import URL resolution. |
Comments suppressed due to low confidence (2)
lib/core/utils/parse-sameorigin-stylesheet.js:72
- Cross-origin detection uses
new URL(importUrl).originandwindow.location.originwithout any compatibility fallback. In IE11,URLis undefined andlocation.originmay be missing, so this will either throw or misclassify same-origin imports. Consider computing both origins with an IE11-safe fallback (e.g.,<a>parsing andprotocol//host).
// `importUrl` is resolved, so the origin tells a cross-origin request
// apart from a same-origin one
const isCrossOriginRequest =
new URL(importUrl).origin !== window.location.origin;
test/core/utils/parse-sameorigin-stylesheet.js:180
- This assertion uses
new URL(..., document.baseURI), which will throw in IE11. Since the base is the current document, an<a>element can be used to resolve the URL without depending on the WHATWGURLAPI.
assert.deepEqual(importedUrls, [
new URL(
'../integration/full/preload-cssom/base.css',
document.baseURI
).href
]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const cssomDir = '/test/integration/full/preload-cssom/'; | ||
| // only `href` and `cssRules` are used by `parseSameOriginStylesheet`. | ||
| // the sheet is in another directory than the document, so resolving the | ||
| // `@import` against the document url would request the wrong file. | ||
| const sheet = { | ||
| href: new URL(cssomDir + 'style-with-import.css', location.origin).href, | ||
| cssRules: [{ type: 3, href: 'base.css' }] | ||
| }; | ||
| const options = { | ||
| rootNode: document, | ||
| shadowId: undefined, | ||
| convertDataToStylesheet: convertDataToStylesheet | ||
| }; | ||
|
|
||
| const importedUrls = []; | ||
| axe.utils | ||
| .parseSameOriginStylesheet(sheet, options, [1, 0], importedUrls, false) | ||
| .then(data => { | ||
| assert.deepEqual(importedUrls, [ | ||
| new URL(cssomDir + 'base.css', location.origin).href | ||
| ]); |
There was a problem hiding this comment.
The unit tests only run in Chrome and Firefox, so I left new URL(...) in the test file as it is. Please tell me if you would like the tests to be able to run in IE11 as well, and I will rewrite them with protocol//host.
| // would resolve it against the url of the document instead. | ||
| // `sheet.href` is null for an inline `<style>` element, in which case the | ||
| // url of the document is the correct base. | ||
| .map(rule => new URL(rule.href, sheet.href || document.baseURI).href) |
There was a problem hiding this comment.
Good catch, thank you. You are right that URL is not available in IE11, and I confirmed that this file works there today, because Array.from and Array.prototype.includes are both handled in lib/core/imports/polyfills.js. So this change would have broken the CSSOM preload in IE11, which is the very thing the fix is about.
I moved both steps into small helpers, resolveImportUrl and isCrossOriginUrl. Each one falls back to the old behavior when the URL constructor is not available, so in IE11 the url is used as it was written and the cross-origin test stays a pattern match on the string. The two go together, because the pattern match is the correct test as long as the url is not resolved. A test with window.URL replaced by a non constructor covers that path.
I chose a fallback over a polyfill to keep the bundle size as it is. If you would rather fix IE11 as well, core-js-pure/actual/url would fit the pattern in polyfills.js, and I am happy to change it.
`CSSImportRule.href` returns the url as it is written in the stylesheet, so it is usually relative. CSS resolves such a url against the url of the stylesheet that contains the `@import`, but `XMLHttpRequest` resolves it against the url of the document. When a stylesheet is not next to the document, axe-core requested a file that does not exist. The CSSOM preload then either rejected, or parsed the error page of the 404 response as CSS. Neither result reaches the caller, so `css-orientation-lock` quietly returned a wrong result. Resolve the url before the request. Since the url is now absolute, the cross-origin test can no longer be a pattern match on the string, so compare the origin instead. That keeps the flag the same as before for both same-origin and cross-origin imports. IE11 has no URL constructor. There, both steps fall back to what they did before this change, so the CSSOM preload keeps working as it used to. Closes issue dequelabs#5268
c4f3b56 to
73b278b
Compare
CSSImportRule.hrefreturns the url as it is written in the stylesheet, so it is usually relative. CSS resolves such a url against the url of the stylesheet that contains the@import, whileXMLHttpRequestresolves it against the url of the document. When a stylesheet is not next to the document, the CSSOM preload requested a file that does not exist.This PR resolves the url against
sheet.hrefbefore the request, and falls back todocument.baseURIfor an inline<style>element, wheresheet.hrefis null.One more change was needed. Since the url is now absolute, the cross-origin test cannot be a pattern match on the string any more, because it would report every same-origin
@importas cross-origin. The origin of the resolved url is compared instead, so the flag keeps the same value as before for both same-origin and cross-origin imports.Tests
Two unit tests are added to
test/core/utils/parse-sameorigin-stylesheet.js, one for the stylesheet url as the base and one for the document url as the fallback. The first one fails without this change.I also ran the unit tests under
test/core, the full integration tests in Chrome (includingtest/integration/full/preload-cssom), andpnpm run test:tsc. All of them pass.Follow up
A relative
@importinside a stylesheet that was fetched overXMLHttpRequestis still resolved against the url of the document. The sheet for the fetched text is created from a<style>element, so itshrefis null and the url of the fetched stylesheet is lost. Fixing that needs the fetched url to be passed down throughparseCrossOriginStylesheet,parseStylesheetandparseSameOriginStylesheet, which changes the signature of three utils. I left it out of this PR to keep the change small, but I am happy to send it as a separate PR if you would like that.Closes issue #5268