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

Elements shadow dom fix #24861

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@robwormald
Member

robwormald commented Jul 12, 2018

Adds a new optional parameter to the renderer.selectRootElement method:
Before:

renderer.selectRootElement(rootSelectorOrNode);
renderer.selectRootElement(rootSelectorOrNode, preserveContents?:boolean);

preserveContents defaults to false to preserve existing renderer behavior. If set to true, the renderer will not remove the existing contents of a root element when bootstrapping a component.

This is primarily useful when combined with the ViewEncapsulation.ShadowDom Renderer - the nodes are preserved, and developers can use <slot> elements to re-project the light DOM nodes.

This change had to be made to the Renderer API itself, rather than an in a specific renderer as the method is called on the default renderer, inside the view engine, rather than a renderer specific to the component: https://github.com/angular/angular/blob/master/packages/core/src/view/services.ts#L756

Fixes #24859

@googlebot googlebot added the cla: yes label Jul 12, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 12, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 12, 2018

throw new Error(`The selector "${selectorOrNode}" did not match any elements`);
}
el.textContent = '';
;

This comment has been minimized.

@mhevery

mhevery Jul 12, 2018

Member

too many ;

let el: any = typeof selectorOrNode === 'string' ? document.querySelector(selectorOrNode) :
selectorOrNode;
if (!el) {
throw new Error(`The selector "${selectorOrNode}" did not match any elements`);
}
el.textContent = '';
if (preserveContent) {
return el;

This comment has been minimized.

@mhevery

mhevery Jul 12, 2018

Member

Not a fan of early returns. Makes the code hard to follow. Rewrite as:

if (!preserveContent) {
  el.textContent = '';
}
@@ -271,6 +274,20 @@ class ShadowDomRenderer extends DefaultDomRenderer2 {
}
}
selectRootElement(selectorOrNode: string|any): any {

This comment has been minimized.

@mhevery

mhevery Jul 12, 2018

Member

Why is there an extra method here?

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 12, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 12, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 12, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 12, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 15, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 15, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 16, 2018

get supportsCustomElements() { return (typeof(<any>global).customElements !== 'undefined'); }
get supportsDeprecatedCustomCustomElementsV0() {

This comment has been minimized.

@gkalpak

gkalpak Jul 16, 2018

Member

This doesn't seem to be used anywhere. Is it necessary?

This comment has been minimized.

@robwormald

robwormald Aug 7, 2018

Member

I want to add a number of new integration tests for native stuff, so I added it here, also to make clear that both of these APIs are DEPRECATED

This comment has been minimized.

@IgorMinar

IgorMinar Aug 16, 2018

Member

@robwormald it's better to add these helper methods when we need them, otherwise lots of them go unused. Please use that rule in the future.

get supportsShadowDom() {
const testEl = document.createElement('div');
return (typeof customElements !== 'undefined') && (typeof testEl.attachShadow !== 'undefined');

This comment has been minimized.

@gkalpak

gkalpak Jul 16, 2018

Member

typeof customElements !== 'undefined' could be replaced with this.supportsCustomElements (here and below).

barBar: string
};
// we only run these tests in browsers that support Shadom DOM slots natively
if (browserDetection.supportsCustomElements && browserDetection.supportsShadowDom) {

This comment has been minimized.

@gkalpak

gkalpak Jul 16, 2018

Member

Nit: supportsShadowDom implies supportsCustomElements afaict.

template: '<div class="slotparent"><slot name="header"></slot><slot name="body"></slot></div>',
encapsulation: ViewEncapsulation.ShadowDom
})
class DefaultSlotsComponent {

This comment has been minimized.

@gkalpak

gkalpak Jul 16, 2018

Member

This component is not used afaict.

const content = testContainer.querySelector('span.projected') !;
const slot = testEl.shadowRoot !.querySelector('slot') !;
const assignedNodes = slot.assignedNodes();
expect(assignedNodes[0]).toEqual(content);

This comment has been minimized.

@gkalpak

gkalpak Jul 16, 2018

Member

This should be toBe(), instead of toEqual(), (here and below) to be sure the node is not duplicated or something.

import {NgElement, NgElementConstructor, createCustomElement} from '../src/create-custom-element';
import {NgElementStrategy, NgElementStrategyEvent, NgElementStrategyFactory} from '../src/element-strategy';
type WithFooBar = {

This comment has been minimized.

@gkalpak

gkalpak Jul 16, 2018

Member

Unused.

templateEl.innerHTML = tpl;
const template = templateEl.content.cloneNode(true) as DocumentFragment;
const testEl = template.querySelector('slot-events-el') !as NgElement & SlotEventsComponent;
const content = template.querySelector('span.projected');

This comment has been minimized.

@gkalpak

gkalpak Jul 16, 2018

Member

Unused.

@mary-poppins

This comment has been minimized.

mary-poppins commented Jul 31, 2018

@mhevery

This comment has been minimized.

@ngbot

This comment has been minimized.

ngbot bot commented Aug 2, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "PR target: *"
    pending status "google3" is pending
    pending status "code-review/pullapprove" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@IgorMinar

this pr has many of unresolved review comments from @gkalpak please address.

@@ -267,7 +267,7 @@ export abstract class Renderer2 {
* @param selectorOrNode The DOM element.
* @returns The root element.
*/
abstract selectRootElement(selectorOrNode: string|any): any;
abstract selectRootElement(selectorOrNode: string|any, preserveContent?: boolean): any;

This comment has been minimized.

@IgorMinar

IgorMinar Aug 2, 2018

Member

please document the new param

This comment has been minimized.

@IgorMinar

IgorMinar Aug 2, 2018

Member

also please document the default value.

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 7, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 8, 2018

@gkalpak

gkalpak approved these changes Aug 8, 2018

Some minor comments. LGTM otherwise 👍

expect(helloWorldEl.getText()).toEqual('Hello World!');
});
it('should display: Hello fromIndex! via name attribute', function () {

This comment has been minimized.

@gkalpak

gkalpak Aug 8, 2018

Member

fromIndex --> Foo

"preprotractor": "tsc -p e2e",
"protractor": "protractor e2e/protractor.config.js"
}
}

This comment has been minimized.

@gkalpak

gkalpak Aug 8, 2018

Member

It's a good idea to have private: true, to avoid accidental publication.

);
customElements.define(
'test-card',
createCustomElement(HelloWorldShadowComponent, {injector})

This comment has been minimized.

@gkalpak

gkalpak Aug 8, 2018

Member

Use TestCardComponent.

</body>
</html>

This comment has been minimized.

@gkalpak

gkalpak Aug 8, 2018

Member

This file doesn't seem to be used atm 😕

This comment has been minimized.

@robwormald

robwormald Aug 8, 2018

Member

same as above - there's a whole suite of integration tests for slots/shadow roots I'd like to add, but can't until this PR lands :)

* @param preserveContent Whether the contents of the root element
* should be preserved, or cleared upon bootstrap (default behavior).
* Use with ViewEncapsulation.ShadowDom to allow simple native
* content projection via <slot> elements.

This comment has been minimized.

@gkalpak

gkalpak Aug 8, 2018

Member

Wrap ViewEncapsulation... and <slot> in backticks to be rendered better in docs.

robwormald added some commits Jul 12, 2018

feat(elements): enable Shadow DOM v1 and slots
When using ViewEncapsulation.ShadowDom, Angular will not remove the child nodes of the DOM node a root Component is bootstrapped into. This enables developers building Angular Elements to use the `<slot>` element to do native content projection.
@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 28, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 28, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 28, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 30, 2018

@mhevery mhevery closed this in 6e828bb Aug 31, 2018

mhevery added a commit that referenced this pull request Aug 31, 2018

mhevery added a commit that referenced this pull request Aug 31, 2018

mhevery added a commit that referenced this pull request Aug 31, 2018

mhevery added a commit that referenced this pull request Aug 31, 2018

feat(elements): enable Shadow DOM v1 and slots (#24861)
When using ViewEncapsulation.ShadowDom, Angular will not remove the child nodes of the DOM node a root Component is bootstrapped into. This enables developers building Angular Elements to use the `<slot>` element to do native content projection.

PR Close #24861

mhevery added a commit that referenced this pull request Aug 31, 2018

mhevery added a commit that referenced this pull request Aug 31, 2018

mhevery added a commit that referenced this pull request Aug 31, 2018

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

feat(elements): enable Shadow DOM v1 and slots (angular#24861)
When using ViewEncapsulation.ShadowDom, Angular will not remove the child nodes of the DOM node a root Component is bootstrapped into. This enables developers building Angular Elements to use the `<slot>` element to do native content projection.

PR Close angular#24861

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment