Don't scroll parents of absolutely-positioned elements

nperez0111 commented

2025-02-25 18:27:50 +01:00

(Migrated from github.com)

FIX: Don't try to scroll absolutely-positioned elements into view by scrolling their parent elements.

I've discovered a minor bug with how scrollToSelection works within prosemirror-view as well as an easy fix for it.

Reproduction

Here is a screen recording which shows the issue occurring. After some modifications to the CSS to re-create the layout of my use-case.

https://github.com/user-attachments/assets/2b31e615-310b-42ea-8c2f-d3367b2441ab

To reproduce this, I wrote a small script that can be pasted into a browser's console on this example. Though, the specific values will likely need to be modified as it depends on your viewport's size.

document.querySelector('body > article:nth-child(3)').style.overflow = 'auto'
document.querySelector('body > article:nth-child(3)').style.height = '900px'
document.getElementById('editor').style.position = 'absolute'
document.getElementById('editor').style.top = '1000px'

The gist of this reproduction is that a parent of the editor element has absolute positioning and is a descendant of an element which is scrollable.

It is invalid to be scrolling the parent, because the editor is absolutely positioned and therefore in a different context as it's ancestor.

Potential Solution

When recursively searching for offsets to scroll by, there already is an existing check to stop iterating when an element has either fixed or sticky positioning, I believe this will also need to check for absolute positioning. Like so:

diff --git a/src/domcoords.ts b/src/domcoords.ts
index 5a693c3..a1586eb 100644
--- a/src/domcoords.ts
+++ b/src/domcoords.ts
@@ -60,7 +60,7 @@ export function scrollRectIntoView(view: EditorView, rect: Rect, startDOM: Node)
         rect = {left: rect.left - dX, top: rect.top - dY, right: rect.right - dX, bottom: rect.bottom - dY}
       }
     }
-    if (atTop || /^(fixed|sticky)$/.test(getComputedStyle(parent as HTMLElement).position)) break
+    if (atTop || /^(fixed|sticky|absolute)$/.test(getComputedStyle(parent as HTMLElement).position)) break
   }
 }

The reasoning is that, like fixed and sticky positioned elements, absolutely positioned elements break from their parent's layout (especially when positioned relative to another element with position: relative). Therefore, it is not valid to attempt to scroll the parent of an absolutely positioned element, since is not within the same layout context.

FIX: Don't try to scroll absolutely-positioned elements into view by scrolling their parent elements. I've discovered a minor bug with how `scrollToSelection` works within `prosemirror-view` as well as an easy fix for it. ### Reproduction Here is a screen recording which shows the issue occurring. After some modifications to the CSS to re-create the layout of my use-case. https://github.com/user-attachments/assets/2b31e615-310b-42ea-8c2f-d3367b2441ab To reproduce this, I wrote a small script that can be pasted into a browser's console on [this example](https://prosemirror.net/examples/basic/). Though, the specific values will likely need to be modified as it depends on your viewport's size. ```js document.querySelector('body > article:nth-child(3)').style.overflow = 'auto' document.querySelector('body > article:nth-child(3)').style.height = '900px' document.getElementById('editor').style.position = 'absolute' document.getElementById('editor').style.top = '1000px' ``` The gist of this reproduction is that a parent of the editor element has absolute positioning and is a descendant of an element which is scrollable. It is invalid to be scrolling the parent, because the editor is absolutely positioned and therefore in a different context as it's ancestor. ### Potential Solution When recursively searching for offsets to scroll by, there already is an existing check [to stop iterating when an element has either `fixed` or `sticky` positioning](https://github.com/ProseMirror/prosemirror-view/blob/bbd0ab23e2bbdcfe8213bba003d5f4e69e6dc73f/src/domcoords.ts#L63), I believe this will also need to check for `absolute` positioning. Like so: ```diff diff --git a/src/domcoords.ts b/src/domcoords.ts index 5a693c3..a1586eb 100644 --- a/src/domcoords.ts +++ b/src/domcoords.ts @@ -60,7 +60,7 @@ export function scrollRectIntoView(view: EditorView, rect: Rect, startDOM: Node) rect = {left: rect.left - dX, top: rect.top - dY, right: rect.right - dX, bottom: rect.bottom - dY} } } - if (atTop || /^(fixed|sticky)$/.test(getComputedStyle(parent as HTMLElement).position)) break + if (atTop || /^(fixed|sticky|absolute)$/.test(getComputedStyle(parent as HTMLElement).position)) break } } ``` The reasoning is that, like `fixed` and `sticky` positioned elements, absolutely positioned elements break from their parent's layout (especially when positioned relative to another element with `position: relative`). Therefore, it is not valid to attempt to scroll the parent of an absolutely positioned element, since is not within the same layout context.

marijnh commented

2025-02-26 08:13:35 +01:00

(Migrated from github.com)

Stopping this loop for absolutely positioned elements doesn't seem like a good idea. It is entirely possible for such an element to be within a scrollable parent that needs to be scrolled to display the cursor.

In fact, your screencast doesn't seem to show a regular absolutely positioned element—I'd expect that to move along with the rest of the page, as the page scrolls.

Stopping this loop for absolutely positioned elements doesn't seem like a good idea. It is entirely possible for such an element to be within a scrollable parent that needs to be scrolled to display the cursor. In fact, your screencast doesn't seem to show a regular absolutely positioned element—I'd expect that to move along with the rest of the page, as the page scrolls.

nperez0111 commented

2025-02-26 09:59:43 +01:00

(Migrated from github.com)

Stopping this loop for absolutely positioned elements doesn't seem like a good idea. It is entirely possible for such an element to be within a scrollable parent that needs to be scrolled to display the cursor.

The element definitely can be within a scrollable parent, but it isn't a 1-1 correlation that the scrollable parent would actually affect the positioning of an absolutely positioned element (i.e. to actually make the element be within view).

In fact, your screencast doesn't seem to show a regular absolutely positioned element—I'd expect that to move along with the rest of the page, as the page scrolls.

The reason that the element does not scroll is that in my screen recording, the element which is scrolling does not have position: relative set on it, causing the absolute positioning to be relative to the page. You can reproduce it with this example: https://stackblitz.com/edit/stackblitz-starters-5yhtzbdi?file=styles.css

Video:

https://github.com/user-attachments/assets/beaad0cf-d711-4306-b1e0-c9dab632c10f

Which is the problem, absolutely positioned elements are only absolutely positioned relative to their nearest non-static position.

So, I guess that the technically correct thing to do here is that, for absolutely positioned elements, is to skip any ancestors which are not statically positioned after encountering an absolutely positioned element.

I think something like this patch should make it more sound than ignoring absolutely positioned elements altogether

> Stopping this loop for absolutely positioned elements doesn't seem like a good idea. It is entirely possible for such an element to be within a scrollable parent that needs to be scrolled to display the cursor. The element definitely can be within a scrollable parent, but it isn't a 1-1 correlation that the scrollable parent would actually affect the positioning of an absolutely positioned element (i.e. to actually make the element be within view). > In fact, your screencast doesn't seem to show a regular absolutely positioned element—I'd expect that to move along with the rest of the page, as the page scrolls. The reason that the element does not scroll is that in my screen recording, the element which is scrolling does not have `position: relative` set on it, causing the `absolute` positioning to be relative to the page. You can reproduce it with this example: https://stackblitz.com/edit/stackblitz-starters-5yhtzbdi?file=styles.css Video: https://github.com/user-attachments/assets/beaad0cf-d711-4306-b1e0-c9dab632c10f Which is the problem, absolutely positioned elements are only absolutely positioned relative to their nearest [non-static position](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_display/Containing_block#identifying_the_containing_block). So, I guess that the technically correct thing to do here is that, for absolutely positioned elements, is to skip any ancestors which are not statically positioned after encountering an absolutely positioned element. I think something like this patch should make it more sound than ignoring absolutely positioned elements altogether

marijnh commented

2025-02-26 13:51:21 +01:00

(Migrated from github.com)

I think using offsetParent, as in attached patch, makes this skipping more straightforward.

I think using `offsetParent`, as in attached patch, makes this skipping more straightforward.

nperez0111 commented

2025-02-26 13:52:32 +01:00

(Migrated from github.com)

I totally agree, wasn't aware of that API.

Appreciate your help here @marijnh!

I totally agree, wasn't aware of that API. Appreciate your help here @marijnh!

YousefED commented