-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
fix(compiler): handle css selectors with space after an escaped character #48558
Conversation
86619a9
to
8ebe1dc
Compare
8ebe1dc
to
4eeefc7
Compare
@@ -152,6 +152,7 @@ const animationKeywords = new Set([ | |||
in comments in lieu of the next selector when running under polyfill. | |||
*/ | |||
export class ShadowCss { | |||
// TODO: Is never re-assigned, could be removed. |
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.
Any insights if this could be safely removed ?
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.
ah yeah good spot 🤔 , by looking at the git history I think it might have been added initially to keep the code in this file close to the original webcomponents.js implementation, even initially this was already used but not re-assigned: d67f029#diff-8100ed7347ea0eb1cd98948f17ca578cfc288fc0d58538e3a83db9dac133276b
I bet that the file has strained a lot from the original webcomponents.js code, I wonder if there's still have any value in keeping this in 🤔
Presubmit is failing, it seems the test coverage isn't enough. I'd appreciate if someone takes the time to check what has been broken. |
bc01058
to
29264da
Compare
29264da
to
f951834
Compare
Thx for the feedback @atscott, turns out JSCore unlike V8 doesn't support variable length lookbehind in regex. I proposed an alternative fix. Edit: Still broken, even though the regex is the same now. |
6e92732
to
16f2b4c
Compare
Converting back to draft, since it seems like work is ongoing. |
16f2b4c
to
7416193
Compare
@dylhunn Actually, I would expect my commit to not break any tests. test_win seems like a false negative. Thanks for your time ! |
it('should handle escaped selector with space (if followed by a hex char)', () => { | ||
expect(s('.\\fc ber {}', 'contenta')).toEqual('.\\fc ber[contenta] {}'); | ||
expect(s('.\\fc ker {}', 'contenta')).toEqual('.\\fc[contenta] ker[contenta] {}'); | ||
expect(s('.pr\\fc fung {}', 'contenta')).toEqual('.pr\\fc fung[contenta] {}'); | ||
}); |
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.
Is there any way these tests can be made to actually verify the behaviors rather than the implementation details? That is, write a test with a selector like über and verify it works as expected. I'm guessing not since the discussion in the bug describes needing to run esbuild with optimization.minify
. At the very least, it would be good to document in the code comment why/what is converting über to .fc ber
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.
There is a CSS.escape
in js but it doesn't do the same as the esbuild. So yeah I don't much of a possiblity to test it.
I added a comment about it in the test and the implementation.
ba94b1c
to
4b8a6ed
Compare
4b8a6ed
to
f363d07
Compare
@JeanMeche Looks like this needs a rebase? |
…cter. In Css, selectors with escaped characters require a space after if the following character is a hex character. ie: .\fc ber which matches class="über" These escaped selectors happen for example when esbuild run with `optimization.minify` fixes angular#48524
f363d07
to
1ba1105
Compare
caretaker: windows failure appears to be a flake |
This PR was merged into the repository by commit bc8cfa2. |
…ed character. (angular#48558)" This reverts commit bc8cfa2.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`15.1.1` -> `15.1.2`](https://renovatebot.com/diffs/npm/@angular%2fanimations/15.1.1/15.1.2) | | [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`15.1.1` -> `15.1.2`](https://renovatebot.com/diffs/npm/@angular%2fcommon/15.1.1/15.1.2) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`15.1.1` -> `15.1.2`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/15.1.1/15.1.2) | | [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | patch | [`15.1.1` -> `15.1.2`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/15.1.1/15.1.2) | | [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`15.1.1` -> `15.1.2`](https://renovatebot.com/diffs/npm/@angular%2fcore/15.1.1/15.1.2) | | [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`15.1.1` -> `15.1.2`](https://renovatebot.com/diffs/npm/@angular%2fforms/15.1.1/15.1.2) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`15.1.1` -> `15.1.2`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/15.1.1/15.1.2) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`15.1.1` -> `15.1.2`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/15.1.1/15.1.2) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v15.1.2`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#​1512-2023-01-25) [Compare Source](angular/angular@15.1.1...15.1.2) ##### compiler | Commit | Type | Description | | -- | -- | -- | | [98ccb57117](angular/angular@98ccb57) | fix | handle css selectors with space after an escaped character. ([#​48558](angular/angular#48558)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [145f848a10](angular/angular@145f848) | fix | resolve deprecation warning ([#​48812](angular/angular#48812)) | ##### router | Commit | Type | Description | | -- | -- | -- | | [a6b10f6e59](angular/angular@a6b10f6) | fix | 'createUrlTreeFromSnapshot' with empty paths and named outlets ([#​48734](angular/angular#48734)) | #### Special Thanks Alan Agius, AleksanderBodurri, Andrew Kushnir, Andrew Scott, Charles Lyding, Dylan Hunn, JoostK, Matthieu Riegler, Paul Gschwendtner, Payam Valadkhan, Virginia Dooley, Yann Thomas LE MOIGNE and dario-piotrowicz <!-- CHANGELOG SPLIT MARKER --> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMTQuMSIsInVwZGF0ZWRJblZlciI6IjM0LjExNC4xIn0=--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1751 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
…cter. (angular#48558) In Css, selectors with escaped characters require a space after if the following character is a hex character. ie: .\fc ber which matches class="über" These escaped selectors happen for example when esbuild run with `optimization.minify` fixes angular#48524 PR Close angular#48558
In prod builds, selectors are optimized and spaces a removed. angular#48558 introduced a regression on selectors without spaces. This commit fixes tihs. Fixes angular#49100
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
In Css, selectors with escaped characters require a space after if the following character is a hex character. ie: .\fc ber which match class="über"
These escaped selectors happen for example when esbuild run with
optimization.minify
fixes #48524
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?