Closed
Bug 784674
Opened 9 years ago
Closed 9 years ago
Improve byline fetching in Readability.js
Categories
(Firefox for Android Graveyard :: Reader View, defect)
Tracking
(firefox16 fixed, firefox17 fixed)
RESOLVED
FIXED
Firefox 17
People
(Reporter: lucasr, Assigned: lucasr)
Details
Attachments
(1 file)
3.22 KB,
patch
|
bnicholson
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Somehow missed Brian's suggestion to remove byline node to avoid duplicate bylines in reader. Also, we need to match both id and class. Using textContent instead of stripping tags should be enough.
Attachment #654202 -
Flags: review?(bnicholson)
Assignee | ||
Updated•9 years ago
|
OS: Linux → Android
Hardware: x86 → All
Updated•9 years ago
|
Attachment #654202 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 1•9 years ago
|
||
Comment on attachment 654202 [details] [diff] [review] Better byline fetching in Readability [Approval Request Comment] User impact if declined: Some articles contain byline info inside main content DIV and we'll show duplicate byline for those. Testing completed (on m-c, etc.): A lot of local testing, no regressions. Risk to taking this patch (and alternatives if risky): Low, should only affect pages with byline in main content DIV, which is not that common. String or UUID changes made by this patch: None.
Attachment #654202 -
Flags: approval-mozilla-aurora?
Comment 2•9 years ago
|
||
Is this going to inbound/central first?
Comment 3•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/08b790ce10cd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] from comment #2) > Is this going to inbound/central first? Sorry, forgot to post the inbound push link. Just landed in m-c.
Comment on attachment 654202 [details] [diff] [review] Better byline fetching in Readability Review of attachment 654202 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/Readability.js @@ +437,5 @@ > continue; > > + let matchString = node.className + node.id; > + if (matchString.search(this.REGEXPS.byline) !== -1 && !this._articleByline) { > + this._articleByline = node.textContent; This later flows to _creditsElement.innerHTML, which would be weird if there were any < and > in the byline.
Updated•9 years ago
|
Attachment #654202 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to microrffr from comment #5) > Comment on attachment 654202 [details] [diff] [review] > Better byline fetching in Readability > > Review of attachment 654202 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/chrome/content/Readability.js > @@ +437,5 @@ > > continue; > > > > + let matchString = node.className + node.id; > > + if (matchString.search(this.REGEXPS.byline) !== -1 && !this._articleByline) { > > + this._articleByline = node.textContent; > > This later flows to _creditsElement.innerHTML, which would be weird if there > were any < and > in the byline. Good point. Filed bug 785549 to track this.
Assignee | ||
Comment 7•9 years ago
|
||
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/f3a2b3f6cd8b
Updated•9 years ago
|
status-firefox16:
--- → fixed
status-firefox17:
--- → fixed
Updated•4 months ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•