Skip to content
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

fixes https://github.com/Microsoft/monaco-editor/issues/1353 #72832

Closed
wants to merge 1 commit into from

Conversation

bcolloran
Copy link

This PR fixes microsoft/monaco-editor#1353 based on the suggestion from @cgillis-aras here: microsoft/monaco-editor#1353 (comment)

Unfortunately, the follow-up comment from @yishn did not work out -- just changing from 'mousewheel' to 'wheel' did not work on Firefox.

This PR follows the pattern used in several other vscode files of using browser.isFirefox rather than registering multiple listeners (from the original suggestion).

Tested and working in vscode and Monaco. Feedback welcome and appreciated! :-)

@msftclas
Copy link

msftclas commented Apr 25, 2019

CLA assistant check
All CLA requirements met.

@bcolloran
Copy link
Author

bump on this @rebornix and @alexandrudima -- anything i can do to aid in review here? Many thanks!

@bcolloran
Copy link
Author

Thought I'd check in on this again. This PR is very small and hopefully not controversial (I tried to follow other examples from the code base as closely as I could).

It's my hope that it will be a very quick (3-ish minute) review. Please let me know if there is anything I can do to make it easy for you all!

Thanks @jrieken, @rebornix and @alexandrudima! :-)

@rebornix
Copy link
Member

rebornix commented May 9, 2019

@bcolloran thanks for your help and sorry for missing your ping. The code works as expected. The only problem is DOMMouseScroll will be deprecated in the future as well so we don't want to replace one deprecated api with another.

I think the right fix should be adopting wheel event incrementally, like #70515 . But again, we should be careful, as the semantics of wheel event is different from mouseWheel.

IMO we should

  1. Adopt wheel for Firefox to unblock Monaco users
  2. Adopt wheel for every browser which supported it and use event. wheelDelta
  3. Introduce an option, say useSystemWheelSensitivity, and use event.delta when it's turned on.

@bcolloran
Copy link
Author

thank you for getting back to me @rebornix :-) Sounds like you have a plan. Is there anything I can do to support #70515? It looks like a thoroughly thought-out solution. I'd love to see it get over the line! 👍

@rebornix
Copy link
Member

@bcolloran it would be awesome that you can help with this issue, having more contribution is always incredible to me. I can do a mitigation for FF and for adopting wheel event, we can probably do that together

Firstly, we need to know what's the expected result of mouseWheel event on different platforms, and different browsers. Take them as input and see what's the output of current VS Code/Monaco. Write tests for them.

Secondly, we change mouseWheel to wheel, and make sure all test cases pass. This way we can ganguratee that users won't be affected by the change.

@Davilink
Copy link

These event aren't standard according to :

it's recommended to use : https://developer.mozilla.org/en-US/docs/Web/API/Element/wheel_event

@rebornix
Copy link
Member

rebornix commented Jul 25, 2019

It's already fixed in master. We can look into how to adopt deltaY/deltaX in the future, for now we still use wheelDeltaY/wheelDeltaX when possible. Thanks for your contribution anyways!

@rebornix rebornix closed this Jul 25, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrolling does not work in Firefox
5 participants