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

replace style binding for z-index with CSS variable #272

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jelhan
Copy link

@jelhan jelhan commented Apr 15, 2020

This removes the style binding used to set the z-index of the container. The default value is set with CSS instead. Default value could be customized with a CSS variable if needed. If consumer prefers to set it on invocation, it could be set as any other HTML attribute using angle bracket component invocation:

<NotificationContainer style="z-index: 9999;" />

This should be preferred over style bindings due to security reasons. Especially style bindings requires an style-src: unsafe-inline directive Content Security Policy, which is bad for security reasons.

The pull request also adds test coverage to prevent a regression regarding CSP compliance. It does so using Ember CLI Content Security Policy, which provides test support in it's v2 release. It could be used to test compliance with a strict CSP. If a test violates the configured CSP it will fail. The addon comes with reasonable defaults and does not require any configuration for this use case.

This is a breaking change and requires a major release.

jelhan added 2 commits April 15, 2020 09:39
Set default value with CSS class instead. Default value could be
customized with CSS variable if needed. If consumer prefers to set it on
invocation, it could be set as any other HTML attribute using angle bracket
component invocation:

```hbs
<NotificationContainer style="z-index: 9999;" />
```

This should be preferred over style bindings due to security reasons.
Especially style bindings requires an `style-src: unsafe-inline` directive
Content Security Policy, which is bad for security reasons.
Ember CLI Content Security Policy ships test support in it's upcoming v2
release. It could be used to test compliance with a strict CSP. If a
test violates the configured CSP it will fail. The addon comes with
reasonable defaults and does not require any configuration for this use
case.
@jelhan jelhan force-pushed the remove-z-index-style-binding branch from 666dc52 to de318da Compare April 15, 2020 10:36
@jelhan
Copy link
Author

jelhan commented Apr 15, 2020

There are still inline styles used:

<div class="c-notification__countdown" style={{notificationClearDuration}}></div>

Seems like this code-path is not triggered by tests. Otherwise CI would have told us.

Even so this pull request is not fixing all CSP issues I think it could be merged. Fixing it for the animation on countdown is not as straight-forward as this one. It's actually dynamic. So need another solution, which may be better discussed separately.

@pichfl pichfl mentioned this pull request May 21, 2021
pichfl added a commit to pichfl/ember-cli-notifications that referenced this pull request Dec 10, 2021
- simplify component class
- move @z-index@ handling to CSS custom property

Replaces mansona#272
pichfl added a commit to pichfl/ember-cli-notifications that referenced this pull request Dec 15, 2021
- simplify component class
- move @z-index@ handling to CSS custom property

Replaces mansona#272
pichfl added a commit to pichfl/ember-cli-notifications that referenced this pull request Dec 15, 2021
- simplify component class
- move @z-index@ handling to CSS custom property

Replaces mansona#272
pichfl added a commit to pichfl/ember-cli-notifications that referenced this pull request Jun 8, 2022
- simplify component class
- move @z-index@ handling to CSS custom property

Replaces mansona#272
pichfl added a commit to pichfl/ember-cli-notifications that referenced this pull request Jun 8, 2022
- simplify component class
- move @z-index@ handling to CSS custom property

Replaces mansona#272
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant