[Shadow CSS] Host context fix #21256

alx1417 · 2018-01-02T15:11:58Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

When you use more than one :host-context selector in the same css rule, the compiled css generate a invalid css code "-shadowcsscontext".

Example:

// Preprocessed code
:host-context(.one):host-context(.two) {}

/* Generated code*/
.one-shadowcsscontext(.two)[nghost],
.one   -shadowcsscontext(.two)[nghost] {}

Issue Number: #19199

What is the new behavior?

You can use two or more :host-context selector in the same css rule and generate all css combinations.

Example:

// Preprocessed code
:host-context(.one) :host-context(.two) {}

/* Generated code*/
.one[nghost] .two[ngcontent],
.one .two [a-host], 
.two[nghost] .one[ngcontent], 
.two .one [nghost], 
.one.two[nghost],
.one.two [nghost] {}

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Fix an error when use two or more :host-context selector. Now all host-context generate all posible combinations in the css result. Close issue angular#19199

googlebot · 2018-01-02T15:12:02Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

alx1417 · 2018-01-02T15:15:49Z

I signed it!

googlebot · 2018-01-02T15:15:53Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

alx1417 · 2018-01-02T15:49:04Z

I signed it!

alx1417 · 2018-01-02T16:04:51Z

I signed it!

googlebot · 2018-01-02T16:04:54Z

CLAs look good, thanks!

Fix an error when use two or more :host-context selector. Now all host-context generate all posible combinations in the css result. Close issue angular#19199

vicb · 2018-01-11T19:35:11Z

What does :host-context(.one):host-context(.two) mean ?
is it equivalent to :host-context(.one .two) ?

alx1417 · 2018-01-12T07:59:56Z

Hi vicb,

The code that you propouse is similar but not equivalent.

When use :host-context(.one .two) only have one combination of class (.one>.two).

When use :host-context(.one):host-context(.two) you have the two possible orders(.one>.two or .two >.one).

Also, in some cases is not possible use :host-context(.one .two) :

You not know the order of the clases. .one .two {} OR .two .one {}
The structure of the scss does not allow it, for example using the host context in multiples @Mixins

// mixins.scss
@mixin device-type-mobile {
  :host-context(.mobile) & {  @content;  }
}
@mixin device-os-ios {
  :host-context(.ios) & { @content;  }
}
// usage.scss
.wrapper {
  background: green;
  @include device-type-mobile {
    background: red;
    @include device-os-ios {
      background: blue;
    }
  }
}
// generated.css (Forced break lines and commented)

 // No mixin applied.
.wrapper[_ngcontent-c5] {
  background: green;
}

// One mixin applied
.mobile[_nghost-c5]   .wrapper[_ngcontent-c5], 
.mobile   [_nghost-c5]   .wrapper[_ngcontent-c5] {
  background: red;
}

// Two mixin applied
// Order .ios > .mobile
.ios[_nghost-c5]   .mobile[_ngcontent-c5]   .wrapper[_ngcontent-c5],
.ios   .mobile   [_nghost-c5]   .wrapper[_ngcontent-c5],
// Order .mobile > .ios
.mobile[_nghost-c5]   .ios[_ngcontent-c5]   .wrapper[_ngcontent-c5], 
.mobile   .ios   [_nghost-c5]   .wrapper[_ngcontent-c5],
// Same level .mobile = .ios
.ios.mobile[_nghost-c5]   .wrapper[_ngcontent-c5],
.ios.mobile   [_nghost-c5]   .wrapper[_ngcontent-c5] {
  background: blue;
}

Thank you for review the code.

vicb · 2018-01-16T18:37:40Z

When use :host-context(.one):host-context(.two) you have the two possible orders(.one>.two or .two >.one).

Do you have a link to a spec that would explain this behavior ?

alx1417 · 2018-01-18T09:01:03Z

Do you have a link to a spec that would explain this behavior ?

In this link the explain of the :host-context say:

The :host-context() selector looks for a CSS class in any ancestor of the component host element, up to the document root.

Not specify the behaviour when use two host-context, but if we apply the previous description to two host context, we have two selectors that "looks for a CSS class in any ancestor".

:host-context(.one) {} // .one [host] {}
:host-context(.two) {} // .two [host] {}
:host-context(.one .two) {} // .one .two [host] {}
:host-context(.two .one ) {} // .two .one [host] {}

:host-context(.one):host-context(.two) {} // .one .two [host], .two .one [host] {}
// .one should be ancestor and .two also
// NOTE: Now, before the fix, this return an invalid css (.one -shadowcsscontext(.two)  [host])

:host-context(.one) {
  color: red;
  :host-context(.two) {
    color: blue;
  }
}
/* Is not necessary that stay at the same level, for example
.one [host] {color: red},
.one .two [host], .two .one[host] {color: blue}
*/

I generate the two orders because the two host-context should by ancestors.

petebacondarwin

First of all, thank you @alx1417 for this PR. I am sorry it has taken so long to review it.

I don't believe the test nor the implementation are correct here.

Can you consider my suggested test? If you agree the test is correct, then the implementation needs to change since it fails this test.

Further, if you are going to update the implementation, then I think the changes should be in the _colonHostContextPartReplacer() method, rather than the _convertColonRule() which is shared with handling :host() selectors. And we should also look at the array manipulation being used as I think this could be a bit more performant.

petebacondarwin · 2021-01-19T12:45:31Z

packages/compiler/test/shadow_css_spec.ts

+        expect(s(':host-context(.one) :host-context(.two) {}', 'contenta', 'a-host'))
+            .toEqual(
+                '.one[a-host] .two[contenta], .one .two [a-host], .two[a-host] .one[contenta], .two .one [a-host], .one.two[a-host], .one.two [a-host] {}');
+      });


Just rebasing and tidying up this PR. I think this test is not actually correct.

First, I don't think that there should be a space between the :host-context() selectors in the string passed to s(). Having :host-context(...) :host-context(...) doesn't make sense to me. I am not even sure how we should interpret this.

Second, I think that contenta should not appear in the final selection at all.

This is what I would expect:

Suggested change

expect(s(':host-context(.one) :host-context(.two) {}', 'contenta', 'a-host'))

.toEqual(

'.one[a-host] .two[contenta], .one .two [a-host], .two[a-host] .one[contenta], .two .one [a-host], .one.two[a-host], .one.two [a-host] {}');

});

expect(s(':host-context(.one):host-context(.two) {}', 'contenta', 'a-host')

.replace(/ \{\}$/, '')

.split(/\,\s+/)

.sort())

.toEqual([

'.one .two[a-host]', // `one` is an ancestor and `two` is on the host

'.one .two [a-host]', // `one` and `two` are both ancestors (in that order)

'.one.two [a-host]', // `one` and `two` are both on the same ancestor

'.one.two[a-host]', // `one` and `two` both on the host

'.two .one [a-host]', // `two` and `one` are both ancestors (in that order)

'.two .one[a-host]', // `two` is an ancestor and `one` is on the host

]);

petebacondarwin · 2021-01-20T15:25:06Z

Closing in favour of #40494.

angular-automatic-lock-bot · 2021-02-20T16:34:30Z

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.}

fix(compiler): fix host-contex issue

99c6318

Fix an error when use two or more :host-context selector. Now all host-context generate all posible combinations in the css result. Close issue angular#19199

googlebot added the cla: no label Jan 2, 2018

alx1417 force-pushed the host-context-fix branch from 509700b to 99c6318 Compare January 2, 2018 15:35

googlebot added cla: yes and removed cla: no labels Jan 2, 2018

fix(compiler): fix host-contex issue

0739c53

Fix an error when use two or more :host-context selector. Now all host-context generate all posible combinations in the css result. Close issue angular#19199

mhevery requested a review from vicb January 4, 2018 00:49

mhevery added the area: core Issues related to the framework runtime label Jan 4, 2018

vicb changed the title ~~Host context fix~~ [Shadow CSS] Host context fix Jan 16, 2018

vicb self-assigned this Jan 29, 2018

vicb removed their request for review August 15, 2018 21:10

vicb removed their assignment Aug 15, 2018

jasonaden added this to the needsTriage milestone Jan 29, 2019

mhevery added this to Needs review in mhevery-review-queue via automation Dec 4, 2020

mhevery moved this from Needs review to Discuss in mhevery-review-queue Dec 4, 2020

mhevery moved this from Discuss to CleanUp in mhevery-review-queue Dec 4, 2020

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 labels Jan 19, 2021

ngbot bot modified the milestones: needsTriage, Backlog Jan 19, 2021

pullapprove bot requested a review from AndrewKushnir January 19, 2021 11:54

petebacondarwin requested changes Jan 19, 2021

View reviewed changes

petebacondarwin mentioned this pull request Jan 20, 2021

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

Closed

petebacondarwin closed this Jan 20, 2021

mhevery-review-queue automation moved this from CleanUp to Done Jan 20, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Shadow CSS] Host context fix #21256

[Shadow CSS] Host context fix #21256

alx1417 commented Jan 2, 2018

googlebot commented Jan 2, 2018

alx1417 commented Jan 2, 2018

googlebot commented Jan 2, 2018

alx1417 commented Jan 2, 2018

alx1417 commented Jan 2, 2018

googlebot commented Jan 2, 2018

vicb commented Jan 11, 2018 •

edited

alx1417 commented Jan 12, 2018

vicb commented Jan 16, 2018

alx1417 commented Jan 18, 2018

petebacondarwin left a comment •

edited

petebacondarwin Jan 19, 2021

petebacondarwin commented Jan 20, 2021

angular-automatic-lock-bot bot commented Feb 20, 2021

-        expect(s(':host-context(.one) :host-context(.two) {}', 'contenta', 'a-host'))
-            .toEqual(
-                '.one[a-host] .two[contenta], .one .two [a-host], .two[a-host] .one[contenta], .two .one [a-host], .one.two[a-host], .one.two [a-host] {}');
-      });
+expect(s(':host-context(.one):host-context(.two) {}', 'contenta', 'a-host')
+            .replace(/ \{\}$/, '')
+            .split(/\,\s+/)
+            .sort())
+    .toEqual([
+      '.one .two[a-host]',   // `one` is an ancestor and `two` is on the host
+      '.one .two [a-host]',  // `one` and `two` are both ancestors (in that order)
+      '.one.two [a-host]',   // `one` and `two` are both on the same ancestor
+      '.one.two[a-host]',    // `one` and `two` both on the host
+      '.two .one [a-host]',  // `two` and `one` are both ancestors (in that order)
+      '.two .one[a-host]',   // `two` is an ancestor and `one` is on the host
+    ]);

[Shadow CSS] Host context fix #21256

[Shadow CSS] Host context fix #21256

Conversation

alx1417 commented Jan 2, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Jan 2, 2018

alx1417 commented Jan 2, 2018

googlebot commented Jan 2, 2018

alx1417 commented Jan 2, 2018

alx1417 commented Jan 2, 2018

googlebot commented Jan 2, 2018

vicb commented Jan 11, 2018 • edited

alx1417 commented Jan 12, 2018

vicb commented Jan 16, 2018

alx1417 commented Jan 18, 2018

petebacondarwin left a comment • edited

Choose a reason for hiding this comment

petebacondarwin Jan 19, 2021

Choose a reason for hiding this comment

petebacondarwin commented Jan 20, 2021

angular-automatic-lock-bot bot commented Feb 20, 2021

vicb commented Jan 11, 2018 •

edited

petebacondarwin left a comment •

edited