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.

Sign up for GitHub

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

Jump to bottom

[CLOSED] Increase / Decrease Font Size with the Mouse Wheel #3054

Open
core-ai-bot opened this issue Aug 29, 2021 · 13 comments
Open

[CLOSED] Increase / Decrease Font Size with the Mouse Wheel #3054

core-ai-bot opened this issue Aug 29, 2021 · 13 comments

Comments

@core-ai-bot
Copy link
Member

Issue by TomMalbran
Tuesday Mar 26, 2013 at 07:25 GMT
Originally opened as adobe/brackets#3250

This should complete the tasks in this Trello Card.

Unfortunately chrome doesn't trigger a mouse wheel event when control is pressed, so I had to use Shift + Mouse Wheel Scroll.

Updated: Since #3300 was merged, this request now only has the font size adjustments using the mouse wheel.

TomMalbran included the following code: https://github.com/adobe/brackets/pull/3250/commits
@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Mar 27, 2013 at 00:01 GMT

A few quick questions:

  • Could you enumerate which things this adds exactly? The only thing on the card that's missing from master, I think, is the mousewheel behavior... but it looks like this pull request also does more work to preserve the scroll pos better. Are those the only two additions?
  • Ctrl+mousewheel is such a common standard that I wonder whether mousewheel zoom is even worth doing if we have to use the nonstandard Shift instead. Do you know whether the lack of events with Ctrl is a CEF limitation, or Chrome, or webkit...?
  • Is Editor.getCharWidth() needed? I only see it called once, initializing an unused var.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Wednesday Mar 27, 2013 at 01:09 GMT

  1. It adds the mousewheel behavior, some tests and fixes to preserve the scroll position, that were broken in some cases with the font size preserve, and I removed the px only case and could add it to the restore font size.
  2. It is a shame that we cant use it, but it is a current limitation in webkit and haven't found any solution to it.
  3. It should be used when calculating the deltaX instead of oldHeight. I was testing between using this and the old solution and forgot to change it back. Using the line height to try to preserve the horizontal scroll doesn't make much sense since the char width is not the same as the char height, and using the char width still isn't the best solution since it doesn't increase on 1 px on every adjustment, but does a better job.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Saturday Mar 30, 2013 at 17:55 GMT

Just a clarification, the scroll position wasn't broken by the font size persistence changes. The positioning never worked correctly, I just preserved the existing behavior intending to fix it later. At any rate, glad it is being addressed.

@core-ai-bot
Copy link
Member Author

Comment by jbalsas
Saturday Mar 30, 2013 at 20:12 GMT

I agree with Peter that shift+wheel may not be worth the effort as it won't be discoverable at all. To add more complexity, in current trackpads cmd+wheel (two finger scroll) doesn't even work. This action is mapped to a pinch gesture instead.

@TomMalbran Do you think we could split this into two separate pull requests? We could review those tests and fixes and deal with whatever we decide to do with the mousewheel later.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Saturday Mar 30, 2013 at 20:45 GMT

@lkcampbell You are right that it never worked, and still doesn't work quite right because of issue #3115. But at least now it doesn't do a scroll after the document is loaded and the font didn't changed, and it should scroll better when the font is increased by more than 1. At least when #3115 is fixed. Maybe I could do a fix that works with out that fixed.

@jblas I know that it is really not common, but since Ctrl was out, I wanted to give it a try anyway. I could move the tests and the scroll fixes to a new request and leave this one at is it until we decide if we want to use Shift, or until we find a way to use Ctrl or decide to do nothing and just close it.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Mar 31, 2013 at 02:31 GMT

The request #3300 now has only the scroll fixes and the unit tests (without the mouse wheel ones). I will leave this request as it is until we decide what to do with the mouse wheel scroll and merge it with #3300 if required.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Friday Apr 12, 2013 at 06:58 GMT

@TomMalbran I think we might as well close this since everything but the mousewheel is captured by #3300, and there's probably not much value in merging the mousewheel stuff given the discoverability issue. Is that ok? Btw -- were you ever able to find a CEF bug or anything for the Ctrl issue?

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Friday Apr 12, 2013 at 08:35 GMT

@peterflynn Ok, I was waiting on what other had to say and if there was someone with more knowledge about CEF, if the event could be enabled. Unfortunately I haven't found anything in the CEF forums (maybe I need to look harder or start a post there?), but I know that this is a webkit bug. The browser captures the event but doesn't send it. Although there are other key combinations that can't be used inside the browser but work in CEF, so maybe this bug could be fixed in CEF? I'll close this if there is no way to fix it.

@core-ai-bot
Copy link
Member Author

Comment by adrocknaphobia
Tuesday Apr 16, 2013 at 21:59 GMT

Sorry. This needs to be Ctrl + Scroll (not Shift). Can we get to the bottom of why this isn't working w/ Ctrl? This does work in Chrome (browser), so maybe there is a CEF bug.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Tuesday Apr 16, 2013 at 22:16 GMT

It's ok, I would like it if it could work with Ctrl, but it doesn't. I made this jsfiddle: http://jsfiddle.net/7nqps/. If you scroll on the results section it will pop an alert with 2, if you press Ctrl, it should pop an alert with 1, but since Chrome catches the mouse wheel event event when Ctrl is pressed and doesn't let it get to the page, it does nothing. This is done so that Ctrl + Scroll zooms in Chrome, but since in CEF this isn't required, it would be nice if there is a way to make CEF push this event.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Thursday Apr 18, 2013 at 02:56 GMT

@TomMalbran if you're interested in pursuing this more, you could search to see if there's an existing bug on CEF and file one if not.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Apr 18, 2013 at 08:54 GMT

I haven't found bug yet, but I did found out that CEF has a zoom implementation, but I am not sure if this could be used to map it to solve this problem, since I don't know much about CEF, and the forums aren't helping much.

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday May 10, 2013 at 01:50 GMT

Sounds like we should close this for now. The scrollwheel issue is captured on the Trello card, so we can pursue it in future when we have time to look at it. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot