Scroll if needed

[jasongrout]

Why do we implement our own scrollIfNeeded (https://github.com/phosphorjs/phosphor/blob/master/src/dom/query.ts#L89) instead of using the browser's scrollIntoView (https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView)?

Alternatively, I think the scrollIfNeeded algorithm could be improved by following (maybe with a threshold?) the spec for scrollIntoView: https://drafts.csswg.org/cssom-view/#element-scrolling-members

[sccolbert]

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.

[sccolbert]

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

[sccolbert]

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

[jasongrout]

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.

[sccolbert]

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

[jasongrout]

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.

[sccolbert]

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

[jasongrout]

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.

[sccolbert]

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.

[jasongrout]

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.

[sccolbert]

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

[jasongrout]

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...

[sccolbert]

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.

[jasongrout]

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.

[sccolbert]

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

[jasongrout]

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

[jasongrout]

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

:)

[jasongrout]

I'm happy to submit a PR, though.

[afshin]

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.

[jasongrout]

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?

[sccolbert]

I'm working on a PR.

[sccolbert]

cf #126

[afshin]

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

The native scrollIntoView might actually be sufficient.

[sccolbert]

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.

[afshin]

Oh right, good call, that was the catch.

[jasongrout]

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

[jasongrout]

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...

[afshin]

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

[sccolbert]

fixed