Support pseudoelements in CSS scoping #25270
Merged
Conversation
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
SteveSandersonMS
added
area-mvc
javiercn
approved these changes
Aug 26, 2020
javiercn
reviewed
Aug 26, 2020
src/Razor/Microsoft.AspNetCore.Razor.Tools/test/RewriteCssCommandTest.cs
Show resolved
Hide resolved
It's a good point. The unit tests are meant to be the statement of what cases are relevant, but it's definitely useful to get a secondary confirmation that they are broad enough. To do this I've just compared against the cases supported by Vue, and it turns out we already did cover all of them with one exception (leading pseudoelements) that I've now added in the latest commit. |
Not that anybody should ever want to do this
SteveSandersonMS
added
ask-mode
|
Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge. |
pranavkm
added
bug
|
Approved for RC1 pending CI completion. |
SteveSandersonMS
added
Done
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.
Fixes #25268
Without this, the new test cases
HandlesPseudoElementsandHandlesSingleColonPseudoElements(added in this PR) would fail.Note that the new test case
HandlesPseudoClasses(also added in this PR) would pass even without the implementation change, but it's good to have more test cases explicitly about pseudoclasses to make sure the pseudoelement support doesn't trip them up.Ask-mode template
Description
The new CSS scoping feature, first shipped in 5.0-preview8, contains a bug whereby a particular kind of CSS rule doesn't get scoped properly.
Customer Impact
This was reported by a customer: https://devblogs.microsoft.com/aspnet/asp-net-core-updates-in-net-5-preview-8/#comment-2540
The impact is that CSS pseudoelements don't work with CSS scoping, so the application UI doesn't render with the intended styling.
The only workaround would be for the developer to change their CSS to avoid having this combination of features, which probably means not using scoping.
It's very desirable to include the fix in RC1 because we want to be sure the CSS scoping feature is totally robust by the time it ships. If we don't include the fix, then customers won't get a ship-ready-candidate version of the feature until RC2, and then there will be very little time left for remaining feedback.
Regression?
No, this is a new feature
Risk
In the worst case, this might theoretically introduce some other new bug into the CSS scoping feature. However, test coverage is pretty good, and it doesn't have interactions with other features.