-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements to getOptimalPosition to handle cases with scrollable ancestors #14755
Conversation
…getVisible(). Rect#getVisible() should exclude parents with overflow: visible.
… target went off the boundaries.
…o in corner cases with multiple scrollable ancestors.
…more real-life scenarios.
…tyle preset with diagonal lines.
…essed the issue of the BlockToolbar not positioning properly due to window scroll.
After this improvements will work we can take a closer look at those two issues: |
I have one scenario detected by our E2E tests which is a regression compared to the Simplified steps:
Result
|
This issue #14755 (comment) fixed with 👉 c13d551 |
"This link has no url" message in the link balloon after saving changesSteps
Result link-actual.mp4 |
We confirm that both issues ☝️ |
…rmation while intersecting with another Rect.
@@ -261,16 +267,53 @@ export default class Rect { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code should at least warn
if !parent
. Using getVisible()
with an abstract rect that has no clear _source
such as DOM range or DOM element makes little sense. It's always fully visible. But doing so in the code may produce confusing results that are very hard to debug.
FYI: I tried to throw here to see how many tests will fail and there are 30+ in utils+ui. They are mostly caused by places like this one where we mock stuff instead of using some real elements
ckeditor5/packages/ckeditor5-ui/tests/panel/balloon/balloonpanelview.js
Lines 1351 to 1358 in d8672c3
targetRect = new Rect( { | |
top: 200, | |
bottom: 400, | |
left: 50, | |
right: 100, | |
width: 0, | |
height: 0 | |
} ); |
Anyway, this is not a blocker but IMO makes for a serious follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, noticed it also... well it's caused cause getVisible
will now works only real DOM elements, and finding intersection between two Rect
's should only be done using getIntersection
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up 👉 #14964
I think we're ready for a merge 👍 |
Suggested merge commit message (convention)
Fix (utils): Balloon panels should not sticks out of the visible part of the editor while scrolling.
Closes #5328.
Closes #14536.
Closes #14667.
Closes #13307.
Closes #10846.
Closes #5499.
Closes #2584.
Closes #5354.
Additional information
The
getOptimalPosition
family tree:https://www.figma.com/file/KBYn1Jv4jbxxul5bwiylfZ/getOptimalPosition-family-tree?type=whiteboard&node-id=0%3A1&t=WFmuNW74Z3beCAWQ-1
TODOS
StreamlineStickyPanelView
logicATM it usesgetScrollableAncestors()
andgetElementsIntersectionRect()
. I think it should depend onRect#getVisible()
instead.Then we can ditch API that is no longer needed.getScrollableAncestors()
getElementsIntersectionRect()
If there's no way to move the logic on top ofRect#getVisible()
, these helpers should move toStickyPanelView
as a private API. They're unlisted in docs anyway, so this should not cause breaking changes.Addressed in 4a77bf9
viewportStickyNorth engaging when not necessary
I think
defaultPositions.viewportStickyNorth
should be used byWidgetToolbarRepository
only. Despite recent changes aimed to keep it in check 199ea15, it still engages in some cases.Probably addressed in 2d19f70.
viewportStickyNorth acting up in case of long tablesThis has something to do withckeditor5/packages/ckeditor5-ui/src/panel/balloon/balloonpanelview.ts
Lines 1159 to 1161 in c14d3f1
Addressed in 2d19f70.
BlockToolbar button misaligned when the window is scrolledWe ditchedgetOptimalPosition()
for button (not the panel) positioning because there's nothing to choose from, really. We used a simple substitute but we didn't account for the fact thatgetOptimalPosition()
returnsposition: absolute
-friendly coordinates.Fixed in cf5e63a.