search

angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.85k stars 27.52k forks source link

Transclusion doesn't handle changes to the content and doesn't allow CSS selectors #15330

Open firelizzard18 opened 7 years ago

firelizzard18 commented 7 years ago

Do you want to request a feature or report a bug? Mostly a feature, kind of a bug

What is the current behavior? Content is transcluded in the way I expect, but an error is thrown and angular doesn't properly bind to the transcluded content.

Minimal reproduction of the problem with instructions Main directive:

directive('myDirective', function () {
    return {
        restrict: 'EA',
        scope: {},
        bindToController: {
            ...
        },
        controllerAs: 'ctrl',
        templateUrl: ...,
        link: linkfn,
        controller: ctrlfn,
        transclude: {
            'slotA': 'sectionA',
            'slotB': 'sectionB'
        }
    }

    function linkfn(scope, elem, attrs, ctrl) {
        ...
    }

    function ctrlfn($scope, $element) {
        ...
    }
})

Template for main directive:

...
<section class="..." my-directive-transclude="slotA"></section>
<section class="..." my-directive-transclude="slotB"></section>
...

Subsidiary directive:

directive('myDirectiveTransclude', function () {
    return {
        restrict: 'A',
        require: '^myDirective',
        scope: false,
        link: function (scope, elem, attrs, ctrl, transclude) {
            var slot = attrs.myDirectiveTransclude

            transclude(function (clone) {
                elem.empty()
                elem.append(clone.children())
            }, elem, slot)
        }
    }
})

The page:

...
<my-directive>
    <section-a>
        content
    </section-a>
    <section-b>
        content
    </section-b>
</my-directive>
...

causes

TypeError: Cannot read property 'childNodes' of undefined
    at nodeLinkFn (http://.../angular.js:9330:58)
    at compositeLinkFn (http://.../angular.js:8620:13)
    at compositeLinkFn (http://.../angular.js:8623:13)
    at compositeLinkFn (http://.../angular.js:8623:13)
    at publicLinkFn (http://.../angular.js:8500:30)
    at lazyCompilation (http://.../angular.js:8844:25)
    at boundTranscludeFn (http://.../angular.js:8637:16)
    at controllersBoundTransclude (http://.../angular.js:9377:22)
    at Object.link (http://.../my-script.js:##:##)
    at http://.../angular.js:1247:18

What is the expected behavior? Content is transcluded in the way I expect and everything works.

What is the motivation / use case for changing the behavior? I want to be able to transclude section-a and section-b into my-directive, but I want real tags and I don't want things nested obnoxiously.

Please tell us about your environment: Visual Studio 2013, Nuget, Windows 10, IIS 7.5 (or whatever comes with win 10).

gkalpak commented 7 years ago

Can you create a live reproduction (e.g. using CodePen, Plnkr etc), so we can investigate this further?

firelizzard18 commented 7 years ago

http://plnkr.co/edit/xPQG5YJvT5JhHtZUvTTY?p=preview

I made a patch to $compile on my server. If there's no slotName that matches the normalized tag name, then I check for any elementSelectors that start with $. I treat everything after the $ as a CSS selector and check if $(node).is(elementSelector.substring(1)). This is not exactly what I want, because I still have an extra element in my DOM, but at least I don't have any bogus non-html elements present.

gkalpak commented 7 years ago

I see. The problem is that linking makes certain assumptions regarding the structure of the compiled DOM. What will work (afaict) is appending clone.children() asynchronously; i.e. after the linking phase. E.g.:

// Change:
elem.append(clone.children());

// To:
scope.$evalAsync(function() { elem.append(clone.children()); });

Here is a simplified demo, but it should work on your demo too.

There is one minor caveat though: Breaking the synchronous nature of linking might break other directives' assumptions. This is an edge-case and unlikely to affect any real usecases (especially on directives using templateUrl, which are already asynchronous), but it is not impossible.

Does this work for you?

BTW, the subject of allowing more flexibility in matching elements to be transcluded (e.g. matching against attributes or classes as well) has been discussed when the multi-slot transclusion was implemented. Since we can't rely on features like jQuery#is() or Element#matches(), adding more flexibility would also increase the complexity. Therefore, we had decided to implement the simple, less flexible nodeName matching only and consider adding more flexibility when/if specific usecases arise.

Off the top of head, trying to avoid adding too much complexity and loc for a feature that will only benefit few people, the following might be a good compromize:

Allow the values of the transclude property to also be "matcher" functions (apart from a normalized nodeName). Each matcher would be called for every node transcluded node (that isn't matched against another slot) and deciding whether the node is a match for the corresponding slot (see example below). This way, the user would be able to use any criterion they wish and also use features (such as jQuery#is() or Element#matches()) that they know are available on the environements they support.

Example:

// Definition of `myComponent`
.component('myComponent', {
  template: `
    <section ng-transclude="slotA"></section>
    <section ng-transclude="slotB"></section>
  `,
  transclude: {
    slotA: node => node.hasAttribute('slot-a'),
    slotB: node => node.classList.contains('slot-b')
  }
})
<!-- Usage of `myComponent` -->
<my-component>
  <header slot-a>Slot A</header>
  <header class="slot-b">Slot B</header>
  <footer slot-a>...A...</footer>
  <footer class="slot-b">...B...</footer>
</my-component>
<!-- Resulting HTML -->
<my-component>
  <section ng-transclude="slotA">
    <header slot-a>Slot A</header>
    <footer slot-a>...A...</footer>
  </section>
  <section ng-transclude="slotB">
    <header class="slot-b">Slot B</header>
    <footer class="slot-b">...B...</footer>
  </section>
</my-component>
firelizzard18 commented 7 years ago

Awesome, thanks! The async append works perfectly, and I think the idea of using a function selector is great! And it certainly covers my usage.

I added var selectorMap = createMap(); immediately after slotMap's initialization.

I made the following patch to forEach($compileNode.contents(), function(node) {:

for (var slotName in selectorMap) {
    var elementSelector = selectorMap[slotName];
    if (!angular.isFunction(elementSelector))
        continue;
    if (!elementSelector(node))
        continue;
    filledSlots[slotName] = true;
    slots[slotName] = slots[slotName] || [];
    slots[slotName].push(node);
}

And in forEach(directiveValue, function(elementSelector, slotName) {, I replaced this:

var optional = (elementSelector.charAt(0) === '?');
elementSelector = optional ? elementSelector.substring(1) : elementSelector;

slotMap[elementSelector] = slotName;

With this:

var optional = false;
if (angular.isString(elementSelector)) {
    optional = (elementSelector.charAt(0) === '?');
    elementSelector = optional ? elementSelector.substring(1) : elementSelector;

    slotMap[elementSelector] = slotName;
} else if (angular.isFunction(elementSelector)) {
    optional = !!elementSelector.optional;

    selectorMap[slotName] = elementSelector;
}

Here's the full diff:

diff --git a/.../angular.js b/.../angular.js
index abcdef..012346 100644
--- a/.../angular.js
+++ b/.../angular.js
@@ -9017,15 +9017,25 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
               $template = [];

               var slotMap = createMap();
+              var selectorMap = createMap();
               var filledSlots = createMap();

               // Parse the element selectors
               forEach(directiveValue, function(elementSelector, slotName) {
                 // If an element selector starts with a ? then it is optional
-                var optional = (elementSelector.charAt(0) === '?');
-                elementSelector = optional ? elementSelector.substring(1) : elementSelector;
+                // PATCH START
+                var optional = false
+                if (angular.isString(elementSelector)) {
+                    optional = (elementSelector.charAt(0) === '?');
+                    elementSelector = optional ? elementSelector.substring(1) : elementSelector;

-                slotMap[elementSelector] = slotName;
+                    slotMap[elementSelector] = slotName;
+                } else if (angular.isFunction(elementSelector)) {
+                    optional = !!elementSelector.optional;
+
+                    selectorMap[slotName] = elementSelector;
+                }
+                // PATCH END

                 // We explicitly assign `null` since this implies that a slot was defined but not filled.
                 // Later when calling boundTransclusion functions with a slot name we only error if the
@@ -9039,6 +9049,18 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

               // Add the matching elements into their slot
               forEach($compileNode.contents(), function(node) {
+                // PATCH START
+                for (var slotName in selectorMap) {
+                    var elementSelector = selectorMap[slotName];
+                    if (!angular.isFunction(elementSelector))
+                        continue;
+                    if (!elementSelector(node))
+                        continue;
+                    filledSlots[slotName] = true;
+                    slots[slotName] = slots[slotName] || [];
+                    slots[slotName].push(node);
+                }
+                // PATCH END
                 var slotName = slotMap[directiveNormalize(nodeName_(node))];
                 if (slotName) {
                   filledSlots[slotName] = true;
gkalpak commented 7 years ago

Glad "async append" works for you :smiley: Would you like to open a PR with the matcher/selector function feature and continue the discussion there?