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

Scroll cursor onscreen if needed on text entry, arrow key use or rotation. #2842

Merged
merged 9 commits into from
Jan 16, 2019

Conversation

montehurd
Copy link
Contributor

@montehurd montehurd commented Jan 15, 2019

https://phabricator.wikimedia.org/T213452

Ensures text insertion point is onscreen when...

  • text is entered when either keyboard covers text insertion point or insertion point is above top of screen
  • arrow buttons used such that insertion points moves off top or bottom of screen
  • device rotates

wefwefwefdds mov

NOTE: there is a simulator-only bug: if you scroll down, then tap the up arrow key on your keyboard repeatedly until the cursor moves off the top of the screen, it will jump back down in jarring way. Is sim-only because it doesn't happen if you're on device and using the ^ button instead of the up arrow keyboard key (which doesn't exists on device).

wmf.commands = {
keyboardHeightChanged: (newHeight) => {
Copy link
Contributor Author

@montehurd montehurd Jan 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't find a way to determine that the keyboard appeared purely from JS land... thought maybe the viewport dimensions would change or something. In native land adjustedContentInset changes as the keyboard's size changes, but didn't see anything in JS land that tracks with that change. If we had a way to directly or indirectly determine changes to keyboard height from JS land we wouldn't need this keyboardHeightChanged message from native land or even the resize listener I added. There's a chance I may have missed something...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could monitor changes to adjustedContentInset with the UIScrollViewDelegate method scrollViewDidChangeAdjustedContentInset instead of the keyboard notification and it'd take into account any overlays that aren't the keyboard (for example if the edit toolbar moves into the view itself to keep it visible even after the keyboard is dismissed). You could also pass the full insets to JS to account for any overlays we'd add to the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

@montehurd montehurd changed the title Scroll cursor onscreen if needed on text entry or arrow key use. Scroll cursor onscreen if needed on text entry, arrow key use or rotation. Jan 15, 2019
@montehurd montehurd added the WIP label Jan 15, 2019
@montehurd
Copy link
Contributor Author

Added WIP tag - found a bug.

@montehurd montehurd removed the WIP label Jan 15, 2019
Copy link
Contributor

@joewalsh joewalsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting change to use scrollViewDidChangeAdjustedContentInset by becoming the the webView.scrollView.delegate instead of listening to the keyboard frame notification

@@ -46,6 +48,18 @@ class SectionEditorViewController: UIViewController {
apply(theme: theme)

WMFAuthenticationManager.sharedInstance.loginWithSavedCredentials { (_) in }

NotificationCenter.default.addObserver(forName: UIWindow.keyboardDidChangeFrameNotification, object: nil, queue: nil, using: { [weak self] notification in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This observation is never removed, so even though self is weak, it continues to fire with self as nil even after the VC is deallocated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thx. We may have same issue in ViewController.swift. I can fix it there when I fix this.

wmf.commands = {
keyboardHeightChanged: (newHeight) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could monitor changes to adjustedContentInset with the UIScrollViewDelegate method scrollViewDidChangeAdjustedContentInset instead of the keyboard notification and it'd take into account any overlays that aren't the keyboard (for example if the edit toolbar moves into the view itself to keep it visible even after the keyboard is dismissed). You could also pass the full insets to JS to account for any overlays we'd add to the top.

@joewalsh joewalsh merged commit 940c1c0 into develop Jan 16, 2019
@joewalsh joewalsh deleted the fix/scroll-typing-into-view/T213452 branch January 16, 2019 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants