Improve byline fetching in Readability.js

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
Reader View
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 17
All
Android
Points:
---

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 654202 [details] [diff] [review]
Better byline fetching in Readability

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

5 years ago
OS: Linux → Android
Hardware: x86 → All
Attachment #654202 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 1

5 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?
Is this going to inbound/central first?
https://hg.mozilla.org/mozilla-central/rev/08b790ce10cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
(Assignee)

Comment 4

5 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 5

5 years ago
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.
Attachment #654202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 6

5 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

5 years ago
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/f3a2b3f6cd8b

Updated

5 years ago
status-firefox16: --- → fixed
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.