SEPURI-SAI-KRISHNA opened a new pull request, #21735:
URL: https://github.com/apache/echarts/pull/21735
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
Fixes two crashes in the tree series: rendering a tree with no laid-out
nodes, and removing several nodes in one update.
### Fixed issues
<!-- No existing issue; found while auditing tree update paths. -->
## Details
### Before: What was the problem?
Two independent crashes, both hit by ordinary tree data updates.
**1. A tree with no nodes to lay out throws.**
```js
chart.setOption({series: [{type: 'tree', data: []}]});
```
```
TypeError: Cannot read properties of null (reading '0')
at legacyCopyOverallTrans (src/coord/View.ts:567)
at viewCoordSysUpdateOverallTrans (src/coord/View.ts:548)
...
at TreeView._updateViewCoordSys (src/chart/tree/TreeView.ts)
```
`_updateViewCoordSys` collects node positions and computes their extent:
```js
const min: number[] = [];
const max: number[] = [];
bbox.fromPoints(points, min, max);
```
`bbox.fromPoints` returns immediately without writing anything when `points`
is
empty, so `min` and `max` stay empty arrays. `max[0] - min[0]` is then
`NaN`, the
existing zero-size corrections below only test `=== 0` so they never fire,
and the
dataRect handed to the view coordinate system is `NaN`. That makes the
resulting
matrix non-invertible, `matrix.invert` returns `null`, and
`legacyCopyOverallTrans`
dereferences it.
This is not only the literal `data: []` case — it also covers a tree whose
nodes
have no valid layout yet, e.g. rendering before an async fetch resolves, or
after
filtering the data down to nothing.
**2. Removing several nodes in one update throws.**
```js
chart.setOption({animation: false, series: [{type: 'tree', data:
fullTree}]});
// drop a subtree
chart.setOption({series: [{type: 'tree', data: smallerTree}]}, true);
```
```
TypeError: Cannot read properties of null (reading '__edge')
at removeNodeEdge (src/chart/tree/TreeView.ts:590)
at removeNode (src/chart/tree/TreeView.ts:690)
at DataDiffer._remove
```
`removeNodeEdge` guards its own node's graphic element but not its source's:
```js
const symbolEl = data.getItemGraphicEl(node.dataIndex) as TreeSymbol;
if (!symbolEl) {
return;
}
const sourceSymbolEl = data.getItemGraphicEl(source.dataIndex) as TreeSymbol;
const sourceEdge = sourceSymbolEl.__edge; // <- source may already be gone
```
`removeNode` sets `data.setItemGraphicEl(dataIndex, null)` in the removal
callback.
With `animation: false` that callback runs synchronously, so when a batch of
nodes
is removed the source node's element is frequently already `null` by the
time its
children's edges are cleaned up.
This one is independent of the empty-data case: it fires whenever removed
nodes
include a parent, even when the resulting tree is not empty (dropping a
subtree, or
collapsing back to just the root).
### After: How does it behave after the fixing?
1. `min`/`max` are seeded when there is no valid point — from the previous
extent if
there is one (the mechanism already used for the collapsed-root case),
otherwise
from zero. The existing zero-size corrections then expand it into a
usable rect,
so the view transform stays invertible.
2. `sourceSymbolEl` is guarded exactly like `symbolEl` immediately above it.
`sourceEdge` is only used as a fallback and everything downstream is
already
behind `if (edge)`, so a missing source simply means there is no edge to
remove.
Emptying a tree, refilling it, and removing subtrees all work with animation
on and off.
## Document Info
One of the following should be checked.
- [x] This PR doesn't relate to document changes
- [ ] The document should be updated later
- [ ] The document changes have been made in apache/echarts-doc#xxx
## Misc
### Security Checking
- [ ] This PR uses security-sensitive Web APIs.
### ZRender Changes
- [ ] This PR depends on ZRender changes (ecomfe/zrender#xxx).
### Related test cases or examples to use the new APIs
Added `test/ut/spec/series/treeUpdate.test.ts`, covering an initially empty
tree, a
tree emptied after having data, removing a subtree, removing all but the
root, and
refilling an empty tree — with `animation` both `false` and `true`.
On `master` the empty-data cases fail regardless of animation, and the
node-removal
cases fail with `animation: false`. The `animation: true` removal cases pass
before
and after, and are kept to document that the timing of the removal callback
is what
exposes the second bug.
`npm run test`, `npx tsc --noEmit` and `eslint` on the changed file all pass.
### Merging options
- [x] Please squash the commits into a single one when merging.
### Other information
`src/chart/tree/TreeView.ts` is also touched by #18491, #21603 and #21681.
Those
changes are in different functions from the two guards here, though
whichever lands
first may need a trivial rebase.
Both crashes end in the same place from a user's point of view — clearing or
shrinking tree data — so they are fixed together rather than split across
two PRs
that would conflict in the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
