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.

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

fix(ivy): handle SafeStyles in [style.prop] correctly #34286

Closed
wants to merge 3 commits into from

Conversation

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Dec 6, 2019

Prior to this commit, values wrapped into SafeStyle were not handled correctly in [style.prop] bindings in case style sanitizer is present (when template contains some style props that require sanitization). Style sanitizer was not unwrapping values in case a given prop doesn't require sanitization.As a result, wrapped values were used as final styling values (see styling/bindings.ts). This commit updates the logic to unwrap safe values in sanitizer in case no sanitization is required.

This PR closes #34023.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
Prior to this commit, values wrapped into SafeStyle were not handled correctly in [style.prop] bindings in case style sanitizer is present (when template contains some style props that require sanitization). Style sanitizer was not unwrapping values in case a given prop doesn't require sanitization.As a result, wrapped values were used as final styling values (see https://github.com/angular/angular/blob/master/packages/core/src/render3/styling/bindings.ts#L620). This commit updates the logic to unwrap safe values in sanitizer in case no sanitization is required.
@AndrewKushnir AndrewKushnir marked this pull request as ready for review Dec 6, 2019
@AndrewKushnir AndrewKushnir requested review from angular/fw-core as code owners Dec 6, 2019
Copy link
Member

IgorMinar left a comment

lgtm, but please improve the tests as suggested.

@@ -1563,6 +1564,58 @@ describe('styling', () => {
expect(html).toMatch(/style=["|']clip-path:\s*url\(.*#test.*\)/);
});

it('should not throw when bound to SafeValue', () => {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 9, 2019

Member

can you please split this into two specs - one for sanitized properties (e.g. background, background-image, or border-image) and one for regular properties (e.g. height).

These two types of properties are handled differently, and we should have explicit tests for both.

alternatively, since it looks like the setup is quite expensive/verbose, you can also separate the [style.xxx] bindings in the template into two distinct groups (two divs or groups of lines separated with a blank line perhaps?) with an inline comment describing both.

This comment has been minimized.

Copy link
@AndrewKushnir

AndrewKushnir Dec 10, 2019

Author Contributor

Thanks for the review @IgorMinar. I've updated the test to separate props by groups and added comments in a template to make it more explicit. Thank you.

@@ -196,7 +196,7 @@ export const ɵɵdefaultStyleSanitizer =
}

if (mode & StyleSanitizeMode.SanitizeOnly) {
return doSanitizeValue ? ɵɵsanitizeStyle(value) : value;
return doSanitizeValue ? ɵɵsanitizeStyle(value) : unwrapSafeValue(value);

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 9, 2019

Member

this change is right, but the surrounding code is super complex without any obvious reason for this complexity. :-(

I'll ask @matsko and @mhevery if they could simplify it in the future.

This comment has been minimized.

Copy link
@matsko

matsko Dec 10, 2019

Member

The reasoning for this is because styleMap() cannot figure out ahead of time whether or not to sanitize a given property--it can only be figured out during runtime (which is why we need the big if statement above).

If this were a class then we could have two methods that do validation + sanitization (which would make it easier to follow), but we need to stick to functions because of runtime bundle size concerns.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 10, 2019

Member

@matsko couldn't we have a private utility function (e.g. isSanitizationRequired(propName: string): boolean) that is used by both the ɵɵdefaultStyleSanitizer and styleMap directly?

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Dec 10, 2019

@matsko

This comment has been minimized.

Copy link
Member

matsko commented Dec 10, 2019

LGTM

@matsko
matsko approved these changes Dec 10, 2019
AndrewKushnir added a commit that referenced this pull request Dec 10, 2019
Prior to this commit, values wrapped into SafeStyle were not handled correctly in [style.prop] bindings in case style sanitizer is present (when template contains some style props that require sanitization). Style sanitizer was not unwrapping values in case a given prop doesn't require sanitization.As a result, wrapped values were used as final styling values (see https://github.com/angular/angular/blob/master/packages/core/src/render3/styling/bindings.ts#L620). This commit updates the logic to unwrap safe values in sanitizer in case no sanitization is required.

PR Close #34286
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
Prior to this commit, values wrapped into SafeStyle were not handled correctly in [style.prop] bindings in case style sanitizer is present (when template contains some style props that require sanitization). Style sanitizer was not unwrapping values in case a given prop doesn't require sanitization.As a result, wrapped values were used as final styling values (see https://github.com/angular/angular/blob/master/packages/core/src/render3/styling/bindings.ts#L620). This commit updates the logic to unwrap safe values in sanitizer in case no sanitization is required.

PR Close angular#34286
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 10, 2020

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.