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

template_binding doesn't clear nested templates on Safari #19109

Closed
justinfagnani opened this issue May 30, 2014 · 15 comments
Closed

template_binding doesn't clear nested templates on Safari #19109

justinfagnani opened this issue May 30, 2014 · 15 comments
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. closed-duplicate Closed in favor of an existing report

Comments

@justinfagnani
Copy link
Contributor

This might ultimately be the cause of https://code.google.com/p/dart/issues/detail?id=16568

In Safari only (maybe IE, but not Firefox), a nested template is not cleared out, and it's bindings are evaluated. The template is hidden with display: none, which is probably why we haven't gotten any reports of this yet.

Here's a minimal reproduction:

index.dart:

import 'dart:html';
import 'package:template_binding/template_binding.dart';
import 'package:smoke/mirrors.dart';

main() {
  useMirrors();
  var template = templateBind(querySelector("#test")).model = new Model();
}

class Model {
  String data = 'bar';
}

index.html:

<!DOCTYPE html>
<html>
  <body>
    <template id="test" bind>
      <div>{{ data }}</div>
      <template bind="{{ data }}" id="inner">
        <div>{{ }}</div>
      </template>
    </template>
    <script type="application/dart" src="polyfill_test.dart"></script>
    <script src="packages/browser/dart.js"></script>
  </body>
</html>

The nested template #inner will have content of <div>Instance of 'Model'</div>, when it should be empty.
@justinfagnani
Copy link
Contributor Author

I should add that I tried to reproduce this in Polymer.js and couldn't.

@jmesserly
Copy link

Removed Area-Polymer label.
Added Area-Pkg label.

@justinfagnani
Copy link
Contributor Author

So it looks like this is caused by the document not being bootstrapped when loaded. Adding:

    TemplateBindExtension.bootstrap(document);

to main() of the repro fixes the problem.

The lazy calls to _decorate() do not fix the problem, because in the polyfill _decorate() does not call bootstrap() it calls _liftNonNativeChildrenIntoContent(), and bootstrap is what recurses into nested templates. This is why only nested templates don't have their contents removed.

So another fix is to have _decorate() call bootstrap() even for non-native templates. This change also fixes the bug:

Original, in TemplateBindExtension._decorate():

    if (instanceRef != null) {
      // template is contained within an instance, its direct content must be
      // empty
      templateElementExt._templateInstanceRef = instanceRef;
    } else if (liftContents) {
      _liftNonNativeChildrenIntoContent(templateElementExt, _node, liftRoot);
    } else if (bootstrapContents) {
      bootstrap(templateElementExt.content);
    }

Fix:

    if (instanceRef != null) {
      // template is contained within an instance, its direct content must be
      // empty
      templateElementExt._templateInstanceRef = instanceRef;
    } else {
      if (liftContents) {
      _liftNonNativeChildrenIntoContent(templateElementExt, _node, liftRoot);
      }
      bootstrap(templateElementExt.content);
    }

This does not match the JS version at: https://github.com/Polymer/TemplateBinding/blob/master/src/TemplateBinding.js#L392

but, they have already called bootstrap on all templates that were in the document on load, where we haven't.

So I see a few different fixes:

  1. Make the above change and document why we differ from the JS version.
  2. Require that users of template_binding call TemplateBindExtension.bootstrap(document) at the beginning of their app.
  3. Insert that call during code generation.

cc @sigmundch.

@sigmundch
Copy link
Member

Added this to the 1.6 milestone.

@sigmundch
Copy link
Member

Removed this from the 1.6 milestone.
Added Polymer-P-1 label.

@sigmundch
Copy link
Member

Removed Polymer-P-1 label.
Added Polymer-Milestone-Next label.

@sigmundch
Copy link
Member

Added PolymerMilestone-Next label.

@sigmundch
Copy link
Member

Removed Polymer-Milestone-Next label.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Added this to the 1.6 milestone.

@sigmundch
Copy link
Member

Removed this from the 1.6 milestone.

@kasperl
Copy link

kasperl commented Aug 6, 2014

Added this to the 1.6 milestone.

@sigmundch
Copy link
Member

Removed this from the 1.6 milestone.

@sigmundch
Copy link
Member

Removed Priority-High label.
Added Priority-Medium label.

@jmesserly
Copy link

nowadays, i think the fix would be to use the initialization package Jake is working on ... that would give us a standardized way to perform the bootstrap.

However ... here's the thing ... if you've loaded the JS version of template_binding.js, you already got this fix "for free" because it will bootstrap the page. So polymer apps are currently immune (they always have polymer.js 0.5.x) ... and if we fix https://code.google.com/p/dart/issues/detail?id=18893 it won't be needed. going to dupe this against that bug.

cc @jakemac53.

@jmesserly
Copy link

Added Duplicate label.
Marked as being merged into #18893.

@justinfagnani justinfagnani added Type-Defect area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. closed-duplicate Closed in favor of an existing report labels Feb 4, 2015
@jmesserly jmesserly mentioned this issue Jun 3, 2015
Closed
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. closed-duplicate Closed in favor of an existing report
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@justinfagnani @jmesserly @sigmundch @kasperl