777562 - Large empty space in parsed article

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P2)

Description

[Brian Nicholson (:bnicholson)]

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).

Comment 1

[Brian Nicholson (:bnicholson)]

Err, wrong blocker.

Comment 2

[Lucas Rocha (:lucasr)]

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.

Comment 3

[Lucas Rocha (:lucasr)]

Have a patch for this one.

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Fix _cleanStyles() to clear all explicit style attributes

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)

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

no need to track for release, just nominate fix for uplift when ready.

Comment 6

[Brian Nicholson (: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 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 652108 [details] [diff] [review]
Fix _cleanStyles() to clear all explicit style attributes

r- until we can figure out the spacing issue

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Pass the right node to have styles cleaned after parsing

Comment 9

[Lucas Rocha (:lucasr)]

(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.

Comment 10

[Lucas Rocha (:lucasr)]

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6edc438cb29a

Comment 11

[Lucas Rocha (:lucasr)]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6edc438cb29a

Comment 13

[Alex Keybl [:akeybl]]

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.

Comment 14

[Lucas Rocha (:lucasr)]

Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/adce83c9dc61

Comment 15

[Paul Feher]

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)