-
Notifications
You must be signed in to change notification settings - Fork 19.8k
fix(matrix): trigger click event on matrix cells #21390
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for your contribution! Please DO NOT commit the files in dist, i18n, and ssr/client/dist folders in a non-release pull request. These folders are for release use only. |
Ovilia
left a comment
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.
Matrix is a layout so its text in the header or body should not trigger 'click' event by default. If you need this information, you need to add a new option matrix.triggerEvent (default false) like https://echarts.apache.org/en/option.html#yAxis.triggerEvent.
|
@Ovilia I've updated the implementation to add the triggerEvent option (default: false) following the same pattern as I would appreciate it if you could take another look. |
Ovilia
left a comment
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.
Please also add test cases in test/matrix.html.
src/component/matrix/MatrixView.ts
Outdated
| name: textValue != null ? textValue + '' : null, | ||
| coord: xyLocator.slice() | ||
| }; | ||
| getECData(cellRect).eventData = eventData; |
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.
It seems that events only trigger when there is text, correct me if I'm wrong. So what's the meaning of setting eventData of cellRect here?
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.
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.
Pull request overview
This PR fixes a bug where click events were not being triggered on matrix component cells. It adds a triggerEvent option to the matrix component (defaulting to false) and implements the necessary eventData attachment to enable click event propagation when enabled.
Key Changes:
- Adds
triggerEventboolean option toMatrixOptioninterface with a default value offalse - Updates
createMatrixCellfunction to attach eventData to cell text elements whentriggerEventis enabled - Adjusts silent property logic to account for both
triggerEventand tooltip interactions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coord/matrix/MatrixModel.ts | Adds triggerEvent option to MatrixOption interface and sets default to false |
| src/component/matrix/MatrixView.ts | Imports getECData, adds targetType parameter to createMatrixCell, attaches eventData to cellText when triggerEvent is enabled, and updates silent property logic |
| test/ut/spec/component/matrix/event.test.ts | Adds comprehensive unit tests for verifying click events work when triggerEvent is true and eventData is not attached when false |
| test/matrix.html | Adds interactive demo showing click events on matrix cells with visual feedback |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (triggerEvent && cellText) { | ||
| const eventData = { | ||
| componentType: 'matrix' as const, | ||
| componentIndex: matrixModel.componentIndex, | ||
| matrixIndex: matrixModel.componentIndex, | ||
| targetType: targetType, | ||
| name: textValue != null ? textValue + '' : null, | ||
| coord: xyLocator.slice() | ||
| }; | ||
| getECData(cellText).eventData = eventData; | ||
| } |
Copilot
AI
Dec 10, 2025
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.
The eventData is only attached to cellText, which means cells without text values (where textValue is null/undefined) won't trigger click events even when triggerEvent is true. Consider whether eventData should also be attached to cellRect for cells without text to ensure consistent event handling across all matrix cells.
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 also have this doubt. @natsuokawai Do you think it be useful to cells without text to trigger events?
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.
@Ovilia
I figured that cells without text (so, no actual data) probably don't need to trigger events.
The way I see it, ECharts events are mainly for when you want to do something with the data shown at a particular point.
| it('should trigger click event on matrix cell when triggerEvent is true', function () { | ||
| const option = { | ||
| matrix: { | ||
| triggerEvent: true, | ||
| x: { | ||
| data: ['A', 'B'] | ||
| }, | ||
| y: { | ||
| data: ['Y'] | ||
| }, | ||
| body: { | ||
| data: [ | ||
| { coord: [0, 0], value: 'Cell A' } | ||
| ] | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| chart.setOption(option); | ||
|
|
||
| let clickedParams: any = null; | ||
|
|
||
| chart.on('click', function (params) { | ||
| clickedParams = params; | ||
| }); | ||
|
|
||
| // Find the matrix cell element | ||
| const zr = chart.getZr(); | ||
| const displayList = zr.storage.getDisplayList(); | ||
|
|
||
| let targetEl; | ||
| for (let i = 0; i < displayList.length; i++) { | ||
| const el = displayList[i]; | ||
| const ecData = getECData(el); | ||
| if (ecData && ecData.eventData && ecData.eventData.name === 'Cell A') { | ||
| // Find the cell with the specific name | ||
| targetEl = el; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| expect(targetEl).toBeDefined(); | ||
|
|
||
| // Trigger click | ||
| zr.trigger('click', { | ||
| target: targetEl, | ||
| offsetX: 10, // Dummy | ||
| offsetY: 10 // Dummy | ||
| }); | ||
|
|
||
| expect(clickedParams).not.toBeNull(); | ||
| expect(clickedParams.componentType).toEqual('matrix'); | ||
| expect(clickedParams.matrixIndex).toEqual(0); | ||
| expect(clickedParams.targetType).toEqual('body'); | ||
| expect(clickedParams.name).toEqual('Cell A'); | ||
| expect(clickedParams.coord).toEqual([0, 0]); | ||
| }); |
Copilot
AI
Dec 10, 2025
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.
Consider adding test coverage for cells without text values to verify whether click events should (or should not) be triggered on cells that only have background styling but no text content. This would help clarify the intended API behavior.
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.
Fixed in 5530423
5530423 to
6609e32
Compare
Brief Information
This pull request is in the type of:
What does this PR do?
Fixes an issue where click events were not triggered on matrix component cells.
Fixed issues
Details
Before: What was the problem?
Even when
matrix.body.silentwas set tofalse(and the cell was made hit-testable, e.g., by settingitemStyle.colorto'transparent'), click events on matrix cells were not being emitted by the ECharts instance. This was because the underlying graphic elements (rects and texts) created inMatrixViewlacked the necessaryeventData(ECData) to identify them as part of the matrix component.After: How does it behave after the fixing?
Click events are now correctly triggered when clicking on matrix cells. The event parameters include
componentType: 'matrix',componentIndex, and the cell'sname.This is achieved by:
getECDatainsrc/component/matrix/MatrixView.ts.eventDatato the cell's text elements during creation increateMatrixCell.events-on-matrix-cell-text.mov
Document Info
One of the following should be checked.
Misc
Security Checking
ZRender Changes
Related test cases or examples to use the new APIs
Added a new test case in
test/ut/spec/component/matrix/event.test.tsto verify the fix.Merging options
Other information