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

Timing issues causes popup to shrink #59

Closed tveimo closed 7 years ago

tveimo commented 8 years ago

When using "Open feeds in background tab", the popup opens, but immediately shrinks to a very small size, prob about 100x100 pixels. If I click the button several times, I am sometimes successfull in opening the popup to its proper size.

olsh commented 8 years ago

@tveimo thanks for the reporting.

Which browser are you using? Could you please check if it happens when all other extensions are disabled? Have you tried to reinstall the extension?

tveimo commented 8 years ago

I tried reinstalling, no change. Turning off the background tab setting fixes the problem (so far). Am running Chrome version 54.0.2840.71 (64-bit).

Haven't tried disabling all other extension yet. Am unsure what the background tab setting helps with? speed?

olsh commented 8 years ago

@tveimo it determines behavior when you click on an article link in the popup. When the option is enabled an article will be opened in a background tab and the popup will remain opened. When the option is disabled and you click on any article link, the article will be opened in an active (focused) tab and the popup will be closed.

Probably we need to rename the option. Do you have some suggestions?

tveimo commented 8 years ago

"Open links to feed items in background tab" maybe?

tveimo commented 8 years ago

Just tried restarting the browser with only the feedly extension enabled, and it has the same behaviour. Now I have also managed to get the issue also with the background tab option enabled, so I guess it's not related to that. I've put up a screenshot of what happens here; https://dl.dropboxusercontent.com/u/6299520/Screen%20Shot%202016-11-17%20at%209.42.39%20pm.png

olsh commented 8 years ago

Could you please right click on extension icon and hit "Inspect popup"? Are there any errors in "Console" tab?

tveimo commented 8 years ago

No errors that I can see, but am not sure if errors will show up there unless the console is already open? I'm seeing this problem much less often now after I starting fiddling with it though.

olsh commented 8 years ago

No errors that I can see, but am not sure if errors will show up there unless the console is already open?

If there are errors in popup scripts, you'll see them.

Could you also go to chrome://extensions/ , enable developer mode and press "background page" near the extension? It'll open the same console window but for background scripts.

tveimo commented 7 years ago

I haven't been able to find any more indications on what is causing this, but I've seen the same behaviour sometimes with the gmail notifier popup, so I'm starting to suspect this might be a chrome issue, possibly related to timing. I'm in Australia, so maybe I'm seeing higher latency to some of the services used by these plugins, so am curious if inserting some debug timeouts in parts of the code might help determine if it's actually caused by timing issue.

tveimo commented 7 years ago

It appears that the popup shrinks to the size it has while it is displaying the throbber. Would it be possible to test out just having the throbber shown in the center of the window at its normal size, without resize? If this is the cause, then maybe the resize happens too late.

tveimo commented 7 years ago

I tried reverting back to version 2.7.2, to avoid the async popup loading, and it seems to have fixed my problems.

olsh commented 7 years ago

@tveimo thanks for the investigation. I'll try to reproduce the problem.

dragonito commented 7 years ago

Got the same problem but only on MacOs. Next time iยดll try to analyse it. Perhaps there is a way to reproduce.

DavidBabel commented 7 years ago

same problem on chrome 55 macOS sierra, here is the aspect :

capture d ecran 2016-12-13 a 11 56 47

I think it's linked to the openning animation. It begin small, grows, then get back to original small size.

olsh commented 7 years ago

@dragonito @DavidBabel Thanks for the help.

@tveimo are you using mac too?

tveimo commented 7 years ago

Yes also on mac. I see a similar problem with the gmail notifier sometimes.

olsh commented 7 years ago

@dragonito I've noticed that you forked the repo. I've written "build" steps just in case. https://github.com/olsh/Feedly-Notifier/blob/master/README.md#build

Sadly, I don't have a mac right now, so, any help is appreciated. :)

DavidBabel commented 7 years ago

Can take a look. But i never tried work on browser extension, you will have to strongly review my code.

DavidBabel commented 7 years ago

What is supposed to be the "sandbox" command in the readme ?

olsh commented 7 years ago

@DavidBabel thanks. If sandbox is specified, the will be compiled a sandbox version of the extension which uses sandbox feedly site http://sandbox.feedly.com/i/welcome I usually use it for testing purposes.

DavidBabel commented 7 years ago

Oh ok, i get it, missing a grunt before "sandbox". Didn't get it was a build option, and not a command named "sandbox" which build it :p

olsh commented 7 years ago

Yeah, sorry, will fix it. Thanks.

DavidBabel commented 7 years ago

62 do the trick, but it's a workaround.

The exact scenario i noticed it the following : 1 - popup expand normaly 2 - just after expand : another event is received and redefine width to dummy values

The "2" does not append everytime, but often anyway. It's possible to click multiple times on the icon to get back a "correct" behaviour.

The "setTimeout" i added override the "2" behaviour just after it appends. It's not clean, but i did not find where the problem is created.

tveimo commented 7 years ago

The dummy values you mention, aren't they the size of the window shown when the throbber is shown, before the window is populated with feed entries?

DavidBabel commented 7 years ago

The size is this one : https://github.com/olsh/Feedly-Notifier/issues/59#issuecomment-266708934 . It's close to.

But sometime it does like that on my tests :

capture d ecran 2016-12-13 a 16 53 30

Since it seems to only append on MacOs, it can be a chrome problem. I let @olsh judges if this workaround is good enouth.

olsh commented 7 years ago

@DavidBabel I did small refactoring of your PR. Could you please test if everything works as expected?

DavidBabel commented 7 years ago

The issue come back sorry ๐Ÿ˜ญ

olsh commented 7 years ago

Reverted it back, please do the final check and I push it to store. Thanks.

DavidBabel commented 7 years ago

I tested it this two ways :

Both rarelly occur the bug again. Consider to increase the 50ms timeout to 300ms. Works for me.

I expect a strange other behavious i did not notice before. After the grows of the popup smoothly, it instant change from "width" to "expandedWidth" size.

olsh commented 7 years ago

@DavidBabel I've pushed another commit, could you please check again? I tried another approach to address the issue.

DavidBabel commented 7 years ago

Hey, @olsh , the issue came back. Sorry again

capture d ecran 2016-12-14 a 12 05 37

The deploy effect i described here https://github.com/olsh/Feedly-Notifier/issues/59#issuecomment-266800491 actually disapear

olsh commented 7 years ago

@DavidBabel Thanks, so, we need to set it to 50ms or 300ms?

DavidBabel commented 7 years ago

To exclude bug when spam click the button and the behaviour when cpu charge is high, it's better. But still a workaround.

Sorry i can't do better, i imagine how frustrating it could be to not be able to test by yourself

olsh commented 7 years ago

@DavidBabel Sorry, I did a mistake in my commit, could you please test again? :) If it falls again, I'll revert to your commit and increase the interval for mac platform.

Sorry i can't do better, i imagine how frustrating it could be to not be able to test by yourself

No, everything is OK, without you I wouldn't solve the issue at all. Thank you for your help! ๐Ÿ‘

DavidBabel commented 7 years ago

Thanks ^^.

On this new version, it works well. The only behaviour which still trigger the problem is a "rapid 3 clicks" on the icon. But i have to admit, we don't do that on current use. The high CPU test does not trigger the problem anymore.

I think you got a very acceptable solution now, since it worked as expected in a very large number of case during common use. Well done.

olsh commented 7 years ago

@tveimo @dragonito I've just published the fixed version(2.12.1) to the store it'll available soon. Could you please test it and confirm that everything works?

Kilhog commented 7 years ago

Hi, The bug is still here for me in 2.12.1 on Chrome 54 MacOs Sierra. ๐Ÿ˜‰ https://youtu.be/5qCZP72Niq4

DavidBabel commented 7 years ago

same bug for me with 2.12.1. this is very strange

tveimo commented 7 years ago

Would it be possible to rerelease the version with async popup loading disabled, until this issue is resolved?

olsh commented 7 years ago

I've disabled async loading for Mac users in 2.12.1. And it didn't help. https://github.com/olsh/Feedly-Notifier/blob/master/src/scripts/popup.js#L187

DavidBabel commented 7 years ago

Hey ! My patch was ugly but actually corrected the problem. I admit that's a workaround, but a workaround which correct the issue.

olsh commented 7 years ago

Yes, your fix works, but I don't like the idea with big timeout (300ms), in this way the popup has a lag on windows (and probably on linux too). I could push a version with 50ms, but you said that the bug happens sometimes. So, we need to find a 100% fix without side effects.

@DavidBabel could you please check that this branch is executed on Mac? https://github.com/olsh/Feedly-Notifier/blob/master/src/scripts/popup.js#L188

@DavidBabel could you please check, if disabling async renedering(remove all executeAsync calls and call all functions directly) resolves this issue completely?

Guys, sorry, if I'd have a Mac, I would fix the issue more quickly.

DavidBabel commented 7 years ago

Yeah don't worry, we understand fully, it's already a great job to consider your issues. I will make your tests tomorrow and will keep you in touch for the results.

DavidBabel commented 7 years ago

Actually the timeout is human perceptible only when bigger than 250ms. But anyway i understand your concern. But in our case, it's not a big deal, and i will tell you why.

The point is :

As my patch was a bit ugly in the code, the behaviour was perfectly nice for the user experience.

Hope i explain it clear enough, my english is tired at friday 5pm.

I do the tests tomorrow and keep you informed.

DavidBabel commented 7 years ago

did not find the time yet. soon :)

tveimo commented 7 years ago

Since this seems to happen to the gmail notifier as well, it looks like its a chrome bug; https://bugs.chromium.org/p/chromium/issues/detail?id=307912

Workaround: "There is clearly a race condition since with timeout 100 this is happening consistently, with timeout 110 only sometimes while with timeout 150 or 50 never."

olsh commented 7 years ago

@tveimo thanks for the info! ๐Ÿ‘ Well, we need to add these timeouts (only for mac). But someone should test it.

tveimo commented 7 years ago

How do I run the extension with real data from the feedly server? I've generated a developer id / clientSecret but I am unable to actually get any feed entries shown in the popup, I only get the login button.

olsh commented 7 years ago

You can't, you should use this server with the test credentials. https://sandbox.feedly.com/

tveimo commented 7 years ago

By editing feedly.api.js? I tried running grunt with clientId=sandbox, the clientSecret I generated, but when running the extension, it seems to be able to login, then tries to load http://localhost/?code=Ayka.... If I manually edit the url to the sandbox one, or just feedly.com, I can get into feedly, but I cannot get any feed entries in the popup. It still shows the login button.