Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[General] Webview improvements and fixes #836

Merged
merged 27 commits into from
Jan 11, 2022
Merged

[General] Webview improvements and fixes #836

merged 27 commits into from
Jan 11, 2022

Conversation

philipwilk
Copy link
Collaborator

@philipwilk philipwilk commented Dec 29, 2021

Listen for dom-ready instead of did-stop-loading for UpdateComponent

This significantly decreases the time taken for users to see the webpage as UpdateComponent no longer waits for the DOM to entirely load, and UpdateComponent fully blocks the webview until it begins rendering. #829

This does not entirely fix the issue, as there is still a short(er) period of time when the webview has begun to load but UpdateComponent is still visible. This is either because of

  • the eventlisteners taking time to fire
  • react taking time to change the refresh state
  • react taking time for UpdateComponent to be derendered/removed.
Now i feel really dumb lol.


Checklist if you have changed something on the Backend or Frontend:

- [ Y ] Tested the feature and it's working on a current and clean install.

Better responsiveness; lets webview and the webpages use their rendering optimisations.

- [ X ] Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)

Latest prefix creation system is utterly broken. Otherwise, no problems.

- [ N/A ] Created / Updated Tests (If necessary)

Not necessary.

- [ X ] Created / Updated documentation (If necessary)

Not necessary.

@philipwilk philipwilk marked this pull request as ready for review December 29, 2021 23:29
@flavioislima
Copy link
Member

The only difference I saw when testing here is that the loading spinner stops earlier and the website is blank then it loads. I don't think this is an improvement over the current solution.

Tbh I think there is not much to do related with that since webview is quite limited in terms of events. This approach you are using was how I did the first time, then I changed it and I think its better like this, listening to the stop loading. Because after the dom is ready, still the elements are not completely painted, so that's why the blank.

using the loading event, Heroic will wait for the content to be painted.
It's not the perfect solution but I think works fine for the users to access the store and get some free games :P

@philipwilk
Copy link
Collaborator Author

The only difference I saw when testing here is that the loading spinner stops earlier and the website is blank then it loads. I don't think this is an improvement over the current solution.

Tbh I think there is not much to do related with that since webview is quite limited in terms of events. This approach you are using was how I did the first time, then I changed it and I think its better like this, listening to the stop loading. Because after the dom is ready, still the elements are not completely painted, so that's why the blank.

using the loading event, Heroic will wait for the content to be painted. It's not the perfect solution but I think works fine for the users to access the store and get some free games :P

Using dom-ready, it loads exactly the same as it does in a normal browser, with did-stop-loading it waits for ages before showing anything at all.

If you visit the epic store using any browser, it will paint the main structural elements before most of the images.
I'm not sure how long of a difference it may make for you, but letting it be rendered normally shows me stuff after 2 seconds, while the way it is done in heroic right now takes 6 whole seconds to show anything at all.

If you really dislike how the website renders normally and would prefer it to fully paint before being rendered, adding the changes made to webview in src/screens/WebView/index.css prevents the webview from being visible when the refresh state is true, stopping the visible jump upwards when refresh becomes false because it's rendered in the same location as the UpdateComponent.

@flavioislima
Copy link
Member

I'll make more tests when I have time but for me both shows up in less than 3 seconds but your solution dismiss the loading faster, showing a empty website. While the current one dismiss the loader then the website shows completely.
Let's keep on hold or ask others opinions as well like @arielj @Nocccer and @dragonDScript

@Nocccer
Copy link
Collaborator

Nocccer commented Dec 30, 2021

If i open epix store in browser it takes always a while before it shows the free games, but the page is already there. If this is about to show the strucuture earlier and don't wait till the full content is loaded, i would prefer the way where we load earlier. Like epic store is shown in the browser.

@ghost
Copy link

ghost commented Dec 30, 2021

As per electron docs:
Event: 'did-finish-load'​

Emitted when the navigation is done, i.e. the spinner of the tab has stopped spinning, and the onload event was dispatched.

Event: 'did-stop-loading'​

Corresponds to the points in time when the spinner of the tab stopped spinning.

Event: 'dom-ready'​

Emitted when the document in the top-level frame is loaded.

I think It would be better to use did-finish-loading event, to make sure it's completely loaded.

@philipwilk
Copy link
Collaborator Author

If i open epix store in browser it takes always a while before it shows the free games, but the page is already there. If this is about to show the strucuture earlier and don't wait till the full content is loaded, i would prefer the way where we load earlier. Like epic store is shown in the browser.

In browser is dom-ready, as it loads right now is did-stop-loading.

@philipwilk
Copy link
Collaborator Author

As per electron docs: Event: 'did-finish-load'​

Emitted when the navigation is done, i.e. the spinner of the tab has stopped spinning, and the onload event was dispatched.

Event: 'did-stop-loading'​

Corresponds to the points in time when the spinner of the tab stopped spinning.

Event: 'dom-ready'​

Emitted when the document in the top-level frame is loaded.

I think It would be better to use did-finish-loading event, to make sure it's completely loaded.

I think this would load for every single js script to finish in the page, including trackers etc, while did-stop-loading

@philipwilk philipwilk closed this Dec 30, 2021
@philipwilk
Copy link
Collaborator Author

nice that the close button is next to the comment button

@philipwilk philipwilk reopened this Dec 30, 2021
@ghost
Copy link

ghost commented Dec 31, 2021

As per electron docs: Event: 'did-finish-load'​
Emitted when the navigation is done, i.e. the spinner of the tab has stopped spinning, and the onload event was dispatched.
Event: 'did-stop-loading'​
Corresponds to the points in time when the spinner of the tab stopped spinning.
Event: 'dom-ready'​
Emitted when the document in the top-level frame is loaded.
I think It would be better to use did-finish-loading event, to make sure it's completely loaded.

I think this would load for every single js script to finish in the page, including trackers etc, while did-stop-loading

You can do a .once()

@philipwilk
Copy link
Collaborator Author

As per electron docs: Event: 'did-finish-load'​
Emitted when the navigation is done, i.e. the spinner of the tab has stopped spinning, and the onload event was dispatched.
Event: 'did-stop-loading'​
Corresponds to the points in time when the spinner of the tab stopped spinning.
Event: 'dom-ready'​
Emitted when the document in the top-level frame is loaded.
I think It would be better to use did-finish-loading event, to make sure it's completely loaded.

I think this would load for every single js script to finish in the page, including trackers etc, while did-stop-loading

You can do a .once()

What's the once for?

@ghost
Copy link

ghost commented Dec 31, 2021

As per electron docs: Event: 'did-finish-load'​
Emitted when the navigation is done, i.e. the spinner of the tab has stopped spinning, and the onload event was dispatched.
Event: 'did-stop-loading'​
Corresponds to the points in time when the spinner of the tab stopped spinning.
Event: 'dom-ready'​
Emitted when the document in the top-level frame is loaded.
I think It would be better to use did-finish-loading event, to make sure it's completely loaded.

I think this would load for every single js script to finish in the page, including trackers etc, while did-stop-loading

You can do a .once()

What's the once for?

To listen to the first call of the event only

@philipwilk
Copy link
Collaborator Author

As per electron docs: Event: 'did-finish-load'​
Emitted when the navigation is done, i.e. the spinner of the tab has stopped spinning, and the onload event was dispatched.
Event: 'did-stop-loading'​
Corresponds to the points in time when the spinner of the tab stopped spinning.
Event: 'dom-ready'​
Emitted when the document in the top-level frame is loaded.
I think It would be better to use did-finish-loading event, to make sure it's completely loaded.

I think this would load for every single js script to finish in the page, including trackers etc, while did-stop-loading

You can do a .once()

What's the once for?

To listen to the first call of the event only

I get that, but wouldn't that lead to undefined behaviour if you switched off the webview and then went back onto it?

@flavioislima
Copy link
Member

@wiryfuture to test in prod, on v2.0.1 I changed the event listener to use dom-ready and yes, with the packaged App I can feel a bigger different and the loading looks better. So fix the conflicts of this one and we will test if the Login is working fine, we can use dom-ready for all URLs IMO. And if everything is fine we can merge this :)

@philipwilk
Copy link
Collaborator Author

The feature is in with commit 3113610, b0624ae is a merge with main commits that i can't see for some reason.
I see this in the log when i sign in with Epic Games and see the SID page in heroic, I think it's meant to redirect after you sign in but i am unsure what the intended functionality is so I cannot comment.
Screenshot_20220106_165345

@philipwilk
Copy link
Collaborator Author

Seem to have resolved previous issues, appears fully functional, but idk about all edge cases,

weblate and others added 5 commits January 7, 2022 00:02
Updated by "Squash Git commits" hook in Weblate.

Update translation files

Updated by "Squash Git commits" hook in Weblate.

Co-authored-by: Hosted Weblate <hosted@weblate.org>
Translate-URL: https://hosted.weblate.org/projects/heroic-games-launcher/gamepage/
Translate-URL: https://hosted.weblate.org/projects/heroic-games-launcher/globals/
Translation: Heroic Games Launcher/GamePage
Translation: Heroic Games Launcher/Globals
* chore: wait before getting the sid

* feat: add Login with sid as second option

* 1i8n: updated keys
@philipwilk
Copy link
Collaborator Author

weblate pls dont mess with my repo it confuses vscode

flavioislima
flavioislima previously approved these changes Jan 9, 2022
Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍🏽

@flavioislima
Copy link
Member

@wiryfuture
Before merging, can you change the USER_AGENT to be:
Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.137 Safari/537.36

And Also on the component:
src/components/UI/WebviewControls/index.tsx
change the line 19 to use did-stop-loading since I noticed that it fixes the bug where the webview URL is not being updated. It takes a few seconds but it works, the current implementation doesn't do that when changing from Wiki to Store for instance.

@philipwilk
Copy link
Collaborator Author

@wiryfuture Before merging, can you change the USER_AGENT to be: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.137 Safari/537.36

And Also on the component: src/components/UI/WebviewControls/index.tsx change the line 19 to use did-stop-loading since I noticed that it fixes the bug where the webview URL is not being updated. It takes a few seconds but it works, the current implementation doesn't do that when changing from Wiki to Store for instance.

Just (out of interest) tried usng dom-ready for that as well and it is also faster, switching to the url as soon as the page starts loading rather than after it finishes loading, like a traditional browser. 😅

@flavioislima
Copy link
Member

@wiryfuture Before merging, can you change the USER_AGENT to be: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.137 Safari/537.36
And Also on the component: src/components/UI/WebviewControls/index.tsx change the line 19 to use did-stop-loading since I noticed that it fixes the bug where the webview URL is not being updated. It takes a few seconds but it works, the current implementation doesn't do that when changing from Wiki to Store for instance.

Just (out of interest) tried usng dom-ready for that as well and it is also faster, switching to the url as soon as the page starts loading rather than after it finishes loading, like a traditional browser. sweat_smile

Actually, I think its not working when navigating inside the page. Because the dom won't change in this case, but the page will reload. so dom-ready wont work on that.

@philipwilk
Copy link
Collaborator Author

@wiryfuture Before merging, can you change the USER_AGENT to be: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.137 Safari/537.36
And Also on the component: src/components/UI/WebviewControls/index.tsx change the line 19 to use did-stop-loading since I noticed that it fixes the bug where the webview URL is not being updated. It takes a few seconds but it works, the current implementation doesn't do that when changing from Wiki to Store for instance.

Just (out of interest) tried usng dom-ready for that as well and it is also faster, switching to the url as soon as the page starts loading rather than after it finishes loading, like a traditional browser. sweat_smile

Actually, I think its not working when navigating inside the page. Because the dom won't change in this case, but the page will reload. so dom-ready wont work on that.

Seemed to work fine when i tested it just now, navigating across the epic website. Are you talking about ids sections on the page or something?

@flavioislima
Copy link
Member

@wiryfuture go to the store, click on any game, navigate through the store. The url should change to the game url or any other. Right now it keeps as the home page. Never changes.
For the change from wiki to store is OK, but should change depending on the store url as well.
I haven't tested but check on the wiki as well. Should have the same issue.

@philipwilk
Copy link
Collaborator Author

@wiryfuture go to the store, click on any game, navigate through the store. The url should change to the game url or any other. Right now it keeps as the home page. Never changes. For the change from wiki to store is OK, but should change depending on the store url as well. I haven't tested but check on the wiki as well. Should have the same issue.

My brain just stopped responding.. there's literally a did-navigate event (aka, url change)

@flavioislima
Copy link
Member

Have you checked if it's working on all situations because I guess I tried did-navigate before and had the same issue. Only one that worked was the one I said above.

@philipwilk
Copy link
Collaborator Author

Have you checked if it's working on all situations because I guess I tried did-navigate before and had the same issue. Only one that worked was the one I said above.

I can see it working when i look at different games or the epic FAQ or unreal website or different location on github. Can't really think of anything else that I should be checking with that?

Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good! Thanks 👍🏽

@flavioislima flavioislima changed the title Improve webview responsiveness [General] Webview improvements and fixes Jan 11, 2022
@flavioislima flavioislima merged commit b67668e into Heroic-Games-Launcher:main Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants