refactor(interaction): Enhance touch event handling with passive options #4056
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR enhances touch event handling by adding the passive option to touch event listeners for improved scrolling performance. When preventDefault is disabled (default), touch events are registered with {passive: true} to allow smooth scrolling. When preventDefault is enabled (boolean true or number threshold), events use {passive: false} to allow calling preventDefault().
Key changes:
- Added passive option logic to
eventrect.tsbased onpreventDefaultconfiguration - Applied
{passive: true}to touch events in shape-specific handlers (arc, radar, treemap, funnel) - Updated documentation to describe the passive option behavior
- Added comprehensive test coverage for passive option in various chart types
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/ChartInternal/interactions/eventrect.ts |
Core implementation: calculates passiveOption based on preventDefault setting and applies it to touch event listeners |
src/ChartInternal/shape/arc.ts |
Applies {passive: true} to arc chart touch events (pie, donut, gauge) |
src/ChartInternal/shape/radar.ts |
Applies {passive: true} to radar chart touch events |
src/ChartInternal/shape/treemap.ts |
Applies {passive: true} to treemap chart touch events |
src/ChartInternal/shape/funnel.ts |
Applies {passive: true} to funnel chart touch events |
src/ChartInternal/ChartInternal.ts |
Applies {passive: true} to SVG-level touch events when touch input is enabled |
src/config/Options/interaction/interaction.ts |
Documents the passive option behavior in the API documentation |
src/ChartInternal/internals/tooltip.ts |
Removes IE9-specific visibility style (unrelated cleanup) |
test/interactions/interaction-touch-spec.ts |
Adds test suites to verify passive option is correctly applied for various scenarios and chart types |
Comment on lines
269
to
333
| describe("passive option for arc types", () => { | ||
| let addEventListenerSpy; | ||
|
|
||
| it("should add touch events with passive: true for pie chart", () => { | ||
| // Spy on addEventListener before chart creation | ||
| const originalAddEventListener = Element.prototype.addEventListener; | ||
| const touchstartCalls = []; | ||
|
|
||
| addEventListenerSpy = sinon.stub(Element.prototype, "addEventListener").callsFake(function(type, listener, options) { | ||
| if (type === "touchstart") { | ||
| touchstartCalls.push({ type, options }); | ||
| } | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| }); | ||
|
|
||
| // Create pie chart | ||
| args = { | ||
| data: { | ||
| columns: [ | ||
| ["data1", 50], | ||
| ["data2", 50] | ||
| ], | ||
| type: "pie" | ||
| }, | ||
| interaction: { | ||
| inputType: { | ||
| touch: true | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| chart = util.generate(args); | ||
|
|
||
| // Check if touchstart events were added with passive: true | ||
| const hasPassiveTrue = touchstartCalls.some(call => { | ||
| const options = call.options; | ||
| return options && options.passive === true; | ||
| }); | ||
|
|
||
| expect(touchstartCalls.length).to.be.above(0); | ||
| expect(hasPassiveTrue).to.be.true; | ||
|
|
||
| addEventListenerSpy.restore(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The same issues identified for "passive event listener option" apply here:
- Conflict with global
beforeEachcausing double chart generation - Missing
afterEachhook for spy cleanup
Consider the same restructuring approach with beforeAll to skip the global beforeEach and an afterEach hook for proper cleanup.
| /^(touchstart|mouseover)$/.test(event.type) && $$.setOverOut(true, data); | ||
| } | ||
| }) | ||
| }, isTouch ? {passive: true} : undefined) |
There was a problem hiding this comment.
Missing space after {passive:. Should be {passive: true} for consistency with the coding style used elsewhere in the codebase.
|
|
||
| // hide tooltip | ||
| tooltip | ||
| .style("display", "none") |
There was a problem hiding this comment.
[nitpick] This change appears unrelated to the PR's stated purpose of "enhancing touch event handling with passive options". The removal of the IE9-specific visibility style is a separate concern and should ideally be in its own commit/PR for better change tracking and git history clarity.
Comment on lines
135
to
275
| describe("passive event listener option", () => { | ||
| let addEventListenerSpy; | ||
|
|
||
| it("should add touch events with passive: true when preventDefault is false", () => { | ||
| // Spy on addEventListener before chart creation | ||
| const originalAddEventListener = Element.prototype.addEventListener; | ||
| const touchstartCalls = []; | ||
|
|
||
| addEventListenerSpy = sinon.stub(Element.prototype, "addEventListener").callsFake(function(type, listener, options) { | ||
| if (type === "touchstart") { | ||
| touchstartCalls.push({ type, options }); | ||
| } | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| }); | ||
|
|
||
| // Create chart with touch input enabled and preventDefault disabled | ||
| args = { | ||
| data: { | ||
| columns: [ | ||
| ["data1", 10, 20, 30], | ||
| ["data2", 20, 16, 18] | ||
| ], | ||
| type: "line" | ||
| }, | ||
| interaction: { | ||
| inputType: { | ||
| touch: true | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| chart = util.generate(args); | ||
|
|
||
| // Check if touchstart events were added with passive: true | ||
| const hasPassiveTrue = touchstartCalls.some(call => { | ||
| const options = call.options; | ||
| return options && options.passive === true; | ||
| }); | ||
|
|
||
| expect(touchstartCalls.length).to.be.above(0); | ||
| expect(hasPassiveTrue).to.be.true; | ||
|
|
||
| addEventListenerSpy.restore(); | ||
| }); | ||
|
|
||
| it("should add touch events with passive: false when preventDefault is true", () => { | ||
| // Spy on addEventListener before chart creation | ||
| const originalAddEventListener = Element.prototype.addEventListener; | ||
| const touchCalls = []; | ||
|
|
||
| addEventListenerSpy = sinon.stub(Element.prototype, "addEventListener").callsFake(function(type, listener, options) { | ||
| if (type === "touchstart" || type === "touchmove") { | ||
| touchCalls.push({ type, options }); | ||
| } | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| }); | ||
|
|
||
| // Create chart with preventDefault enabled | ||
| args = { | ||
| data: { | ||
| columns: [ | ||
| ["data1", 10, 20, 30], | ||
| ["data2", 20, 16, 18] | ||
| ], | ||
| type: "line" | ||
| }, | ||
| interaction: { | ||
| inputType: { | ||
| touch: { | ||
| preventDefault: true | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| chart = util.generate(args); | ||
|
|
||
| // Check if touch events were added with passive: false | ||
| const allPassiveFalse = touchCalls.every(call => { | ||
| const options = call.options; | ||
| return options && options.passive === false; | ||
| }); | ||
|
|
||
| expect(touchCalls.length).to.be.above(0); | ||
| expect(allPassiveFalse).to.be.true; | ||
|
|
||
| addEventListenerSpy.restore(); | ||
| }); | ||
|
|
||
| it("should add touch events with passive: false when preventDefault is a number", () => { | ||
| // Spy on addEventListener before chart creation | ||
| const originalAddEventListener = Element.prototype.addEventListener; | ||
| const touchCalls = []; | ||
|
|
||
| addEventListenerSpy = sinon.stub(Element.prototype, "addEventListener").callsFake(function(type, listener, options) { | ||
| if (type === "touchstart" || type === "touchmove") { | ||
| touchCalls.push({ type, options }); | ||
| } | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| }); | ||
|
|
||
| // Create chart with preventDefault as a number | ||
| args = { | ||
| data: { | ||
| columns: [ | ||
| ["data1", 10, 20, 30], | ||
| ["data2", 20, 16, 18] | ||
| ], | ||
| type: "line" | ||
| }, | ||
| interaction: { | ||
| inputType: { | ||
| touch: { | ||
| preventDefault: 50 | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| chart = util.generate(args); | ||
|
|
||
| // Check if touch events were added with passive: false | ||
| const allPassiveFalse = touchCalls.every(call => { | ||
| const options = call.options; | ||
| return options && options.passive === false; | ||
| }); | ||
|
|
||
| expect(touchCalls.length).to.be.above(0); | ||
| expect(allPassiveFalse).to.be.true; | ||
|
|
||
| addEventListenerSpy.restore(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The addEventListenerSpy should be cleaned up in an afterEach hook rather than at the end of each test. If a test fails before reaching restore(), the spy will remain stubbed and affect subsequent tests. Consider adding an afterEach hook to ensure cleanup:
describe("passive event listener option", () => {
let addEventListenerSpy;
afterEach(() => {
if (addEventListenerSpy) {
addEventListenerSpy.restore();
addEventListenerSpy = null;
}
});
// ... tests
});
Comment on lines
315
to
467
| describe("passive option for other shape types", () => { | ||
| let addEventListenerSpy; | ||
|
|
||
| it("should add touch events with passive: true for radar chart", () => { | ||
| // Spy on addEventListener before chart creation | ||
| const originalAddEventListener = Element.prototype.addEventListener; | ||
| const touchstartCalls = []; | ||
|
|
||
| addEventListenerSpy = sinon.stub(Element.prototype, "addEventListener").callsFake(function(type, listener, options) { | ||
| if (type === "touchstart") { | ||
| touchstartCalls.push({ type, options }); | ||
| } | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| }); | ||
|
|
||
| // Create radar chart | ||
| args = { | ||
| data: { | ||
| columns: [ | ||
| ["data1", 30, 200, 100], | ||
| ["data2", 130, 100, 140] | ||
| ], | ||
| type: "radar" | ||
| }, | ||
| interaction: { | ||
| inputType: { | ||
| touch: true | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| chart = util.generate(args); | ||
|
|
||
| // Check if touchstart events were added with passive: true | ||
| const hasPassiveTrue = touchstartCalls.some(call => { | ||
| const options = call.options; | ||
| return options && options.passive === true; | ||
| }); | ||
|
|
||
| expect(touchstartCalls.length).to.be.above(0); | ||
| expect(hasPassiveTrue).to.be.true; | ||
|
|
||
| addEventListenerSpy.restore(); | ||
| }); | ||
|
|
||
| it("should add touch events with passive: true for treemap chart", () => { | ||
| // Spy on addEventListener before chart creation | ||
| const originalAddEventListener = Element.prototype.addEventListener; | ||
| const touchstartCalls = []; | ||
|
|
||
| addEventListenerSpy = sinon.stub(Element.prototype, "addEventListener").callsFake(function(type, listener, options) { | ||
| if (type === "touchstart") { | ||
| touchstartCalls.push({ type, options }); | ||
| } | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| }); | ||
|
|
||
| // Create treemap chart | ||
| args = { | ||
| data: { | ||
| columns: [ | ||
| ["data1", 30, 200, 100], | ||
| ["data2", 130, 100, 140] | ||
| ], | ||
| type: "treemap" | ||
| }, | ||
| interaction: { | ||
| inputType: { | ||
| touch: true | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| chart = util.generate(args); | ||
|
|
||
| // Check if touchstart events were added with passive: true | ||
| const hasPassiveTrue = touchstartCalls.some(call => { | ||
| const options = call.options; | ||
| return options && options.passive === true; | ||
| }); | ||
|
|
||
| expect(touchstartCalls.length).to.be.above(0); | ||
| expect(hasPassiveTrue).to.be.true; | ||
|
|
||
| addEventListenerSpy.restore(); | ||
| }); | ||
|
|
||
| it("should add touch events with passive: true for funnel chart", () => { | ||
| // Spy on addEventListener before chart creation | ||
| const originalAddEventListener = Element.prototype.addEventListener; | ||
| const touchstartCalls = []; | ||
|
|
||
| addEventListenerSpy = sinon.stub(Element.prototype, "addEventListener").callsFake(function(type, listener, options) { | ||
| if (type === "touchstart") { | ||
| touchstartCalls.push({ type, options }); | ||
| } | ||
| return originalAddEventListener.call(this, type, listener, options); | ||
| }); | ||
|
|
||
| // Create funnel chart | ||
| args = { | ||
| data: { | ||
| columns: [ | ||
| ["data1", 30, 200, 100], | ||
| ["data2", 130, 100, 140] | ||
| ], | ||
| type: "funnel" | ||
| }, | ||
| interaction: { | ||
| inputType: { | ||
| touch: true | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| chart = util.generate(args); | ||
|
|
||
| // Check if touchstart events were added with passive: true | ||
| const hasPassiveTrue = touchstartCalls.some(call => { | ||
| const options = call.options; | ||
| return options && options.passive === true; | ||
| }); | ||
|
|
||
| expect(touchstartCalls.length).to.be.above(0); | ||
| expect(hasPassiveTrue).to.be.true; | ||
|
|
||
| addEventListenerSpy.restore(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The same issues identified for "passive event listener option" apply here:
- Conflict with global
beforeEachcausing double chart generation - Missing
afterEachhook for spy cleanup
Consider the same restructuring approach with beforeAll to skip the global beforeEach and an afterEach hook for proper cleanup.
- Updated touch event bindings to use passive options for improved performance. Ref naver#4055
netil
force-pushed
the
passiveEvent#4055
branch
from
ddfd7fd to
ddfb08e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
#4055
Details
Updated touch event bindings to use passive options for improved performance.