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
ace: add external keyboard support on iOS (except history) #3172
Conversation
/* ----------- iPad mini ----------- */ | ||
|
||
/* Portrait and Landscape */ | ||
@media only screen |
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.
@etamponi I don't know if using css media queries based on device width is a good idea for this. what happens if my desktop browser window is smaller than that? Is there a way to do this with js based user agent detection + conditional stylesheet loading or class name appending?
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 agree. It would be much better to do useragent detection via js and to add a css class to the page.
|
||
var iOS = /iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream; | ||
if (iOS) { | ||
this.textInput = new TextInput_iOS(renderer.getTextAreaContainer(), this); |
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.
@etamponi re comment above, could you get away with appending a class to the editor here? and using the css to target based on that classname rather than viewport size?
This looks awesome! I only found a couple of small issues:
|
Thanks @nightwing :) Ok, so this works by: 1) using a specific placeholder to detect special keys and 2) resetting the selection after every selectionchange event that has been handled. I don't know why this approach didn't work before though... 🤔 It's my first attempt, and it worked :D Anyway, for the sake of explanation, let's make an example: the hidden textarea contains: Needing to attach it to the Thanks for discovering the fast typing bug :) I think the reason is that I reset |
Ok, as I suspected, adding a timeout fixes it. |
@@ -74,7 +75,13 @@ var Editor = function(renderer, session) { | |||
this.renderer = renderer; | |||
|
|||
this.commands = new CommandManager(useragent.isMac ? "mac" : "win", defaultCommands); | |||
this.textInput = new TextInput(renderer.getTextAreaContainer(), this); | |||
|
|||
var iOS = /iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream; |
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.
why does this check for MSStream?
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.
That is because MS tried to break browser detection - see http://stackoverflow.com/a/9039885/101970
this.textInput = new TextInput(renderer.getTextAreaContainer(), this); | ||
|
||
var iOS = /iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream; | ||
if (iOS) { |
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.
would be better to move navigator.userAgent test to https://github.com/ajaxorg/ace/blob/master/lib/ace/lib/useragent.js#L104 and use useragent.isIOS
here.
var text = dom.createElement("textarea"); | ||
text.className = "ace_text-input ace_text-input-ios"; | ||
|
||
if (useragent.isTouchPad) |
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 can be removed
.ace_text-input-ios { | ||
position: absolute !important; | ||
top: -100000px !important; | ||
left: -100000px !important; |
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.
why is this needed? this will break composition handling.
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.
Moving the text box off screen prevents iOS rendering selection handles, which are not affected by opacity settings.
return text.selectionStart === 0 && text.selectionEnd === text.value.length; | ||
}; | ||
// IE8 does not support setSelectionRange | ||
if (!text.setSelectionRange && text.createTextRange) { |
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.
could you remove ie8 code from here?
return text.selectionStart === 0 && text.selectionEnd === text.value.length; | ||
}; | ||
// IE8 does not support setSelectionRange | ||
if (!text.setSelectionRange && text.createTextRange) { |
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.
could you remove ie8 code from here?
break; | ||
} | ||
} | ||
text.style.height = "100px"; |
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.
why do we need to set height? this may be better to do somewhere else
Not sure what best to do with it since this isn't my PR, but I have a cleaned up version of this at |
@tqc your branch looks good, could you confirm in a comment here that the changes you've made are under the terms of the project's BSD license. I can merge after that, thanks! |
@nightwing Confirmed. |
Unfortunately I don't have time to keep working on this, sorry for the inconvenience. It was meant as a simple proof of concept and as someone stated in a different bug report, it might cause problems with other plugins (like |
@tqc perhaps I was dreaming... :D I was pretty sure there was a bug somewhere related to my changes that asked about support for plugins like the vim one. Since I can't find it anymore, I guess it is fine 👍 |
I, for one, would love to see this get finished and merged... :-) |
Hey guys, didn't they fix this in new iOS 11? I didn't test it yet, but maybe someone knows. Arrow keys. |
I've just checked on an iPad running iOS 11. The Kitchen Sink demo https://ace.c9.io/build/kitchen-sink.html does not work with the arrow keys. This merge is still very much needed. Thanks! |
@aaronvegh thanks for checking! I didn't yet buy iPad because of this. Also, some offtopic, did you by any chance use your ipad with keyboard for coding in terminal vim as ide or something? Very interested in experience. If you even have video of this process somewhere public, would be very interested to see. |
Here's a video you can look at: I recorded this using iOS 11's screen recorder. It's Panic's Coda running in the terminal with a keyboard. You'll see nano, and using the arrow keys to navigate history. |
@aaronvegh thank you very much. If you have reddit account, I can gild it 🙇 |
merged as #3310 |
@etamponi what approach did you have in mind for these? Do not leave us in mystery like Fermat did:) |
@tqc I found the mention about vim support, it is on #37 on which I cited this PR. I thought the comment meant that my PR broke vim support on iOS, but apparently that wasn't the case :) @nightwing: you now put me in a hard situation... Should I do like Fermat, so that I have at least something in common with him, or should I answer your question and lose this great chance? :) I'm writing from my phone right now, so I can't elaborate a lot, I'll be more precise tomorrow. To make history work, it should be enough to track when the content of the hidden textbox changes without any other event has been detected (keypress, copy/paste etc). This should be pretty easy to do for Cmd+Z (undo), less so for Cmd+Shift+Z (redo). I'll try to recollect my original thoughs and elaborate more on this tomorrow :) right now I don't remember anything on how to make touch selection work too. |
More details on history support:
This would be my plan, but again, it depends on the possibility of making programmatic changes to a textarea preserving/changing its history. |
guys in my experience the arrow keys on an external keyboard still break typing in sharelatex on ios... |
@ildodo12 sharelatex still uses 1.2.5 (check by typing ace.version in the browser console), ask them to update to 1.2.8 |
This adds some basic external keyboard handling and fixes part of #37 .
What is supported:
What is not supported:
However, this provides a pretty usable experience.
How does it work?
The only event that iOS sends to textareas and other editable elements when any "special" character is pressed is
selectionchange
. If we pick the text in the textarea in the correct way, it is possible to use this event to detect what special character has been pressed.It is possible to modify the code to also support history and touch, but I'll leave it for another PR :)