Make menubar float work inside a scollable parent (not just window)

[cjbest]

Say you put a prosemirror editor with a menubar inside of a scrollable div, and you still want that nice float + sticky behaviour.

Before, we wouldn't even catch the scroll events (they don't get fired on window.) With this change, we look for cases where we are in a scrollable element, catch those events too, and detect + float to the appropriate top location.

This is my first PR here, so I apologize in advance if I've made any faux pas.

[marijnh]

Thanks for looking into this, and welcome.

I can see a potential problem with an overflow: auto parent—if the document is small when the editor is initialized, the parent may not be recognized as scrollable yet. Then, as the user types content, it becomes scrollable, but the menubar doesn't notice because it only checks on startup.

We could use getComputedStyle and check overflow (and overflow-y I guess) instead. That still doesn't help when the styling changes around a running editor, but might be a little more robust. Does that work for you?

I think there's also an issue when there are multiple scrolling parents (which might be made worse by my proposed change). Would it make sense to have an array of scrolling parents, rather than a single one?

[cjbest]

Those are good ideas. I started withgetComputedStyle but it broke in my use case, because it was indeed getting updated after. Perhaps a hybrid of both would be ideal - I expect it’s not expensive too listen to scroll on something that never fires it.

I can take a crack at this soon.

[cjbest]

@marijnh how about this updated approach: just listen for scroll on all parents + window?

Just adding/removing a listener for an event that never gets fired is cheap, and it makes this implementation quite simple and robust.

It turns out for my use case I was both changing the size (as the user typed beyond 1 page) and changing the computed overflow-y (in a media query) but doing it this way makes it pretty unbreakable..

[marijnh]

Thanks, that looks like a good idea. Could you update the name of the function that gets the parent nodes to reflect its current behavior? That's the last nitpick I have, I think.

[cjbest]

How about getAllWrapping?

[marijnh]

That works. Merged as 7f442f0, and released as 1.0.3.

[jabbermarky]

@cjbest - I'd like to make a change to the menubar so that I can pass a top offset. I'm not sure how your scrollAncestor change is supposed to work. In the demo example, the scrollAncestor is always undefined, and therefore, the top position of the menubar is never changed, i.e. this test in updateFloat() in menubar.js always fails:
if (scrollAncestor) this.menu.style.top = top + "px"

[cjbest]

@jabbermarky the scrollAncestor gets set when the thing that is scrolling is not the whole document, but rather some other enclosing container.

For example, lets say you want to have a top bar that's above the editor. You could make a two-pane flexbox layout, with the the bottom pane flexing and having overflow-y: scroll and put the prosemirror (with menubar) in it.

[marijnh]

Ah, I missed this new discussion before opening #18—but yeah, see my comment there.