25.59 KB, image/jpeg
47 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
Created attachment 8507523 [details] The HTML5 doctype used in an HTML email is visible. User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.104 Safari/537.36 Steps to reproduce: I used PutsMail (https://putsmail.com/) to send an HTML email test with an HTML5 doctype. I used the following code: <!doctype html> <title>Test</title> <h1>Hello World</h1> Actual results: In Firefox OS 220.127.116.11 simulator, the e-mail app shows the HTML5 doctype before the rest of the email content. Expected results: The doctype of an HTML email should not appear on screen. This bug only occurs when using an HTML5 doctype. For example, using an XHTML 1.0 Transitionnal doctype doesn't make it visible before the email content.
Hey Andrew, is it something easy to fix?
(In reply to Julien Wajsberg [:julienw] from comment #1) > Hey Andrew, is it something easy to fix? Hah, so, I was going to provide you pointers about how to fix this since I'm on quasi-vacation this week (on vacation but working a little; would have gotten to this next week when I get back), but while providing you the pointers I found the bug. Our poor man's doctype regex at: https://github.com/mozilla-b2g/bleach.js/blob/worker-thread-friendly/lib/bleach.js#L70 is case sensitive. The HTML5 spec explicitly says the doctype is case-insensitive. So we need to add an 'i' flag. If you were interested in fixing an email bug, let me know and I'll help find you a trickier one!
Created attachment 8510482 [details] [review] Make the doctype regex case-insensitive and add a variety of full and snippet tests
haha :) Do you think it's worth uplifting to v2.1?
Comment on attachment 8510482 [details] [review] Make the doctype regex case-insensitive and add a variety of full and snippet tests [Approval Request Comment] [Bug caused by] (feature/regressing bug #): Not a regression. [User impact] if declined: Users may see "<!doctype HTML>" or something like that at the start of HTML messages, including their snippet. It depends on the doctype string used by the author of the message (or more likely), their software. [Testing completed]: New coverage of previously failing cases in bleach.js mocha tests for both snippet and full body sanitization. [Risk to taking this patch] (and alternatives if risky): Considered very low risk because it's a single character change to make a regexp case-insensitive. [String changes made]: No string changes made.
Comment on attachment 8510482 [details] [review] Make the doctype regex case-insensitive and add a variety of full and snippet tests Do not understand why this would be a blocker, and at this point in 2.1 we want to avoid unwanted code-churn and fix only blocking bugs, so this will have to wait for 2.2 unless there is a strong reason I am missing to understand to make this a 2.1+ blocker.
Merged in gaia master: https://github.com/mozilla-b2g/gaia/commit/5904450bdd364f19628322ae108b67499ade5409 from pull request: https://github.com/mozilla-b2g/gaia/pull/25474