-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Scroll support for mobile #2555
Conversation
Tests fail on
Tests currently fail on main build of Ace, so the problem isn't caused by my changes. |
Added fixes to test described by: 2a0a696 |
@@ -253,6 +254,26 @@ function DefaultHandlers(mouseHandler) { | |||
return ev.stop(); | |||
} | |||
}; | |||
|
|||
this.onTouchMove = function (ev) { | |||
if (ev.getAccelKey()) |
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.
this checks are probably not relevant for touch events
This looks good as a simple way to get minimal scrolling support. Btw have you seen #2474? |
var factor = 1, | ||
touchObj = e.changedTouches[0]; | ||
|
||
e.wheelX = -(parseInt(touchObj.clientX) - startx) / factor; |
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.
are there browsers where touchObj.clientX
is not a number?
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 saw that pull request, but noticed it was removed, and was for an iOS application. So I have nothing to go off of there.
In regards to touchObj.clientX not being a number, if that's ever the case, then the browser is not following W3 standards (http://www.w3.org/TR/touch-events/#idl-def-Touch).
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.
then the browser is not following W3 standards
then we don't need parseInt
call here, right?
I can merge this once you fill the cla form as described in CONTRIBUTING.md
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.
From the W3 site,
"clientX of type long, readonly
The horizontal coordinate of point relative to the viewport in pixels, excluding any scroll offset"
Since it's a long, we want to parse it as an int to keep scrolling at individual pixel sizes. I guess technically we could skip the parsing of an int, since a long still isn't a decimal. And thanks, I'll get right on that cla.
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.
CLA submitted.
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.
long and int are the same in javascript.
Some browsers can also return fractional values for clientX, but they support fractional values for things like style.top, and work better if scroll values are not rounded.
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.
Good to know. Those changes have been committed.
Thank you. |
Updates ACE to commit: ajaxorg/ace-builds@3fb55e8 This includes basic scrolling support in mobiles ajaxorg/ace#2555 iOS 6 or later supported. Android 4.x or later is supported. Re enabled indent and outdent seems to work now, Ive tested it. Bug: T119086 Bug: T69328 Change-Id: If8ecc499c281c92c53982cee281a88119a773514
There is an outstanding issue with scroll support on touch devices (one the issues described here: fix: #37 , fix: #403, fix: #1726 ). Added event listeners for touch move and touch start events to scroll appropriately.