Closed Bug 784674 Opened 7 years ago Closed 7 years ago

Improve byline fetching in Readability.js

Categories

(Firefox for Android :: Reader View, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 17
Tracking Status
firefox16 --- fixed
firefox17 --- fixed

People

(Reporter: lucasr, Assigned: lucasr)

Details

Attachments

(1 file)

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)
OS: Linux → Android
Hardware: x86 → All
Attachment #654202 - Flags: review?(bnicholson) → review+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
(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.
Attachment #654202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
You need to log in before you can comment on or make changes to this bug.