search

atlas-engineer / nyxt

Nyxt - the hacker's browser.
https://nyxt-browser.com/
9.64k stars 404 forks source link

Restore Scroll Position on Reload #3324

Closed jmercouris closed 4 months ago

jmercouris commented 5 months ago

Description

Fixes #3325

Checklist:

jmercouris commented 5 months ago

I'm afraid that there are some misunderstandings with regards to the report from #1263. It is both a non reproducible bug report (wrt local files not honoring URIs with fragments) and a feature request. This PR addressed the latter, which is superseded by #3325.

I don't believe it has anything to do with URI fragments. Here is the use case I think it is:

I am writing a LaTeX file. I compile it to HTML. I want to update the HTML view in Nyxt that is rendering my LaTeX file. For this use case, it works perfectly. It is not a function of the renderer. The render can't actually generally solve this problem. We are just providing a naieve solution that works for 99% of cases.

Concretely, this solution seems to suffer from a fundamental issue. Navigate to https://en.wikipedia.org/wiki/Tomato#Nutrition, issue reload-current-buffer and the conclusion follows.

Yes, that is a problem. To solve this we need to compare the scroll position that we are upon reload rather than blindly moving by some offset. In other words, scrolling to an absolute position would solve this problem.

In conclusion, I'd suggest closing this PR and think about a correct implementation in the future.

I've outlined a straightforward path to make this pull request correct for 100% of cases! Therefore, I disagree.

aadcg commented 5 months ago

I've outlined a straightforward path to make this pull request correct for 100% of cases! Therefore, I disagree.

Given that the first iteration of this PR missed a very important point, what makes us think that we won't fall prey of the same overconfidence? Is this feature so important for us to risk providing a flawed behavior for such a central usability issue (reloading a buffer)?

More importantly, I believe we should decrease the surface area to make mistakes. We're not fixing anything here, but rather improvising a solution over a non-central aspect in my humble opinion.

Regardless, proceed as your judgement best advises you.

jmercouris commented 5 months ago

This is not a trivial thing. In my opinion it is a fix. The fact that on reload things scroll willy-nilly is infuriating. This fixes a LONG standing issue I've had.

jmercouris commented 5 months ago

Thank you for finding the first bug, please give it another try, I don't think there remain any problems.

aadcg commented 5 months ago

I still think that this is the renderer's responsibility. Test it via (webkit:webkit-web-view-reload (nyxt/renderer/gtk:gtk-object (current-buffer))). It just works, and we don't need to think about what might go wrong with this ad-hoc JS implementation. Besides webkit:webkit-web-view-reload, there's also webkit:webkit-web-view-reload-bypass-cache.

See on-signal-decide-policy from gtk.lisp. There seems to be a :reload navigation-type.

aadcg commented 5 months ago

Also, I've tested the new iteration of your implementation and it still suffers from the issue I've reported.

Navigate to https://en.wikipedia.org/wiki/Tomato#Nutrition, issue reload-current-buffer and the conclusion follows.

jmercouris commented 5 months ago

I'm very surprised, because the example you've posted works perfectly on my machine...

jmercouris commented 5 months ago

https://github.com/atlas-engineer/nyxt/assets/1691662/e04b6556-7da1-43f1-b99c-e457567b5884

Are you sure that you are loading the branch correctly?

jmercouris commented 5 months ago

With regards to the information about WebKit supporting a buffer reload function, that will require changing our FFI. Which is of course possible, but now we will have buffer-load AND buffer-reload.

aadcg commented 5 months ago

With regards to the information about WebKit supporting a buffer reload function, that will require changing our FFI. Which is of course possible, but now we will have buffer-load AND buffer-reload.

Correct. I find that to be a much superior solution - less surface area for mistakes and right assignment of responsibilities.

jmercouris commented 5 months ago

OK, I will implement it that way then. I know this is indeed not an important task... but I am already this deep. Plus, I find it interesting.

aadcg commented 4 months ago

@jmercouris please don't forget to delete the remote branch when closing a PR.

jmercouris commented 4 months ago

Ah yes!