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.

Sign up for GitHub

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

Jump to bottom

fix(compiler): improved support for :host-context() selectors #40494

Closed
wants to merge 3 commits into from
Closed

fix(compiler): improved support for :host-context() selectors #40494

wants to merge 3 commits into from

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Jan 20, 2021 •
edited

Two commits that fix problems with emulated shadow dom handling of :host-context.

Fixes #19199
Fixes #14349

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Jan 20, 2021
@ngbot ngbot bot modified the milestone: Backlog Jan 20, 2021
@google-cla google-cla bot added the cla: yes label Jan 20, 2021
@petebacondarwin petebacondarwin mentioned this pull request Jan 20, 2021
Closed
3 tasks
@petebacondarwin petebacondarwin mentioned this pull request Jan 20, 2021
Closed
JoostK
JoostK approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am no subject expert on this matter but this looks correct to me.

packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved
packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved
packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved
packages/compiler/src/shadow_css.ts Outdated Show resolved Hide resolved
AndrewKushnir
AndrewKushnir approved these changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 20, 2021
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir
Copy link
Contributor

@petebacondarwin presubmit indicated that there are some issues in IE11 on Win7, here is an error message:

TypeError: Syntax error in regular expression

The error is coming from the minified code, so there is no exact location available in logs :(

Please let me know if you would you be able to test it in IE11 or you want me to do more debugging and find the problematic RegExp?

@petebacondarwin
Copy link
Member Author

@AndrewKushnir - I suspect it must be that in IE11 the RegExp constructor cannot take a RegExp as its first argument: https://stackoverflow.com/a/40629349. I'll fix that.

@AndrewKushnir
Copy link
Contributor

Presubmit #2.

@AndrewKushnir AndrewKushnir removed action: presubmit The PR is in need of a google3 presubmit state: blocked labels Jan 22, 2021
@AndrewKushnir
Copy link
Contributor

@petebacondarwin additional changes fixed issues in the presubmit and it's now passing. Thank you.

@AndrewKushnir
Copy link
Contributor

Thanks for the quick fix @petebacondarwin, I've cancelled the previous TGP run and started a new TGP with the most recent changes from this PR (also Ivy Blueprint run).

@AndrewKushnir
Copy link
Contributor

Quick update after running TGP: there are some cases when multiple selectors are used inside :host-context, for ex.:

:host-context(.foo, .bar) { padding: 0px; }

That would require additional investigation on whether we should support it (it doesn't seem to be a part of the spec) or drop support for it (thus making this a breaking change).

@petebacondarwin
fix(compiler): support multiple :host-context() selectors
59cce9d 
In `ViewEncapsulation.Emulated` mode, the compiler must generate additional
combinations of selectors to handle the `:host-context()` pseudo-class function.

Previously, when there is was more than one `:host-context()` selector in a
rule, the compiler was generating invalid selectors.

This commit generates all possible combinations of selectors needed to
match the same elements as the native `:host-context()` selector.

Fixes angular#19199
@petebacondarwin
fix(compiler): handle :host-context and :host in the same selector
1abc156 
In `ViewEncapsulation.Emulated` mode the compiler converts `:host` and
`:host-context` pseudo classes into new CSS selectors.

Previously, when there was both `:host-context` and `:host` classes in a
selector, the compiler was generating incorrect selectors. There are two
scenarios:

* Both classes are on the same element (i.e. not separated). E.g.
  `:host-context(.foo):host(.bar)`. This setup should only match the
  host element if it has both `foo` and `bar` classes. So the generated
  CSS selector should be: `.foo.bar<hostmarker>`.
* The `:host` class is on a descendant of the `:host-context`. E.g.
  `:host-context(.foo) :host(.bar)`. This setup should only match the
  `.foo` selector if it is a proper ancestor of the host (and not on the
  host itself). So the generated CSS selector should be:
  `.foo .bar<hostmarker>`.

This commit fixes the generation to handle these scenarios.

Fixes angular#14349
@petebacondarwin
fix(compiler): support multiple selectors in :host-context()
3897bab 
The previous commits refactored the `ShadowCss` emulator to support
desirable use-cases of `:host-context()`, but it dropped support
for passing a comma separated list of selectors to the `:host-context()` .

This commit rectifies that omission, despite the use-case not being
valid according to the ShadowDOM spec, to ensure backward compatibility
with the previous implementation.
@AndrewKushnir
Copy link
Contributor

Presubmit #3.

@AndrewKushnir
Copy link
Contributor

Global Presubmit.

@AndrewKushnir
Copy link
Contributor

Quick update after global presubmit: there is one failing target (not a flake) with a slight visual difference in screen shot tests. I will perform further investigation and will keep this thread updated...

@AndrewKushnir
Copy link
Contributor

@petebacondarwin I've extracted a part of CSS that is now processed differently:

:host-context(.toolbar--top) :host {}

Current (in master branch):

.toolbar--top[a-host] [a-host],
.toolbar--top [a-host] [a-host] {}

After the changes in this PR:

.toolbar--top [a-host] {}

@petebacondarwin
Copy link
Member Author

petebacondarwin commented Jan 29, 2021 •
edited

Thanks for running the presubmit and providing this breaking case @AndrewKushnir. I think this really is a situation where this PR is correct and the old behaviour is wrong. The way to interpret the style in question:

:host-context(.toolbar--top) :host {}

Is that it will match only if there is an element with class toolbar--top that is on either a full ancestor of the host element or the host element itself (e.g. div.toolbar--top <host> or <host>.toolbar--top); followed a full descendant of this element being the host element. In other words:

.toolbar--top[a-host] [a-host]
.toolbar--top [a-host]

Since the first of these two selectors should never match, if taking the spec accurately, (you cannot have an element that is descended from itself) then only the second selector is relevant. So we end up with

.toolbar--top [a-host] {}

If we consider the selectors previously generated:

.toolbar--top[a-host] [a-host]
.toolbar--top [a-host] [a-host]

These selectors would never match anything, since the [a-host] element cannot be a descendant of itself.
I don't believe the application developer meant the this.

So I think that in this case we should fix up the downstream application rather than make any change to this PR. WDYT?

@petebacondarwin
Copy link
Member Author

This collection of examples seems to show that the implementation of :host-context(xxx) :host type selectors in this PR is at odds with what Chrome does.

https://stackblitz.com/edit/angular-ivy-wamhub?file=src%2Fapp%2Fstyles.ts

So either Chrome is also buggy in this regard or my understanding of the spec is wrong.

@petebacondarwin
Copy link
Member Author

petebacondarwin commented Jan 31, 2021 •
edited

Also I note that the failure is a style that is no longer being assigned but the new selector is more relaxed than the old selector: The new selector .toolbar--top [a-host] in this PR would select everything that the previous selectors would: .toolbar--top[a-host] [a-host], .toolbar--top [a-host] [a-host].

@AndrewKushnir
Copy link
Contributor

FYI, the code of an app was updated in Google's codebase (it turned out that the CSS rule was unused) and I've started a new Global Presubmit to verify that there are no similar cases and the change can be safely submitted. Will keep this thread updated.

@AndrewKushnir AndrewKushnir removed action: presubmit The PR is in need of a google3 presubmit state: blocked labels Feb 16, 2021
@AndrewKushnir
Copy link
Contributor

@petebacondarwin FYI presubmits are successful for the changes in this PR, we can proceed with merging (plz add the "merge" label if it's ready to go)!

@petebacondarwin
Copy link
Member Author

That's great news @AndrewKushnir - thanks for helping to shepherd this through.

@petebacondarwin petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Feb 16, 2021
josephperrott pushed a commit that referenced this pull request Feb 16, 2021
@petebacondarwin @josephperrott
fix(compiler): support multiple :host-context() selectors (#40494)
c20e304 
In `ViewEncapsulation.Emulated` mode, the compiler must generate additional
combinations of selectors to handle the `:host-context()` pseudo-class function.

Previously, when there is was more than one `:host-context()` selector in a
rule, the compiler was generating invalid selectors.

This commit generates all possible combinations of selectors needed to
match the same elements as the native `:host-context()` selector.

Fixes #19199

PR Close #40494
josephperrott pushed a commit that referenced this pull request Feb 16, 2021
@petebacondarwin @josephperrott
fix(compiler): handle :host-context and :host in the same selector (
ab8ea87 
#40494)

In `ViewEncapsulation.Emulated` mode the compiler converts `:host` and
`:host-context` pseudo classes into new CSS selectors.

Previously, when there was both `:host-context` and `:host` classes in a
selector, the compiler was generating incorrect selectors. There are two
scenarios:

* Both classes are on the same element (i.e. not separated). E.g.
  `:host-context(.foo):host(.bar)`. This setup should only match the
  host element if it has both `foo` and `bar` classes. So the generated
  CSS selector should be: `.foo.bar<hostmarker>`.
* The `:host` class is on a descendant of the `:host-context`. E.g.
  `:host-context(.foo) :host(.bar)`. This setup should only match the
  `.foo` selector if it is a proper ancestor of the host (and not on the
  host itself). So the generated CSS selector should be:
  `.foo .bar<hostmarker>`.

This commit fixes the generation to handle these scenarios.

Fixes #14349

PR Close #40494
josephperrott pushed a commit that referenced this pull request Feb 16, 2021
@petebacondarwin @josephperrott
fix(compiler): support multiple selectors in :host-context() (#40494)
fb0ce21 
The previous commits refactored the `ShadowCss` emulator to support
desirable use-cases of `:host-context()`, but it dropped support
for passing a comma separated list of selectors to the `:host-context()` .

This commit rectifies that omission, despite the use-case not being
valid according to the ShadowDOM spec, to ensure backward compatibility
with the previous implementation.

PR Close #40494
josephperrott pushed a commit that referenced this pull request Feb 16, 2021
@petebacondarwin @josephperrott
fix(compiler): handle :host-context and :host in the same selector (
679c3bf 
#40494)

In `ViewEncapsulation.Emulated` mode the compiler converts `:host` and
`:host-context` pseudo classes into new CSS selectors.

Previously, when there was both `:host-context` and `:host` classes in a
selector, the compiler was generating incorrect selectors. There are two
scenarios:

* Both classes are on the same element (i.e. not separated). E.g.
  `:host-context(.foo):host(.bar)`. This setup should only match the
  host element if it has both `foo` and `bar` classes. So the generated
  CSS selector should be: `.foo.bar<hostmarker>`.
* The `:host` class is on a descendant of the `:host-context`. E.g.
  `:host-context(.foo) :host(.bar)`. This setup should only match the
  `.foo` selector if it is a proper ancestor of the host (and not on the
  host itself). So the generated CSS selector should be:
  `.foo .bar<hostmarker>`.

This commit fixes the generation to handle these scenarios.

Fixes #14349

PR Close #40494
josephperrott pushed a commit that referenced this pull request Feb 16, 2021
@petebacondarwin @josephperrott
fix(compiler): support multiple selectors in :host-context() (#40494)
645c2ef 
The previous commits refactored the `ShadowCss` emulator to support
desirable use-cases of `:host-context()`, but it dropped support
for passing a comma separated list of selectors to the `:host-context()` .

This commit rectifies that omission, despite the use-case not being
valid according to the ShadowDOM spec, to ensure backward compatibility
with the previous implementation.

PR Close #40494
@kreuzerk kreuzerk mentioned this pull request Mar 17, 2021
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@JoostK JoostK JoostK approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
5 participants
@petebacondarwin @AndrewKushnir @jessicajaniuk @JoostK @crisbeto