olsh / Feedly-Notifier

Google Chrome, Firefox, Opera and Microsoft Edge extension for reading news from RSS aggregator Feedly
http://olsh.github.io/Feedly-Notifier/
Mozilla Public License 2.0
273 stars 38 forks source link

Clicking a link inside a feed's content inside the popup opens blank/home page #84

Closed ThiefMaster closed 7 years ago

ThiefMaster commented 7 years ago

Happens both with left and middle click.

olsh commented 7 years ago

@ThiefMaster , Please specify your browser. I can't reproduce it on Firefox 53 and Chrome 58 on Window 10.

ThiefMaster commented 7 years ago

Firefox 53

ThiefMaster commented 7 years ago

I just checked with the debugger and after var url = link.data("link");, url is still undefined. link.data() is an empty object.

ThiefMaster commented 7 years ago

Found the problem. data-link is not set on links that are part of a feed's content. Running this in the console fixed it:

$('.content a').filter(function() {
    return $(this).data('link') === undefined
}).each(function() {
    $(this).data('link', $(this).attr('href'));
})

As a proper fix I'd suggest this (untested):

-var url = link.data("link")
+var url = link.data("link") || link.attr('href')

Somewhat unrelated, but I believe var isFeed = link.hasClass("title") && $("#feed").is(":visible"); might breakf if someone has class=title in their feed.

olsh commented 7 years ago

I just checked with the debugger and after var url = link.data("link");, url is still undefined. link.data() is an empty object.

Hmm, strange, I've just tested it on the feed https://www.yahoo.com/news/rss/topstories and it works fine. Could you please provide RSS feed for reproducing?

ThiefMaster commented 7 years ago

http://blog.fefe.de/rss.xml?html

olsh commented 7 years ago

Can't reproduce this: https://www.screencast.com/t/aaEbfW5rLN

What OS are you using?

ThiefMaster commented 7 years ago

Win7. It seems to happen only with "Expand all feeds when popup is opened" enabled.

olsh commented 7 years ago

Found the problem. data-link is not set on links that are part of a feed's content. Running this in the console fixed it:

A similar code is executed on feed opening. https://github.com/olsh/Feedly-Notifier/blob/ff1d21e721e02f0cbaaddfed42bc72cb0c6f0b4f/src/scripts/popup.js#L129-L134

Probably there are errors in the console?

Somewhat unrelated, but I believe var isFeed = link.hasClass("title") && $("#feed").is(":visible"); might breakf if someone has class=title in their feed.

Thanks, yes, there can be a bug.

olsh commented 7 years ago

Win7. It seems to happen only with "Expand all feeds when popup is opened" enabled.

Got it, thanks.