Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(Angular): properly get node name for svg element wrapper #10170

Closed

Conversation

pkozlowski-opensource
Copy link
Member

Fixes #10078

@googlebot
Copy link

CLAs look good, thanks!

@pkozlowski-opensource
Copy link
Member Author

After thinking about this one some more I don't feel like this is a good solution - I mean - it makes the error go away but I we should probably fix this on the $location level. Closing for now, will send an alternative patch.

@pkozlowski-opensource
Copy link
Member Author

Well, actually, after spending some time trying to write a proper test on the $location level, here is the one I could come up with:

iit('should not throw if svg <use> element is clicked', function () {

      inject(function($rootScope, $compile, $rootElement, $document, $location) {
        // we need to do this otherwise we can't simulate events
        $document.find('body').append($rootElement);
        $document.find('body').append(
            '<svg xmlns="http://www.w3.org/2000/svg">' +
              '<symbol viewBox="0 0 16 16" id="icon">' +
                '<polygon points="16,7 9,7 9,0 7,0 7,7 0,7 0,9 7,9 7,16 9,16 9,9 16,9 "/>' +
              '</symbol></svg>');
        var template = '<svg><use xlink:href="#icon"></use></svg>';
        var element = $compile(template)($rootScope);

        $rootElement.append(element);
        var useEl = element.find('use').eq(0);
        expect(function() {
          browserTrigger(jqLite(useEl[0].instanceRoot), 'click');
        }).not.toThrow();
      });
    });

The trouble is, though, that triggering a fake click event on a node that doesn't have nodeName is rather messy and would require several modifications to the browserTrigger function. I'm not sure we should make those changes for the corner case like this one.

All in all I'm leaning toward declaring my initial fix as "good enough".

@petebacondarwin
Copy link
Member

@pkozlowski-opensource - how about an e2e test? Would that be possible?

@pkozlowski-opensource
Copy link
Member Author

@petebacondarwin I guess so

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

Successfully merging this pull request may close these issues.

Unable to get value of the property 'nodeName': object is null or undefined
3 participants