Increase / Decrease Font Size with the Mouse Wheel

[TomMalbran]

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.

[peterflynn]

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.

[TomMalbran]

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.

[lkcampbell]

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.

[jbalsas]

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.

[TomMalbran]

@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.

[TomMalbran]

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.

[peterflynn]

@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?

[TomMalbran]

@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.

[adrocknaphobia]

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.

[TomMalbran]

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.

[peterflynn]

@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.

[TomMalbran]

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.

[njx]

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.