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

fix(ngSrcset): ignore undefined ng-srcset directive #14493

Closed
wants to merge 3 commits into from
Closed

fix(ngSrcset): ignore undefined ng-srcset directive #14493

wants to merge 3 commits into from

Conversation

rphv
Copy link
Contributor

@rphv rphv commented Apr 22, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
This is a bug fix.

What is the current behavior? (You can also link to an open issue here)
Currently, a TypeError is thrown when data-ng-attr-srcset is undefined.

What is the new behavior (if this is a feature change)?
Now, the attribute is ignored if it's undefined.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

Other information:

Closes #14470

gkalpak
gkalpak reviewed Apr 22, 2016
View reviewed changes
test/ng/directive/ngSrcsetSpec.js
@@ -28,5 +28,12 @@ describe('ngSrcset', function() {
$rootScope.$digest();
expect(element.attr('srcset')).toBe('http://example.com/image1.png 1x,unsafe:javascript:doEvilStuff() 2x');
}));

it('should not throw an error if undefined', inject(function($rootScope, $compile) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't try it, but I am pretty sure this test passes without the fix.
Can you confirm that's inded tha case and change the test, so it fails without the fix ?

Copy link
Contributor Author

@rphv rphv Apr 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right @gkalpak. That test wasn't testing the fix.

I've updated the test and verified that it fails without the fix.

@gkalpak gkalpak added this to the Backlog milestone Apr 22, 2016
@gkalpak
Copy link
Member

gkalpak commented Apr 22, 2016

Thx @rphv for working on this.
Could you address my comment ? (Feel free to ping me once you have, so I can take another look.)

@rphv
Copy link
Contributor Author

rphv commented Apr 23, 2016

Test updated.

On Fri, Apr 22, 2016 at 3:57 AM, Georgios Kalpakas <notifications@github.com

wrote:

Thx @rphv https://github.com/rphv for working on this.
Could you address my comment ? (Feel free to ping me once you have, so I
can take another look.)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14493 (comment)

@gkalpak
Copy link
Member

gkalpak commented Apr 25, 2016

@rphv, since this is not ng-attr- specific (and there is no guarantee that its implementatation will keep exercising the same code paths in the future), could you also add a test that call attrs.$set('srcset', undefined) directly on an <img> element ?

(Also, you don't have to use data- prefixes in tests - they are just adding clutter.)

@rphv rphv force-pushed the srcset-fix-branch branch from 2c6a645 to 8b6d940 Compare April 26, 2016 00:48
@rphv
Copy link
Contributor Author

rphv commented Apr 26, 2016

Thank you for your advice and attention.

I'm not sure if the new test I added is in the right place or in the
accepted style of the project. However, it does fail without the fix.

On Mon, Apr 25, 2016 at 11:01 AM, Georgios Kalpakas <
notifications@github.com> wrote:

@rphv https://github.com/rphv, since this is not ng-attr- specific (and
there is no guarantee that its implementatation will keep exercising the
same code paths in the future), could you also add a test that call attrs.$set('srcset',
undefined) directly on an element ?

(Also, you don't have to use data- prefixes in tests - they are just
adding clutter.)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14493 (comment)

@gkalpak gkalpak closed this in 2310e10 Apr 26, 2016
gkalpak pushed a commit that referenced this pull request Apr 26, 2016
@rphv @gkalpak
fix($compile): properly handle setting srcset to undefined
308f22b 
Previously, calling `Attributes#$set('srcset', value)` on an `<img>` element would throw if `value`
were undefined, as it assumed `value` is always a string.
This commit fixes the issue, by skipping the unnecessary string manipulation when `value` is not
defined.

Closes #14470

Closes #14493
gkalpak pushed a commit that referenced this pull request Apr 26, 2016
@rphv @gkalpak
fix($compile): properly handle setting srcset to undefined
85a53ea 
Previously, calling `Attributes#$set('srcset', value)` on an `<img>` element would throw if `value`
were undefined, as it assumed `value` is always a string.
This commit fixes the issue, by skipping the unnecessary string manipulation when `value` is not
defined.

Closes #14470

Closes #14493
@gkalpak
Copy link
Member

gkalpak commented Apr 26, 2016

Tweaked the test a bit and merged.
Thx for working on this 👍

@rphv rphv deleted the srcset-fix-branch branch April 27, 2016 18:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes component: $compile frequency: low needs: test severity: inconvenient type: bug
Projects
None yet
Milestone
Backlog - won't fix
Development

Successfully merging this pull request may close these issues.

"TypeError: Cannot read property 'split' of undefined" when using ng-attr-srcset with potentially null value
3 participants
@rphv @gkalpak @googlebot