Fix text becoming invisible in HTML editor#755
Conversation
This is done to fix the issue of text becoming invisible when content height is too long
|
hey @daniloercoli 👋When I first switch to html mode on WPAndroid app, the scroll position is at the bottom of the TextInput, I couldn't figure out why this might be happening, any ideas? |
|
@daniloercoli There's another thing I couldn't figure out on Android side. There's an extra top padding on the html content, and I couldn't detect where it comes from. When I inspect it, it shouldn't be there according to values, could I be missing sth?
|
etoledom
left a comment
etoledom
left a comment
There was a problem hiding this comment.
Thank you for tackling this @pinarol !
It works well on both iOS and Android.
It's a pity that we lost the interactive keyboard dismissal but this fix is 💯 more important.
The pan-to-dismiss I think feels good enough, and the distance to drag for the dismiss also feels correct.
In general this HTML view needs a lot of more ❤️ I guess 😁
I added a couple of code comments, otherwise I'm happy to ✅
Great job! 🎉
|
|
||
| this.panResponder = PanResponder.create( { | ||
| onStartShouldSetPanResponder: ( ) => true, | ||
| onStartShouldSetPanResponderCapture: ( ) => true, |
There was a problem hiding this comment.
I think that just the onStartShouldSetPanResponderCapture is necessary for the pan responder to capture the event (and trigger onPanResponderMove).
src/components/html-text-input.js
Outdated
| keyboardDismissMode="interactive" > | ||
| <View | ||
| { ...( this.isIOS ? { ...this.panResponder.panHandlers } : {} ) } | ||
| style={ { flex: 1 } } > |
There was a problem hiding this comment.
Do you think it's a good idea to remove this extra wrapper view and add the panHandlers directly to KeyboardAvoidingView?
It seems to work well but I wonder if there's a benefit on keeping this wrapper.
There was a problem hiding this comment.
there's no benefit currently so I removed
| ref={ ( textInput ) => this.textInput = textInput } | ||
| textAlignVertical="top" | ||
| multiline | ||
| style={ { ...styles.htmlView, height: this.state.contentHeight + 16 } } |
There was a problem hiding this comment.
contentHeight is not being used anymore. It would be good to remove it from the state.
There was a problem hiding this comment.
And being at it, it looks like the textInput ref is also not being used anymore. This might be a good opportunity to clean it up :)
There was a problem hiding this comment.
All true!
As far as I see textInput was even not used before this PR :)
I believe this has been the case from the beginning of times 🤔 I remember giving this a try long time ago without good results. |
|
@etoledom Ready for another look |
You mean that the caret is at the end of the text @pinarol ? If that's the case, it is the default behavior on Android, the caret is positioned at the end of the text on focus. We've made changes in the wrapper to move the caret at the beginning of a block onfocus/split. |
I see the caret only if I tap to somewhere, and yes when I tap, the caret appears at the end. If that's the default behavior on Android I wouldn't be worried though. |
etoledom
left a comment
etoledom
left a comment
There was a problem hiding this comment.
Thank you for the updates! 🎉
|
Will merge this since the padding issue in this comment doesn't look related with this PR. I removed the top padding of html content and the padding is still there on Android so I am thinking there can be an inset value on native Android level? @daniloercoli if you think the other way feel free to send your feedbacks and I can open a new PR. |
3 participants
To Fix: #744
Root cause analysis and the solution proposal can be found in the issue. ^^^
To fix the anomalies we are removing the outer ScrollView and letting the TextView scroll in its own bounds, thus, letting iOS system make the required optimizations.
After the fix:
iOS:
Android:
Implementation Details
Since we can not use keyboardDismissMode prop anymore I've added a pan gesture to detect when the user drags down and close the keyboard with it. This is active for iOS only, keyboardDismissMode='interactive' was also an iOS only behavior.
To Test
yarn iosRepeat tests for following platforms:
WPiOS
WPAndroid(Except for draggin down to close the keyboard)
Try using also a non iPhone X series device like iPhone 8.
cc @daniloercoli