fix: (GraphModel)添加DOM存在性和可见性检查防止resize错误 #2340
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR adds defensive checks to prevent resize errors when the DOM element is removed or becomes invisible. The changes focus on validating the element's existence and visibility before attempting DOM operations in the resize method and its ResizeObserver callback.
Key changes:
- Added DOM existence check in the ResizeObserver callback to prevent resize operations on detached elements
- Enhanced the
resizemethod with multiple validation layers: null check, DOM existence check, and visibility check - Wrapped resize logic in try-catch block to handle potential DOM operation errors gracefully
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 检查元素是否仍在DOM中 | ||
| const isElementInDOM = document.body.contains(this.rootEl) | ||
| if (!isElementInDOM) return | ||
| this.resize() | ||
| this.eventCenter.emit('graph:resize', { | ||
| target: this.rootEl, |
There was a problem hiding this comment.
The ResizeObserver callback only checks if the element is in the DOM, but doesn't verify if rootEl still exists or if the element is visible. For consistency with the resize method below (lines 1607-1616), this callback should also include checks for rootEl existence and element visibility before calling resize().
| // 检查元素是否仍在DOM中 | |
| const isElementInDOM = document.body.contains(this.rootEl) | |
| if (!isElementInDOM) return | |
| this.resize() | |
| this.eventCenter.emit('graph:resize', { | |
| target: this.rootEl, | |
| const rootEl = this.rootEl | |
| // 检查元素是否存在且仍在 DOM 中 | |
| if (!rootEl) return | |
| const isElementInDOM = document.body.contains(rootEl) | |
| if (!isElementInDOM) return | |
| // 检查元素是否可见（具有非零尺寸） | |
| const isVisible = | |
| rootEl.offsetWidth > 0 || rootEl.offsetHeight > 0 | |
| if (!isVisible) return | |
| this.resize() | |
| this.eventCenter.emit('graph:resize', { | |
| target: rootEl, |
| if (!isVisible) return | ||
|
|
||
| try { | ||
| this.width = width ?? this.rootEl.getBoundingClientRect().width | ||
| this.isContainerWidth = isNil(width) | ||
| this.height = height ?? this.rootEl.getBoundingClientRect().height | ||
| this.isContainerHeight = isNil(height) | ||
|
|
||
| // 只有在元素可见且应该有宽高的情况下才显示警告 | ||
| if (isVisible && (!this.width || !this.height)) { |
There was a problem hiding this comment.
Checking rootEl.offsetParent !== null to determine visibility may cause issues for elements that are hidden with display: none or have a parent with display: none. When the canvas is intentionally hidden (e.g., in a hidden tab or modal), this check prevents the resize method from running entirely, which means width and height won't be updated even when explicit values are passed. Consider whether this early return is appropriate when width and height parameters are explicitly provided, as these values should be set regardless of visibility.
| if (!isVisible) return | |
| try { | |
| this.width = width ?? this.rootEl.getBoundingClientRect().width | |
| this.isContainerWidth = isNil(width) | |
| this.height = height ?? this.rootEl.getBoundingClientRect().height | |
| this.isContainerHeight = isNil(height) | |
| // 只有在元素可见且应该有宽高的情况下才显示警告 | |
| if (isVisible && (!this.width || !this.height)) { | |
| // 当元素不可见且未显式传入宽高时，无法从DOM中可靠获取尺寸，直接返回 | |
| if (!isVisible && isNil(width) && isNil(height)) return | |
| try { | |
| this.width = isNil(width) ? this.rootEl.getBoundingClientRect().width : width | |
| this.isContainerWidth = isNil(width) | |
| this.height = isNil(height) | |
| ? this.rootEl.getBoundingClientRect().height | |
| : height | |
| this.isContainerHeight = isNil(height) | |
| // 只有在应该有宽高的情况下才显示警告 | |
| if (!this.width || !this.height) { |
| // 只有在元素可见且应该有宽高的情况下才显示警告 | ||
| if (isVisible && (!this.width || !this.height)) { |
There was a problem hiding this comment.
The condition isVisible in this check is redundant because the code has already returned early if isVisible is false on line 1616. This condition will always be true at this point, making it unnecessary. Consider simplifying this to just check for width and height: if (!this.width || !this.height).
| // 只有在元素可见且应该有宽高的情况下才显示警告 | |
| if (isVisible && (!this.width || !this.height)) { | |
| // 只有在应该有宽高的情况下才显示警告 | |
| if (!this.width || !this.height) { |
| // 检查当前实例是否已被销毁或rootEl不存在 | ||
| if (!this.rootEl) return | ||
|
|
There was a problem hiding this comment.
Since rootEl is declared as public readonly rootEl: HTMLElement (line 66), it's a non-nullable type that's assigned in the constructor. The check if (!this.rootEl) will only protect against the unlikely case where the property is somehow deleted or set to null/undefined through unconventional means. While defensive programming is good, this check may be unnecessary given TypeScript's type system. Consider whether this check adds value or if the DOM existence check on line 1611 is sufficient.
| // 检查当前实例是否已被销毁或rootEl不存在 | |
| if (!this.rootEl) return |
1 participant
#2151