Large empty space in parsed article

VERIFIED FIXED in Firefox 16

Status

()

defect
P2
normal
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: bnicholson, Assigned: lucasr)

Tracking

unspecified
Firefox 17
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox16- verified, firefox17 verified, firefox18 verified)

Details

Attachments

(2 attachments)

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).
Err, wrong blocker.
Blocks: 777557
No longer blocks: 723713
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.
Assignee: nobody → bnicholson
Assignee: bnicholson → lucasr.at.mozilla
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)
no need to track for release, just nominate fix for uplift when ready.
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?
Priority: -- → P2
No longer blocks: 777557
https://hg.mozilla.org/mozilla-central/rev/6edc438cb29a
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+
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.