2.92 KB, patch
|Details | Diff | Splinter Review|
1.12 KB, patch
|Details | Diff | Splinter Review|
The reader mode parsed page for http://articles.latimes.com/2012/mar/27/health/la-he-green-coffee-weight-loss-20120328 has a large empty space before the article content. It also shows the image caption at the top, which don't want (unless we include the image).
Caused by a style attribute on a div from the original article. We need to make sure to strip any style from the original content.
Have a patch for this one.
cleanStyles was simply not working as expected because of the experimental bits in the original Readability.js code. We don't need those "readability-styled" markers. Fixes this specific issue on latimes.com but also fixes rendering issues on other sites (e.g. slate.com)
Attachment #652108 - Flags: review?(bnicholson)
I noticed this adds extra spacing between arbitrary paragraphs on some articles: * http://m.yahoo.com/w/legobpengine/news/blogs/lookout/russian-punk-band-verdict-found-guilty-hooliganism-115937812.html * http://m.yahoo.com/w/legobpengine/news/blogs/lookout/west-nile-aerial-attack-creates-controversy-virus-stricken-133758732.html There may be other unforeseen side effects of removing this - let's get a test framework working to make sure this change is safe.
Comment on attachment 652108 [details] [diff] [review] Fix _cleanStyles() to clear all explicit style attributes r- until we can figure out the spacing issue
Attachment #652108 - Flags: review?(bnicholson) → review-
(In reply to Brian Nicholson (:bnicholson) from comment #6) > I noticed this adds extra spacing between arbitrary paragraphs on some > articles: > > * > http://m.yahoo.com/w/legobpengine/news/blogs/lookout/russian-punk-band- > verdict-found-guilty-hooliganism-115937812.html > * > http://m.yahoo.com/w/legobpengine/news/blogs/lookout/west-nile-aerial-attack- > creates-controversy-virus-stricken-133758732.html > > There may be other unforeseen side effects of removing this - let's get a > test framework working to make sure this change is safe. This new (and simpler) patch fixes the issue without causing regressions on the websites above.
Attachment #655005 - Flags: review?(mark.finkle) → review+
Comment on attachment 655005 [details] [diff] [review] Pass the right node to have styles cleaned after parsing [Approval Request Comment] User impact if declined: Reader might show broken layout in content because we're not cleaning style attributes from original site. This might have relatively often. Testing completed (on m-c, etc.): Local testing in a long list of websites, no regressions found. Risk to taking this patch (and alternatives if risky): Very low, one line change to run a function on the correct node. String or UUID changes made by this patch: None.
Attachment #655005 - Flags: approval-mozilla-aurora?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Comment on attachment 655005 [details] [diff] [review] Pass the right node to have styles cleaned after parsing [Triage Comment] Low risk, mobile only, approving for Beta 16.
Attachment #655005 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/adce83c9dc61
Verified on: Firefox Mobile 16 beta 2, Nightly 18.0a1 2012-09-04 and Aurora 17.0a2 2012-09-04 Samsung Galaxy Tab (Android 2.3.3)
You need to log in before you can comment on or make changes to this bug.