Add dummy 'wheel' event handler to enable scroll zoom on safari #7474
Conversation
emilykl
force-pushed
the
safari-scroll-zoom
branch
from
68f04da to
06c3e7f
Compare
emilykl
force-pushed
the
safari-scroll-zoom
branch
from
8370097 to
e6618c6
Compare
| if(typeof plotObj.addEventListener === 'function') { | ||
| plotObj.addEventListener("wheel", () => {}); | ||
| } |
There was a problem hiding this comment.
Why is the if block necessary? Can plotObj be undefined?
There was a problem hiding this comment.
@camdecoster There's several tests which pass an empty object ({}) as plotObj, which was leading to an error causing the test to fail.
I'm not sure if plotObj is supposed to be a DOM object or if other types are allowed.
There was a problem hiding this comment.
Looks like in the actual code (tests aside), Events.init() is only called in one place, and the argument is the output of getGraphDiv(), which cannot be null or undefined but it could theoretically be some other type besides a DOM element.
There was a problem hiding this comment.
I wonder if we should modify the tests rather than adding this if statement.
There was a problem hiding this comment.
Maybe just use the optional chaining operator? I think that would essentially do the same thing as the if block, but is more compact and you wouldn't have to update any tests.
plotObj?.addEventListener("wheel", () => {});
There was a problem hiding this comment.
@camdecoster I've reverted to the if 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.
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.
The operator should come after plotObj in case that value is null/undefined. Where it is now, it could fail if plotObj isn't defined.
| plotObj.addEventListener?.("wheel", () => {}); | |
| plotObj?.addEventListener("wheel", () => {}); |
There was a problem hiding this comment.
@camdecoster Yes, but the specific case causing the test failures was that plotObj was defined but plotObj.addEventListener was undefined.
I guess we could do plotObj?.addEventListener?.("wheel", () => {}); to cover both.
There was a problem hiding this comment.
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.
emilykl
force-pushed
the
safari-scroll-zoom
branch
from
876c90a to
b142a3d
Compare
Co-authored-by: Mojtaba Samimi <[email protected]>
package.json
Outdated
| }, | ||
| "overrides": { | ||
| "falafel": { | ||
| "acorn": "^8.1.1" |
There was a problem hiding this comment.
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.
OK. I'll put back the if statement for now and open a separate PR for discussing upgrading acorn.
There was a problem hiding this comment.
@archmoj @camdecoster I have opened #7478 regarding the acorn override
|
Thank you @emilykl |
Closes #7452