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

cjbest commented

2018-02-13 19:35:33 +01:00

(Migrated from github.com)

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.

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 commented

2018-02-13 21:37:00 +01:00

(Migrated from github.com)

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?

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 commented

2018-02-13 21:58:52 +01:00

(Migrated from github.com)

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.

Those are good ideas. I started with`getComputedStyle` 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 commented

2018-02-15 05:35:22 +01:00

(Migrated from github.com)

@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 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 commented

2018-02-15 12:02:13 +01:00

(Migrated from github.com)

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 commented

2018-02-15 19:33:06 +01:00

(Migrated from github.com)

How about getAllWrapping?

How about `getAllWrapping`?

marijnh commented

2018-02-15 21:42:21 +01:00

(Migrated from github.com)

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

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

🎉 1

jabbermarky commented

2018-03-12 21:59:43 +01:00

(Migrated from github.com)

@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 - 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 commented

2018-03-13 01:18:58 +01:00

(Migrated from github.com)

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

@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 commented

2018-03-13 08:44:06 +01:00

(Migrated from github.com)

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

Rows
Columns

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