Scroll if needed about phosphor HOT 29 CLOSED

Our implementation is closer in behavior to this:
https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoViewIfNeeded

Which is not widely supported by browsers.

from phosphor.

If you can get scrollIntoView to behave like our scrollIfNeeded across browsers, I'm happy to ditch our version.

from phosphor.

and scrollIfNeeded does take a threshold option, so I'm not sure I follow you on that part.

from phosphor.

If you want, we can implement the algorithm with a threshold, if it's needed. I only see one place where it is used, and that doesn't use the threshold.

from phosphor.

I still don't follow. The third argument to scrollIfNeeded is an optional threshold. What are you asking for here?

from phosphor.

Our implementation is closer in behavior to this:
https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoViewIfNeeded

Ah, then our implementation is buggy, in that it doesn't take into account the element width.

from phosphor.

Bug is a strong word :) It's working as designed, but is only designed to scroll vertically: https://github.com/phosphorjs/phosphor/blob/master/src/dom/query.ts#L92-L95

from phosphor.

I still don't follow. The third argument to scrollIfNeeded is an optional threshold. What are you asking for here?

I'm saying we could implement the algorithm in scrollIntoView, but with a threshold. Separately, I'm questioning the need for a threshold parameter, since it doesn't seem to be used, at least in this repo.

from phosphor.

There's lots of functionality in this repo which is not used by the repo itself :) It was easy enough to add a threshold, which I knew would be a common request, so there it is.

from phosphor.

Bug is a strong word :) It's working as designed, but is only designed to scroll vertically: https://github.com/phosphorjs/phosphor/blob/master/src/dom/query.ts#L92-L95

Even vertically, it has problems and is scrolling unnecessarily. For example, see jupyterlab/jupyterlab#478.

from phosphor.

PEBKAC - you're calling scrollIfNeeded on the wrong node.

from phosphor.

It was easy enough to add a threshold, which I knew would be a command request, so there it is.

Easy enough, I know. I don't have your foresight that it will be requested, though, and saw it as a premature customization. Hence my questioning it...

from phosphor.

You are asking it to scroll the large container node into view, instead of the element with focus. Call scrollIfNeeded on the input node, and I'll bet you'll see better behavior.

from phosphor.

The point is that if you are asking a node that is larger than the viewport to scroll into view, and it's already in view, scrollIfNeeded, as implemented, will unnecessarily scroll so the bottom is aligned with the bottom.

Edit: that is, if the top is in view, but the bottom is below the viewport.

from phosphor.

Yeah, I realized that as soon as I typed my response.

from phosphor.

The algorithm for scrollIntoView correctly handles this case where the element is larger than the viewport.

from phosphor.

Yeah, I realized that as soon as I typed my response.

:)

from phosphor.

I'm happy to submit a PR, though.

from phosphor.

n.b., Here's a bit of background history.

scrollIfNeeded is used in a few spots in JupyterLab (and thus implemented multiple times). We added it into phosphor so we wouldn't have to keep repeating ourselves when we switch over to mono-repo. The threshold argument doesn't exist in the JupyterLab versions, but since we were making it available to all future phosphor users, we decided it would be good to add that argument even though it's not being used.

For example, if you consider the notebook/widget version:

https://github.com/jupyter/jupyterlab/blob/master/src/notebook/notebook/widget.ts#L998-L1006

... you'll see that threshold was hard-coded. We decided to make that an argument instead in the phosphor version.

from phosphor.

you'll see that threshold was hard-coded

Dummy me wasn't seeing it in the code I was originally looking at - thanks for pointing out at least one place it was being used. I'm still not sure it's a good idea to have the threshold. What was the problem without the threshold, and is it fixed if we implement a proper scrollIfNeeded?

from phosphor.

I'm working on a PR.

from phosphor.

cf #126

from phosphor.

I don't remember what the original reason for the threshold was, I'm afraid.

The native scrollIntoView might actually be sufficient.

from phosphor.

The problem with the native scrollIntoView is that several browsers don't support the options argument, which is what we need to get the "if needed" part of the behavior.

from phosphor.

Oh right, good call, that was the catch.

from phosphor.

Yeah, agreed that if we need the IfNeeded part, we'll have to implement our own.

from phosphor.

Perhaps the threshold accounted for padding/border in the elements? In other words, if my element is padded by 10 pixels, then I'm okay with it being actually 10 pixels off screen...

from phosphor.

I assume that's exactly the sort of reasoning that went into it, but I can't say so authoritatively.

from phosphor.

fixed

from phosphor.