Remember scroll distance of every directory, solves #18805

[pellaeon]

• scroll distance $('#app-content').scrollTop() will be recorded whenever
  directory is changed
• will be restored after _setDirectory(), and load enough file rows if
  scroll distance is not enough
• recording using sessionStorage, the storage is per-window-session
  ie. every window has its own, will survive reload and restore

[pellaeon]

Solves #18805 , should support IE8 (IE8 supports sessionStorage)

[PVince81]

Thanks @pellaeon

I think it would be better to store the last clicked file name instead of the scroll distance. Imagine if when returning to the previous folder, the contents of the folder has changed ? In this case the scroll distance will not necessarily point to the last file.

There is already code that can be called to make it autoscroll to a specific file: fileList.scrollTo(fileName);. However I think the highlight effect should be disabled, maybe add an additional option to that method to disable the highlight effect.

[PVince81]

Ok, seems @jancborchardt thought that keeping the highlight is a good idea, see #18805 (comment)

So simply calling fileList.scrollTo(fileName) should be enough.

[jancborchardt]

@PVince81 yeah, seems good to just take the last clicked folder. This could lead to some problems when there is no such thing as a last clicked folder but that’s something we could solve step by step, right?

@pellaeon thanks for taking this on! :)

[pellaeon]

Hmm, but I think returning to the exact same position would be more intuitive, if the file is not there, relevant position of other files would still be the same, and the user can easily see that the original file is gone.
If user enters a folder and quickly goes back, he/she will see the position of the upper level folder slightly changed, for me it feels like something is out of place.
And also in OSX's Finder, clicking the previous button will bring user to the exact same position where he/she just left.

[PVince81]

also to note: the scroll position can also change if the window's size changed.

@pellaeon I understand your point, basically hoping to show the exact same view as before.

One idea would be to take note of the distance between the file name and the top of the window, but that's the same as just using scroll top without the file name.

Not sure now...

[jancborchardt]

Both approaches sound valid. I’d say go for the one which seems to make most sense to both of you and then improve upon that, since both seem to have some potential edge-cases.

[pellaeon]

How about merging this first, later I can propose a PR that record the original file list (before leaving), and compare it with the newly obtained file list when re-visiting. If the list has changed, we fallback to fileList.scrollTo(fileName), if not, then use my loadAndScrollTo(distance).

This should completely solves the problem, though with a lot of overhead.

Do we really need to handle this edge case?

If both approaches are good enough, I would prefer mine, for it is already implemented ;-)

[PVince81]

Regardless whether we store the scroll position or the last selected folder, I think this approach will not work as expected. Here in this PR the scroll value is permanently stored in the session storage, which means that every time you open that folder (from browser back button, clicking the breadcrumb or opening its URL on a separate tab), it will scroll you down.

I think what we want is only scroll back to the last folder when you click the "Back" button in the browser. In this case, the scroll value (or last folder name) must be stored in the history stack here https://github.com/owncloud/core/blob/v8.2.0/apps/files/js/app.js#L219.

Currently the History API wrapper in OC only supports setting URL parameters, it would need to be extended to also support a second argument to store "hidden" session parameters. The "state" object needs to be changed to be different from the URL params: https://github.com/owncloud/core/blob/v8.2.0/core/js/js.js#L1805. Once this is done, the popstate event (browser back button) will deliver the stored session state back here https://github.com/owncloud/core/blob/v8.2.0/core/js/js.js#L1873 (e.state instead of parsing the URL). Note that this doesn't not work in IE8, but it doesn't matter much as long as it doesn't break in that version.

[pellaeon]

I thought about that actually, I only implemented it this way because it's easier, I tried to do it with OC.Util.History but had trouble getting it to work. Now after your explanation I think misunderstood how popstate and state object work.

Reasons for sessionStorage implementation:

1. It supports IE8
2. OSX's Finder also does this, it restores scroll position of every folder no matter you press "Back" or the folder in sidebar to open it. At least users would not be unexpected of the behavior.
3. Using history API would be more complex (now it's only setItem() and getItem())

Given the reasons above, what do you think?

[PVince81]

I still think we should use the history API. It doesn't matter if it doesn't work in IE8 as this is already a "luxury" feature, IE users don't need luxury 😉

Let me know if you need help for adjusting the OC.History stuff.

[pellaeon]

OK I'll re-implement this using history API

2015-10-30 17:22 GMT+08:00 Vincent Petry notifications@github.com:

I still think we should use the history API. It doesn't matter if it
doesn't work in IE8 as this is already a "luxury" feature, IE users don't
need luxury [image: 😉]

Let me know if you need help for adjusting the OC.History stuff.

—
Reply to this email directly or view it on GitHub
#20132 (comment).

[pellaeon]

After experimenting with history API, I found that the way we understand pushState() seems to be wrong.

Consider the case where I go from /foo to /foo/bar by clicking bar, what pushState() is pushing onto the history stack (its params) is associated with the new directory /foo/bar, not the one you're coming from, which is /foo. You can look at this by looking at pushState()'s parameters, params.dir is always /foo/bar.

When onPopState event is fired, the provided state object is the state object of /foo.

That is, we save scroll distance in /foo/bar but read it from /foo.

Scroll distance should be saved in the old history entry /foo, not the new one /foo/bar, so we should use history.replaceState() instead of pushState().

[pellaeon]

@PVince81 I would still prefer sessionStorage though, for the sake of future maintainers, I believe the history API implementation is overly complex (it has to be), and sessionStorage implementation's behavior is not so uncommon, sometimes even preferrable.

Now we have 2 implementations, maybe we can have someone else's opinions on their behaviors.

1. sessionStorage implementation: will restore scroll position no matter how you get into that folder. (breadcrumb and browser back button)
2. history API implementation: will only restore scroll position if you use browser back button

@jancborchardt what do you think?

[PVince81]

I think it would be fine to use replaceState. The OC.History class can be extended for this.

My worry with the other approach is unpredictable behavior like when the user opens the same folder in a separate tab, it will scroll down to some position and the user might wonder why, as this position has nothing to do with the current tab.
Unless you find a way to store in sessionStorage the window's id so that it only happens on that specific window and not another one, but I believe that there is no such thing as unique window id. Also we need a way to clear the sessionStorage at some point.

[pellaeon]

Every page (tab) has its own sessionStorage, will not interfere with each other and will last until page close. You might have misunderstood what I meant by "window" in the commit log, where I meant the window object.

Please take a look at https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage

[PVince81]

Ah! Nice one. I did confuse it with "localStorage".

Forget about the history API then.

Can you clear the sessionStorage's value after reading it ?

[pellaeon]

Why clear it ? When you go to a new directory, the current scroll distance will overwrite the old one, ie. whenever you change directory, the old directory's scroll distance will be remembered.

[PVince81]

Can you use this.$container instead of the global selector ?

[PVince81]

Right, I was still thinking about cross-window cases, which is wrong. Not clearing it is fine.

• TEST: regular files app
• TEST: public link share
• TEST: browser forward/back buttons
• TEST: clicking breadcrumb (switching multiple times)

[pellaeon]

I don't know how to write tests for these, could you point me to some guides? or can you write the tests?

[PVince81]

This is just the manual test plan for later when I get to testing it. 😄

Regarding unit tests, it should be possible to stub the sessionStorage.
You can see some example tests here: https://github.com/owncloud/core/blob/master/apps/files/tests/js/filelistSpec.js#L55 and https://github.com/owncloud/core/blob/master/apps/files/tests/js/filelistSpec.js#L1306
For example you could write a test that calls changeDirectory and checks whether the scroll value was passed to the stubbed session storage.

If that's too complicated for now I can take care of the tests later.

[pellaeon]

I'll leave the tests for you ;-)

[fatiheke]

this is work. but gallery app "thumbnails folder" not work.
Please fix this problem.

--- update ---
my solution:

Problem: gallery thumbnails mode folder click and back browser button not Remember scroll position (Remember scroll distance of every directory - #20132)

my solution;

/* feke /
$(document).ready(function() {
$('.album').click(function() {
sessionStorage.scrollCur = $('#content-wrapper').scrollTop();
sessionStorage.M="0";
});
$(window).bind('mousewheel DOMMouseScroll', function(event){
if (event.originalEvent.wheelDelta > 0 || event.originalEvent.detail < 0) {
// scroll up
sessionStorage.M="1";
}
else {
sessionStorage.M="1";
}
});
if (sessionStorage.M == "0") {
$('#content-wrapper').scrollTop(sessionStorage.scrollCur)
};
});
/ feke */

sample problem page;
http://web-inter.net/index.php/apps/gallery/s/NPbT4S1hwu0XoYZ#

I want to develop this code
I'm not very good at programming
Thank you

[pellaeon]

@PVince81 I added the tests, are there anything more to test?

And regarding the case for public link shares, I also add code to prefix sessionStorage key with sharing token. How should I test this? public link share tests seems to be located at apps/files_sharing/tests/js/publicAppSpec.js, should I add my fileList tests there?

[PVince81]

Hmm, the main file list should not know anything about sharing token. We need to find a way to put that code in the public file list overrided functions instead.

[pellaeon]

Isn't public view share the same fileList class as private? apps/files_sharing/js/public.js#65

[PVince81]

Not exactly, the public.js overrides some method to add public-page specific code, like token handling

[PVince81]

(ideally in the future that class needs to be extracted as PublicFileList that extends FileList, something for another time)

[pellaeon]

@PVince81 how about this? I re-implemented changeDirectory in public fileList, js tests all pass.

[PVince81]

Instead of copying the whole method, you can call the parent method using OCA.Files.FileList.prototype.changeDirectory.apply(this, arguments).

So in that piece you can store the value.

Then, override reloadCallback to do the second part.

Do you think that would fit the workflow you intended ?

[pellaeon]

I don't quite understand you. If I call OCA.Files.FileList.prototype.changeDirectory.apply(this, arguments) then the sessionStorage key used would be /path, not <sharingToken>:/path, it would still mix up with the private fileList keys.

And even if I override reloadCallback , the callback after that, this.reload().then(function(success){ would still be called, in there loadAndScrollTo is called again, overriding the position I set in reloadCallback.

Or we can also choose not to handle this edge case, because there are only 2 ways mixed-up keys would cause unintended behavior:

1. in the public page, user copy-pastes a private fileList URL, like /index.php/apps/files/?dir=%2Ftest into the same tab
2. in the public page, user clicks the top-left ownCloud logo and logs in
   (And the opposite way around in the private page, of course)

[PVince81]

@pmaier1 @felixheidecke thoughts from the UX perspective ?

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.