fix(compile): do not create directives for constant media URL attributes #16737

petebacondarwin · 2018-10-18T14:04:34Z

By creating attribute directives that watch the value of
media url attributes (e.g. img[src]) we caused a conflict
when both src and data-src were appearing on the
same element. As each directive was trying to write to the
attributes on the element, where AngularJS treats src and
data-src as synonymous.

This commit ensures that we do not create create such directives
when the media url attribute is a constant (no interpolation).

Because of this (and because we no longer sanitize URLs in the
$attr.$set() method, this commit also updates ngHref and
ngSrc to do a preliminary sanitization of URLs in case there
is no interpolation in the attribute value.

googlebot · 2018-10-18T14:04:36Z

☹️ Sorry, but only Googlers may change the label cla: yes.

jbedard · 2018-10-18T15:30:42Z

src/ng/compile.js

@@ -3871,6 +3871,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
        compile: function() {
            return {
              pre: function attrInterpolatePreLinkFn(scope, element, attr) {
+                if (trustedContext === $sce.MEDIA_URL && isConstantInterpolation(interpolateFn)) {
+                  // Constant MEDIA_URL attributes should just be sanitized and not watched.
+                  attr[name] = interpolateFn(scope);


Do we still need this assignment? I thought to go back to the previous behavior we'd want to do no assignments.

I thought we'd just update this condition to return early like it previously did...

This was the only way to keep all the tests passing. If you change the condition you linked to then a load of other ngSrc etc related tests fail. This is due to the fact that the url sanitization has moved to the interpolation rather than being done in the compiler.

Oh :/

And we can't do this if + return outside the directive? Before doing directives.push...

No because we need to set the sanitized value to the attribute at least once.

fbrandel · 2018-10-22T15:45:46Z

When I run the fix locally, it seems that the ng-attr-data-src directive on images now stopped working. The attribute is not interpolated/watched, but it should. Unfortunately I was not able to provide a test case for this, just noticed it by manually trying it out.

The original reported bug was fixed though :)

petebacondarwin · 2018-10-23T07:32:28Z

@fbrandel - I don't think what you are suggesting ever worked with Angular. The following variants on ng-attr work: ng-attr-src and data-ng-attr-src but ng-attr-data-src does not work. See http://next.plnkr.co/edit/sbrIYJYyKE0S7XVA?preview

fbrandel · 2018-10-23T08:24:22Z

@petebacondarwin ng-attr-data-src should bind to the data-src attribute, not the src attribute. So the plunkr shows no image (which is correct) but an interpolated data-src attribute.

With the PR applied this stopped working, as no data-src attribute is present on the element.

petebacondarwin · 2018-10-23T09:04:50Z

Ahah! Let me see if I can reproduce in a unit test...

petebacondarwin · 2018-10-23T09:43:45Z

@fbrandel I added unit tests for this case but I cannot reproduce your issue. Please can you take a look and see if you can do so?

fbrandel · 2018-10-23T15:15:20Z

@petebacondarwin After building the project with the PR applied, I can see the tests pass.

fbrandel · 2018-10-24T10:55:47Z

@petebacondarwin Sorry to come up with more investigation ;) It seems that the combination of src and ng-attr-data-src attributes on an image now behaves differently. With a src attribute set on the image, the data-src does not contain the value of the ng-attr-data-src expression but the value of src.

Plunkr: https://next.plnkr.co/edit/mMn0sv47O81Hi0FJ

Unit Test:

it('should compile img with constant [src]-attribute and [ng-attr-data-src] attribute', inject(function() {
        $rootScope.name = 'some-image.png';
        element = $compile('<img src="constant.png" ng-attr-data-src="{{name}}">')($rootScope);
        expect(element.attr('data-src')).toBeUndefined();

        $rootScope.$digest();
        expect(element.attr('src')).toBe('constant.png');
        expect(element.attr('data-src')).toBe('some-image.png');

        $rootScope.name = 'other-image.png';
        $rootScope.$digest();
        expect(element.attr('src')).toBe('constant.png');
        expect(element.attr('data-src')).toBe('other-image.png');
}));

petebacondarwin · 2018-10-24T11:11:03Z

Hmm, oh dear. This is a can of worms...

petebacondarwin · 2018-10-24T20:48:30Z

@fbrandel - can you take a look at this alternative approach? See if you can break this one :-)

fbrandel · 2018-10-25T09:18:07Z

@petebacondarwin Looking good with my scenarios! Thank you so far for taking care!

mgol · 2018-10-25T10:01:11Z

test/ng/compileSpec.js

@@ -12109,6 +12118,47 @@ describe('$compile', function() {
        expect(element.attr('dash-test4')).toBe('JamieMason');
      }));

+      it('should work with img[src]', inject(function() {


It tests ng-attr-src, not src. The same remark for the next test.

Narretz · 2018-10-25T11:58:51Z

We are now setting the attribute twice, even for interpolated attributes, which is not that great imo. Can we detect if the attribute value has not been interpolated?

gkalpak · 2018-10-25T13:59:47Z

src/ng/directive/attrs.js

@@ -422,6 +422,10 @@ forEach(['src', 'srcset', 'href'], function(attrName) {
          propName = null;
        }

+        // We need to sanitize the url at least once, in case it is a constant
+        // non-interpolated attribute.
+        attr.$set(normalized, $sce.getTrustedMediaUrl(attr[normalized]));


Maybe we could check the current value and only call $set() if getTrustedMediaUrl() return a different value.

…ributes By creating attribute directives that watch the value of media url attributes (e.g. `img[src]`) we caused a conflict when both `src` and `data-src` were appearing on the same element. As each directive was trying to write to the attributes on the element, where AngularJS treats `src` and `data-src` as synonymous. This commit ensures that we do not create create such directives when the media url attribute is a constant (no interpolation). Because of this (and because we no longer sanitize URLs in the `$attr.$set()` method, this commit also updates `ngHref` and `ngSrc` to do a preliminary sanitization of URLs in case there is no interpolation in the attribute value. Fixes angular#16734

…URL attributes

petebacondarwin added type: bug component: $compile frequency: low cla: yes needs: review severity: regression labels Oct 18, 2018

petebacondarwin added this to the 1.7.x milestone Oct 18, 2018

googlebot removed the cla: yes label Oct 18, 2018

googlebot added the cla: yes label Oct 18, 2018

petebacondarwin self-assigned this Oct 18, 2018

petebacondarwin mentioned this pull request Oct 18, 2018

With AngularJS 1.7 the data-src attribute of an image gets automatically replaced #16734

Closed

1 task

jbedard reviewed Oct 18, 2018

View reviewed changes

petebacondarwin force-pushed the issue-16734 branch from 774a336 to fbf5eb3 Compare October 24, 2018 20:46

petebacondarwin changed the title ~~fix(compile): do not watch constant media attributes~~ fix(compile): do not create direcrtives for constant media URL attributes Oct 24, 2018

petebacondarwin changed the title ~~fix(compile): do not create direcrtives for constant media URL attributes~~ fix(compile): do not create directives for constant media URL attributes Oct 24, 2018

petebacondarwin force-pushed the issue-16734 branch from fbf5eb3 to ab77738 Compare October 24, 2018 20:47

petebacondarwin force-pushed the issue-16734 branch from ab77738 to 4a3f356 Compare October 24, 2018 20:49

mgol reviewed Oct 25, 2018

View reviewed changes

gkalpak approved these changes Oct 25, 2018

View reviewed changes

gkalpak mentioned this pull request Nov 20, 2018

Problem with ngSrc and src order (with 1.7.x) #16766

Closed

3 tasks

petebacondarwin added 3 commits November 20, 2018 08:44

fixup! fix(interpolate): do not create directives for constant media …

9688941

…URL attributes

fixup! fix(interpolate): do not create directives for constant media …

0687a4f

…URL attributes

petebacondarwin force-pushed the issue-16734 branch from 6393def to 0687a4f Compare November 20, 2018 08:53

petebacondarwin merged commit 0687a4f into angular:master Nov 20, 2018

fix(compile): do not create directives for constant media URL attributes #16737

fix(compile): do not create directives for constant media URL attributes #16737

Uh oh!

Conversation

petebacondarwin commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Oct 18, 2018

Uh oh!

jbedard Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbedard Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

fbrandel commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin commented Oct 23, 2018

Uh oh!

fbrandel commented Oct 23, 2018

Uh oh!

petebacondarwin commented Oct 23, 2018

Uh oh!

petebacondarwin commented Oct 23, 2018

Uh oh!

fbrandel commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbrandel commented Oct 24, 2018

Uh oh!

petebacondarwin commented Oct 24, 2018

Uh oh!

petebacondarwin commented Oct 24, 2018

Uh oh!

fbrandel commented Oct 25, 2018

Uh oh!

mgol Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

Narretz commented Oct 25, 2018

Uh oh!

gkalpak Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petebacondarwin commented Oct 18, 2018 •

edited

Loading

petebacondarwin Oct 18, 2018 •

edited

Loading

fbrandel commented Oct 22, 2018 •

edited

Loading

fbrandel commented Oct 23, 2018 •

edited

Loading