Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2k
Add dummy 'wheel' event handler to enable scroll zoom on safari#7474
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
archmoj 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.
Thanks for the update.
Please add a draft log.
💃
| if(typeofplotObj.addEventListener==='function'){ | ||
| plotObj.addEventListener("wheel",()=>{}); | ||
| } |
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.
Why is theif block necessary? CanplotObj be undefined?
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.
@camdecoster There's severaltests which pass an empty object ({}) asplotObj, which was leading to anerror causing the test to fail.
I'm not sure ifplotObj is supposed to be a DOM object or if other types are allowed.
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.
Looks like in the actual code (tests aside),Events.init() is only called inone place, and the argument is the output ofgetGraphDiv(), which cannot benull orundefined but it could theoretically be some other type besides a DOM element.
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 wonder if we should modify the tests rather than adding thisif statement.
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.
Maybe just use the optional chaining operator? I think that would essentially do the same thing as theif block, but is more compact and you wouldn't have to update any tests.plotObj?.addEventListener("wheel", () => {});
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.
Nice, TIL.
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.
@camdecoster I've reverted to theif statement due to the conditional chaining operator not being supported in our build pipeline (see#7477) -- are you OK with me merging as it is currently?
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.
Yes, this code will work fine.
src/lib/events.js Outdated
| * https://github.com/d3/d3/issues/3035 | ||
| * https://github.com/plotly/plotly.js/issues/7452 | ||
| */ | ||
| plotObj.addEventListener?.("wheel",()=>{}); |
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 operator should come afterplotObj in case that value is null/undefined. Where it is now, it could fail ifplotObj isn't defined.
| plotObj.addEventListener?.("wheel",()=>{}); | |
| plotObj?.addEventListener("wheel",()=>{}); |
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.
@camdecoster Yes, but the specific case causing the test failures was thatplotObj was defined butplotObj.addEventListener was undefined.
I guess we could doplotObj?.addEventListener?.("wheel", () => {}); to cover both.
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.
Ah, I misunderstood what you were saying before. In this case,plotObj equals{}. Then it's fine to leave it as you had it. No need to add unnecessary checks. It can always be addressed later if it becomes a problem.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
package.json Outdated
| }, | ||
| "overrides": { | ||
| "falafel": { | ||
| "acorn":"^8.1.1" |
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 don't feel comfortable introducing overriding dependencies like that in this PR, specially if we want to release a patch.
IMHO This should be discussed in a separate PR.
I suggest we use bring back the if statement.
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.
OK. I'll put back theif statement for now and open a separate PR for discussing upgradingacorn.
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.
@archmoj@camdecoster I have opened#7478 regarding theacorn override
archmoj commentedJul 18, 2025
Thank you@emilykl |
c9a6b3b intomasterUh oh!
There was an error while loading.Please reload this page.
Closes#7452