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.

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

Fix templatizer under the template polyfill. forceDocumentUpgrade #2707

Closed
wants to merge 1 commit into from

Conversation

garlicnation
Copy link
Contributor

was interacting problematically with un-prepped HTMLTemplateElements.
By forcing a bootstrap before the document upgrade, we sidestep this issue.

@garlicnation garlicnation force-pushed the templatizeRender branch 2 times, most recently from 8afc8f2 to 04550fe Compare November 9, 2015 21:36
@dfreedm
Copy link
Member

dfreedm commented Nov 9, 2015

We should be able to remove this code then: https://github.com/Polymer/polymer/blob/master/src/mini/template.html#L36-L42

@dfreedm
Copy link
Member

dfreedm commented Nov 9, 2015

LGTM with nit to remove the leftover template bootstrap code in _prepTemplate

was interacting problematically with un-prepped HTMLTemplateElements.
By forcing a bootstrap before the document upgrade, we sidestep this issue.
@dfreedm
Copy link
Member

dfreedm commented Nov 9, 2015

Notes for reasoning:

The HTMLTemplateElement polyfill will not run until DOMContentLoaded.
The HTMLImports Polyfill works around this by upgrading templates in each document before asking the CustomElements polyfill to observe the document and allow for element upgrades.

However, when using vulcanize with all the inlining flags, all elements will upgrade before the template polyfill runs, because of the call to forceDocumentUpgrade in the element upgrade flow. This can break dom-repeat and a few other elements that expect their template to have a content document.

This patch ensures that all templates in the document will be upgraded before all of the elements upgrade, just as in the standard use with HTMLImports.

@sorvell @kevinpschaaf PTAL

@kevinpschaaf
Copy link
Member

Reviewed along with @sorvell.

Making the fix in dom-module's forceDocumentUpgrade isn't the best place, since ideally elements that are known to not have dom-module's (like dom-repeat) wouldn't try to look for a dom-module in the first place, fail, and call that function.

I'm inclined to instead add specific code to bootstrap template extension elements in Polymer's startup flow, ala add this to _prepTemplate (leaving the existing code as-is):

if (this.localName == 'template') {
  HTMLTemplateElement.decorate(this);       
  HTMLTemplateElement.bootstrap(this.content);      
}

This issue highlights a potentially serious performance issue with the way forceDocumentUpgrade does an expensive tree walk each time an element that intentionally has no dom-module is registered; opened separate issue for that here: #2708

@kevinpschaaf
Copy link
Member

Per discussion, consider moving decorate & bootstrap code to CustomElements polyfill: https://github.com/webcomponents/webcomponentsjs/blob/master/src/CustomElements/upgrade.js#L60

@dfreedm
Copy link
Member

dfreedm commented Nov 9, 2015

I'm not sure I follow. Do you want the Template polyfill to actually use the CustomElement machinery?

@dfreedm
Copy link
Member

dfreedm commented Nov 10, 2015

Just to be 100% clear, this PR should be closed and we will proceed with webcomponents/webcomponentsjs#439 right?

@kevinpschaaf
Copy link
Member

Yes, that's the plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants