1012663 - [SMS] Remove inline style for CSP compliance

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[:gerard-majax]

Confere bug 968907 and bug 858787. We need to remove all CSS inline usage in certified apps.

https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/index.html#L371
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/index.html#L488

Those are in comments, I'm not sure if they are to be considered as an issue.

Comment 1

[Julien Wajsberg [:julienw]]

Actually, line 488 is bug 1006781 already, but kuddo for finding line 371, I missed it.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Julien Wajsberg [:julienw] (away until 2nd June) from comment #1)
> Actually, line 488 is bug 1006781 already, but kuddo for finding line 371, I
> missed it.

Note that line 514 is also an issue (you can't have <style> either).

https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/index.html#L514

Comment 3

[Julien Wajsberg [:julienw]]

Definitely, thanks.

Comment 5

[Julien Wajsberg [:julienw]]

So, the main issues (with commit hashes) are:

https://github.com/mozilla-b2g/gaia/blob/83e6c16dceec422f254c9c966ce6948e849759e6/apps/sms/index.html#L377
https://github.com/mozilla-b2g/gaia/blob/83e6c16dceec422f254c9c966ce6948e849759e6/apps/sms/index.html#L510
https://github.com/mozilla-b2g/gaia/blob/83e6c16dceec422f254c9c966ce6948e849759e6/apps/sms/index.html#L523-L525

Comment 6

[Julien Wajsberg [:julienw]]

As Anthony told me, we should try to use <img> elements instead of background images; but maybe it's not easy given the constraints we have here.

So I'd suggest to only switch to using "style.<xxx> =" in this bug so that we can switch on CSP 1.0 quickly, and in a follow-up try to use <img> elements.

Comment 7

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8438590 [details] [review]
GitHub pull request URL

Hey Julien,

This PR contains two separate commits. The first one addresses CSP compliance issue and the second one is solely related to Attachment.js refactoring as we were discussing previously. I'd rather move second commit to a separate bug\PR as it's not really related to CSP.

Thanks!

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8438590 [details] [review]
GitHub pull request URL

Added comments on github, mostly for the refactoring commit

thanks for separating this in 2 commits, actually maybe we want to keep the commit actually separate so that we can uplift only the fix if necessary ?

Comment 10

[Julien Wajsberg [:julienw]]

Thinking more of it, let's do 2 bugs with these 2 commits, like you suggested.

Comment 11

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

(In reply to Julien Wajsberg [:julienw] from comment #10)
> Thinking more of it, let's do 2 bugs with these 2 commits, like you
> suggested.

Sure, updated PRs and filed follow-up bug 1025552

Comment 12

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8440428 [details] [review]
GitHub pull request URL

Thanks for review! PR is updated.

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8440428 [details] [review]
GitHub pull request URL

r=me with the nit

Comment 14

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> Comment on attachment 8440428 [details] [review]
> GitHub pull request URL
> 
> r=me with the nit

Thanks for review! Nit fixed, travis is greeen, so checkin-needed.

Comment 15

[Carsten Book [:Tomcat]]

master = https://github.com/mozilla-b2g/gaia/commit/ded83da34a42ca0ee2bea69ad27d67afaa3006e2

Comment 16

[Ed Morley [:emorley]]

Unfortunately I've had to revert this for intermittent failures in sms/test/unit/attachment_test.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42051727&tree=Mozilla-Inbound

https://github.com/mozilla-b2g/gaia/commit/77b533771a2d07c27b14691fd47af305e763dda8

Comment 17

[Julien Wajsberg [:julienw]]

Ed, do you see similar issues (120 sec timeout) in other files too?

I suspect that the issue comes from somewhere else.

Comment 18

[Ed Morley [:emorley]]

I haven't seen any other 120s timeouts - it's a different pattern to the other marionette-webapi and gaia-ui-test timeouts I've seen elsewhere

Comment 19

[Oleg Zasypkin [:azasypkin]]

As agreed on IRC, patch has been relanded:

master: https://github.com/mozilla-b2g/gaia/commit/60307b4fc59c8a5ae2a1e87f80827cd43c58c1f9