Scroll cursor back into view when it goes out of bounds #34

jesiegel1 · 2016-03-16T03:58:49Z

Fix #3

Add JS method to calculate the caret position within the RichEditorView
Set scrollView contentSize.height based on html content height
Change scrollView's contentOffset if caret is outside of RichEditorView's bounds

cjwirth · 2016-03-16T04:15:59Z

Hey! Excellent job so far!

Just a few glitches I noticed when giving it a run down --

Entering the first character on a new line will scroll it again
After having scrolled, adding a new line inside a portion that is in view still scrolls -- it would be nice if it scrolled only when it needed to

I've illustrated both issues in this gif:

Definitely better than it was before though!

Thanks 👍

jesiegel1 · 2016-03-16T15:00:51Z

Should now be fixed. I modified the CSS file slightly and it seems to have done the trick.

cjwirth · 2016-03-17T13:00:28Z

It's definitely getting there. It's even better than before!
One bug I did notice is that if you start somewhere "in the middle" and make newlines until it should scroll, instead of just scrolling to where the cursor is visible, it moves the cursor back to (near to) its original position:

jesiegel1 · 2016-03-17T17:39:02Z

When recreating the bug, it seems that the cursor position ISN'T actually changing, it's just the offset of the view. If you scroll the view all the way down, you'll see the cursor is exactly where it should be. I just need to fix the change in scroll offset when doing the above.

jesiegel1 · 2016-03-17T19:43:35Z

Aright, I think it's fixed...let me know if you find more bugs. Let the testing begin!

cjwirth · 2016-03-19T09:07:28Z

RichEditorView/Assets/editor/rich_editor.js

+    var sel=window.getSelection();
+    if (sel.rangeCount) {
+        var range = sel.getRangeAt(0);
+        var needsWorkAround = (range.startContainer.nodeName.toLowerCase() == 'div' && range.startOffset == 0);


Could you add a comment or something about what this workaround is for and what it's doing?
Like, why is it needed? What is it returning instead of the normal case?

cjwirth · 2016-03-19T09:29:27Z

Oh I found another bug! Isn't this fun? lol 😸

If the cursor is out of view (below the cutoff) and you input something, it scrolls up when you input something -- that is good. However, if the cursor is out of bounds (above the cutoff) and you input something, it doesn't scroll into view. Also making newlines in that case is kind of weird too.

jesiegel1 · 2016-03-21T00:45:42Z

good catch! right now the code just checks the caret position against the lower bounds, so my guess is I just need to check the caret position against the upper bounds as well and it'll be fixed.

mfrawley · 2016-03-21T19:20:49Z

Really looking forward to seeing this get merged!

mfrawley · 2016-03-23T10:06:24Z

RichEditorView/Classes/RichEditorView.swift

+        var scrollView = editor.webView.scrollView
+
+        var contentHeight: CGFloat?
+        let htmlHeight = editor.runJS("document.getElementById('editor').clientHeight;")


Generally a good practice to wrap the JS implementation details with a public accessor method on the RE namespace IMHO

I agree. Even in this case, since document.getElementById('editor').clientHeight is literally the same as RE.editor, I might not be too opposed to just RE.editor.clientHeight (I'm not sure how I'm doing it in other places.)

did it that way to match: https://github.com/jesiegel1/RichEditorView/blob/master/RichEditorView/Classes/RichEditorView.swift#L176

mfrawley · 2016-03-28T13:20:45Z

@cjwirth I'd say it's best to keep things working consistently. Updating the example project isn't going to break anyone's code and it probably is good to mention such things in release notes as breaking changes.

cjwirth · 2016-04-04T01:55:40Z

@mfrawley That's a good point. We should probably go with that then!

@jesiegel1 Not trying to rush your or anything, but any progress with the issue of when the cursor is above the visible area?

KKKiller · 2016-04-12T06:47:49Z

how could i use this branch.

jesiegel1 · 2016-04-12T11:13:56Z

@cjwirth only just saw this notification. I'll try to work on it today if I can, or by the end of the week at least.

jesiegel1 · 2016-04-17T19:51:32Z

@cjwirth Review and lemme know if you find any issues - this was a tricky one!

cjwirth · 2016-04-18T03:25:13Z

Awesome! Yeah looking at the code it looks like something that might have taken a while to figure out. I'l take a look later today, probably tonight or something!

But yeah, if it's all working, that's pretty exciting.

cjwirth · 2016-04-18T03:27:26Z

@KKKiller Are you using CocoaPods? If so, you need to change the podfile to reference the in-development branch.

Bascially, you will change

pod 'RichEditorView'

to

pod 'RichEditorView', :git => 'git@github.com:jesiegel1/RichEditorView.git'

and then re-run pod install to get the in-dev version.

You can read more about podfile configuration here

cjwirth · 2016-04-21T12:53:46Z

Really close! I was really close to merging it. But! Scrolling the cursor out of bounds below and then typing should bring the cursor into view. It's really close, but I think its maybe like 1 line off. Making a new line works, as you can see.

btw: It works if you go out of bounds upwards.

cjwirth · 2016-04-22T03:07:31Z

Cool! It seems to be working now!

Let me take a look at the code and see how it looks :)

cjwirth · 2016-04-22T03:10:05Z

RichEditorView/Assets/editor/rich_editor.js

+RE.wrapTextNodes = function() {
+    var contents = RE.editor.childNodes;
+    for (var i = 0; i < contents.length; i++) {
+        if (contents[i].nodeType === 3) {


It might be nice to have an object or enum or something so we don't have magic numbers flying around. Maybe nodeType == 3 is obvious to JS devs, but i have no idea.

Maybe Node.TEXT_NODE instead? https://developer.mozilla.org/en/docs/Web/API/Node/nodeType

yea that works, totally reasonable

cjwirth · 2016-04-27T08:10:20Z

Do you think you can fix some of the suggestions I had? Do you want me to just go and clean it up myself? It seems to be working pretty well, so just a little bit of cleanup and I'll merge it.

jesiegel1 · 2016-04-27T17:36:36Z

@cjwirth yessir. hopefully should be able to do it later today/tomorrow.

KKKiller · 2016-05-06T10:48:21Z

I have used this branch in my project and it is absolutely much better that the master. but today i got a bug,and i have no idea about how to fix it.

when i set a HTMlL string to richEditor, it shown perfect .but when i click at any position of the text , and then i edit the text, it will scroll to the bottom . and after that ,it will work normal.

this bug is not fatal ,but it feel bad.can you help me or give me some advice! thanks much!

jesiegel1 · 2016-05-06T11:59:30Z

Have you used this branch with the most recent commits?

KKKiller · 2016-05-08T06:07:40Z

yes i have update the latest commit.
when i begin edit in any positoin, the corsor will come to the end, and the htmlString will creat a div at the end .
what should i do

KKKiller · 2016-05-08T15:19:12Z

when i edit a HTMLstring, the latest cimmits add a mothed named as "wrapTextNodes", it will creat lots of div in my html, what is the function of this method?

jesiegel1 · 2016-05-08T23:51:07Z

@KKKiller Without text nodes being wrapped in divs, it causes issues for calculating where the caret is. Why is creating lots of divs an issue? Generally you want your html content enclosed in some type of container. What is happening visually as a result of the divs being added?

cjwirth · 2016-05-09T01:28:24Z

Hey, I've been away from Github for a while -- how is this going? Is the extra added div a problem? I understand that it's needed to calculate cursor position, but is it causing problems elsewhere?

KKKiller · 2016-05-09T02:59:42Z

the divs will add lots of blank rows visually. i have remove the method "wrapTextNodes" in my project, it seems work normal. will there be any other bug if i remove it?

jesiegel1 · 2016-05-09T03:01:08Z

did you actually try to see if it will add lots of blank rows? generally in html/css, if an explicit width/height isn't set for the div, the div takes up no space.

KKKiller · 2016-05-09T03:03:33Z

beyond div and /div ,there is much of blank, i think it is the blank takes up the space.

cjwirth · 2016-05-10T05:10:12Z

@KKKiller Maybe I'm not understanding what you're saying, but I'm not able to reproduce it in the sample app. Do you have some example code, or a screenshot or something?

cjwirth · 2016-05-13T06:32:42Z

If not, I'll just merge this and we can open a new issue or something?

KKKiller · 2016-05-13T08:30:53Z

OK,look forward to your merge.

cjwirth · 2016-05-28T10:54:47Z

@jesiegel1 I'm not sure if you're interested or not, but I refactored it a little bit to make it simpler and more type safe. Thought you might like to take a look 😃

jesiegel1 · 2016-05-31T20:11:19Z

@cjwirth always interested. will definitely take a look.

cjwirth mentioned this pull request Mar 16, 2016

View doesn't scroll if you keep typing out of bounds #3

Closed

cjwirth reviewed Mar 19, 2016
View reviewed changes

jesiegel1 added 4 commits March 21, 2016 01:27

Add JS method for finding caret position to help scrolling

154e5e1

fix scrolling bug when deleting all characters on a line

55396c1

Modify css to fix scrolling bug

5dbbf81

Fix scrollView offset and JS caret methods

d3bf5b1

jesiegel1 force-pushed the master branch from f02b009 to d3bf5b1 Compare March 21, 2016 05:28

mfrawley reviewed Mar 23, 2016
View reviewed changes

cjwirth mentioned this pull request Apr 7, 2016

Actually This is not any issue i am just asking my requirment.... #37

Closed

jesiegel1 added 2 commits April 17, 2016 15:50

add method to wrap all text nodes in divs, add new-line boolean

ae7c2fe

update scrollView contentOffset when caret above upper scrollView bounds

50597b9

fixes bug when new line parent node is not 'div'

c6c5be5

cjwirth mentioned this pull request Apr 18, 2016

When i run the sample,it got errors from the keyboardManager ,i am not good at swift ,how should i tackle with it ? #45

Closed

cjwirth reviewed Apr 22, 2016
View reviewed changes

cjwirth changed the title ~~Add JS method for finding caret position to help scrolling~~ Scroll cursor back into view when it goes out of bounds Apr 27, 2016

jesiegel1 added 2 commits May 4, 2016 10:02

fixes

7c59a46

indentation fix

d39d4c3

cjwirth merged commit b3be0fc into cjwirth:master May 13, 2016

Scroll cursor back into view when it goes out of bounds #34

Scroll cursor back into view when it goes out of bounds #34

Conversation

jesiegel1 commented Mar 16, 2016

Fix #3

cjwirth commented Mar 16, 2016

jesiegel1 commented Mar 16, 2016

cjwirth commented Mar 17, 2016

jesiegel1 commented Mar 17, 2016

jesiegel1 commented Mar 17, 2016

cjwirth Mar 19, 2016

Choose a reason for hiding this comment

cjwirth commented Mar 19, 2016

jesiegel1 commented Mar 21, 2016

mfrawley commented Mar 21, 2016

mfrawley Mar 23, 2016

Choose a reason for hiding this comment

cjwirth Mar 25, 2016

Choose a reason for hiding this comment

jesiegel1 Apr 18, 2016 • edited Loading

Choose a reason for hiding this comment

mfrawley commented Mar 28, 2016

cjwirth commented Apr 4, 2016

KKKiller commented Apr 12, 2016

jesiegel1 commented Apr 12, 2016

jesiegel1 commented Apr 17, 2016 • edited Loading

cjwirth commented Apr 18, 2016

cjwirth commented Apr 18, 2016

cjwirth commented Apr 21, 2016

cjwirth commented Apr 22, 2016

cjwirth Apr 22, 2016

Choose a reason for hiding this comment

jesiegel1 Apr 22, 2016

Choose a reason for hiding this comment

cjwirth commented Apr 27, 2016

jesiegel1 commented Apr 27, 2016

KKKiller commented May 6, 2016 • edited Loading

jesiegel1 commented May 6, 2016

KKKiller commented May 8, 2016

KKKiller commented May 8, 2016

jesiegel1 commented May 8, 2016

cjwirth commented May 9, 2016

KKKiller commented May 9, 2016

jesiegel1 commented May 9, 2016

KKKiller commented May 9, 2016 • edited Loading

cjwirth commented May 10, 2016

cjwirth commented May 13, 2016

KKKiller commented May 13, 2016

cjwirth commented May 28, 2016

jesiegel1 commented May 31, 2016

jesiegel1 Apr 18, 2016 •

edited

Loading

jesiegel1 commented Apr 17, 2016 •

edited

Loading

KKKiller commented May 6, 2016 •

edited

Loading

KKKiller commented May 9, 2016 •

edited

Loading