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

Elements shadow dom fix #24861

Closed
wants to merge 9 commits into from
Closed

Elements shadow dom fix #24861

wants to merge 9 commits into from

Conversation

robwormald
Copy link
Contributor

@robwormald robwormald commented Jul 12, 2018 •
edited

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

@mary-poppins
Copy link

You can preview 1eec884 at https://pr24861-1eec884.ngbuilds.io/.

@robwormald robwormald force-pushed the elements-shadow-dom-fix branch 2 times, most recently from 458cf2f to 525ca3e Compare July 12, 2018 21:36
@mary-poppins
Copy link

You can preview 458cf2f at https://pr24861-458cf2f.ngbuilds.io/.

mhevery
mhevery reviewed Jul 12, 2018
View reviewed changes
packages/platform-browser/src/dom/dom_renderer.ts Outdated
throw new Error(`The selector "${selectorOrNode}" did not match any elements`);
}
el.textContent = '';
;
Copy link
Contributor

Choose a reason for hiding this comment

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

too many ;

packages/platform-browser/src/dom/dom_renderer.ts Outdated
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (!preserveContent) {
  el.textContent = '';
}

packages/platform-browser/src/dom/dom_renderer.ts Outdated
@@ -271,6 +274,20 @@ class ShadowDomRenderer extends DefaultDomRenderer2 {
}
}

selectRootElement(selectorOrNode: string|any): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there an extra method here?

@mary-poppins
Copy link

You can preview 525ca3e at https://pr24861-525ca3e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 7a08d69 at https://pr24861-7a08d69.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 8617d87 at https://pr24861-8617d87.ngbuilds.io/.

@mhevery mhevery added the area: core Issues related to the framework runtime label Jul 12, 2018
@mary-poppins
Copy link

You can preview 8d290e2 at https://pr24861-8d290e2.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 8cafe35 at https://pr24861-8cafe35.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 1f5de46 at https://pr24861-1f5de46.ngbuilds.io/.

@mary-poppins
Copy link

You can preview f9b7adc at https://pr24861-f9b7adc.ngbuilds.io/.

gkalpak
gkalpak reviewed Jul 16, 2018
View reviewed changes
packages/platform-browser/testing/src/browser_util.ts

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

get supportsDeprecatedCustomCustomElementsV0() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

packages/platform-browser/testing/src/browser_util.ts Outdated

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

@gkalpak gkalpak Jul 16, 2018 •
edited

Choose a reason for hiding this comment

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

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

packages/elements/test/slots_spec.ts Outdated
barBar: string
};
// we only run these tests in browsers that support Shadom DOM slots natively
if (browserDetection.supportsCustomElements && browserDetection.supportsShadowDom) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: supportsShadowDom implies supportsCustomElements afaict.

packages/elements/test/slots_spec.ts Outdated
template: '<div class="slotparent"><slot name="header"></slot><slot name="body"></slot></div>',
encapsulation: ViewEncapsulation.ShadowDom
})
class DefaultSlotsComponent {
Copy link
Member

Choose a reason for hiding this comment

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

This component is not used afaict.

packages/elements/test/slots_spec.ts Outdated
const content = testContainer.querySelector('span.projected') !;
const slot = testEl.shadowRoot !.querySelector('slot') !;
const assignedNodes = slot.assignedNodes();
expect(assignedNodes[0]).toEqual(content);
Copy link
Member

Choose a reason for hiding this comment

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

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

packages/elements/test/slots_spec.ts Outdated
import {NgElement, NgElementConstructor, createCustomElement} from '../src/create-custom-element';
import {NgElementStrategy, NgElementStrategyEvent, NgElementStrategyFactory} from '../src/element-strategy';

type WithFooBar = {
Copy link
Member

Choose a reason for hiding this comment

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

Unused.

packages/elements/test/slots_spec.ts Outdated
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');
Copy link
Member

Choose a reason for hiding this comment

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

Unused.

@mary-poppins
Copy link

You can preview 41e0914 at https://pr24861-41e0914.ngbuilds.io/.

@benoitjchevalier benoitjchevalier mentioned this pull request Jul 31, 2018
Closed
@mhevery
Copy link
Contributor

mhevery commented Aug 2, 2018

http://test/OCL:207128490:BASE:207128536:1533232686068:75ae6d95

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Aug 2, 2018
@ngbot
Copy link

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
IgorMinar suggested changes Aug 2, 2018
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

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

packages/core/src/render/api.ts Outdated
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

please document the new param

Copy link
Contributor

Choose a reason for hiding this comment

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

also please document the default value.

@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Aug 3, 2018
@mary-poppins
Copy link

You can preview a81d30f at https://pr24861-a81d30f.ngbuilds.io/.

@mhevery mhevery closed this in 6e828bb Aug 31, 2018
mhevery pushed a commit that referenced this pull request Aug 31, 2018
@robwormald @mhevery
ci: update chrome version to 67 (#24861)
1d8e821 
PR Close #24861
mhevery pushed a commit that referenced this pull request Aug 31, 2018
@robwormald @mhevery
ci: update firefox version to 61 (#24861)
6e6489a 
PR Close #24861
mhevery pushed a commit that referenced this pull request Aug 31, 2018
@robwormald @mhevery
test(core): update Web Platform feature detection (#24861)
d76a7d6 
PR Close #24861
mhevery pushed a commit that referenced this pull request Aug 31, 2018
@robwormald @mhevery
test(elements): add basic integration test for angular elements (#24861)
4815b92 
PR Close #24861
mhevery pushed a commit that referenced this pull request Aug 31, 2018
@robwormald @mhevery
feat(elements): enable Shadow DOM v1 and slots (#24861)
c9844a2 
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 pushed a commit that referenced this pull request Aug 31, 2018
@robwormald @mhevery
fix(elements): strict null checks (#24861)
a8210d0 
PR Close #24861
mhevery pushed a commit that referenced this pull request Aug 31, 2018
@robwormald @mhevery
fix(elements): add compiler to integration (#24861)
a080ffc 
PR Close #24861
mhevery pushed a commit that referenced this pull request Aug 31, 2018
@robwormald @mhevery
fix(elements): add compiler dependency (#24861)
6143da6 
PR Close #24861
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
fix(core): do not clear element content when using shadow dom (angula…
ef1cd72 
…r#24861)

PR Close angular#24861
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
ci: update chrome version to 67 (angular#24861)
dabb3de 
PR Close angular#24861
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
ci: update firefox version to 61 (angular#24861)
5bdc84b 
PR Close angular#24861
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
test(core): update Web Platform feature detection (angular#24861)
4811c66 
PR Close angular#24861
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
test(elements): add basic integration test for angular elements (angu…
9f085f7 
…lar#24861)

PR Close angular#24861
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
feat(elements): enable Shadow DOM v1 and slots (angular#24861)
a5f37d3 
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 pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
fix(elements): strict null checks (angular#24861)
bcc1b69 
PR Close angular#24861
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
fix(elements): add compiler to integration (angular#24861)
bb789df 
PR Close angular#24861
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018
@robwormald @petebacondarwin
fix(elements): add compiler dependency (angular#24861)
7e3d28c 
PR Close angular#24861
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
fix(core): do not clear element content when using shadow dom (angula…
46f1e45 
…r#24861)

PR Close angular#24861
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
ci: update chrome version to 67 (angular#24861)
b0fe4b2 
PR Close angular#24861
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
ci: update firefox version to 61 (angular#24861)
371daab 
PR Close angular#24861
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
test(core): update Web Platform feature detection (angular#24861)
557e5e5 
PR Close angular#24861
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
test(elements): add basic integration test for angular elements (angu…
6a2ab85 
…lar#24861)

PR Close angular#24861
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
feat(elements): enable Shadow DOM v1 and slots (angular#24861)
79047ca 
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
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
fix(elements): strict null checks (angular#24861)
cefde58 
PR Close angular#24861
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
fix(elements): add compiler to integration (angular#24861)
9ffe5c2 
PR Close angular#24861
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@robwormald @FrederikSchlemmer
fix(elements): add compiler dependency (angular#24861)
b1e2cb5 
PR Close angular#24861
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@IgorMinar IgorMinar IgorMinar approved these changes

@gkalpak gkalpak gkalpak approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Renderer should not clear host contents when using Shadow DOM
6 participants
@robwormald @mary-poppins @mhevery @IgorMinar @gkalpak @googlebot