Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

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

Merged
merged 10 commits into from
May 13, 2016
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
154e5e1
Add JS method for finding caret position to help scrolling
jesiegel1 Mar 16, 2016
55396c1
fix scrolling bug when deleting all characters on a line
jesiegel1 Mar 16, 2016
5dbbf81
Modify css to fix scrolling bug
jesiegel1 Mar 16, 2016
d3bf5b1
Fix scrollView offset and JS caret methods
jesiegel1 Mar 17, 2016
ae7c2fe
add method to wrap all text nodes in divs, add new-line boolean
jesiegel1 Apr 17, 2016
50597b9
update scrollView contentOffset when caret above upper scrollView bounds
jesiegel1 Apr 17, 2016
c6c5be5
fixes bug when new line parent node is not 'div'
jesiegel1 Apr 18, 2016
13ebbbf
clean up and bug fixes for setting scrollView offset
jesiegel1 Apr 21, 2016
7c59a46
fixes
jesiegel1 May 4, 2016
d39d4c3
indentation fix
jesiegel1 May 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions RichEditorView.xcodeproj/project.xcworkspace/contents.xcworkspacedata
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions RichEditorView/Assets/editor/rich_editor.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
<link rel="stylesheet" type="text/css" href="normalize.css">
<link rel="stylesheet" type="text/css" href="style.css">
</head>
<body><div id="editor" contentEditable="true" placeholder="" class="placeholder">
</div>
<body>
<div id="editor" contentEditable="true" placeholder="" class="placeholder"></div>
<script type="text/javascript" src="rich_editor.js"></script>
</body>
</html>
65 changes: 65 additions & 0 deletions RichEditorView/Assets/editor/rich_editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ RE.rangeOrCaretSelectionExists = function() {
RE.editor.addEventListener("input", function() {
RE.updatePlaceholder();
RE.backuprange();
RE.wrapTextNodes();
RE.callback("input");
});

Expand Down Expand Up @@ -372,3 +373,67 @@ RE.getSelectedHref = function() {

return href ? href : null;
};

/* Make sure all text nodes are wrapped in divs! */

RE.wrapTextNodes = function() {
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason all the nodes have to be wrapped in divs? I think I told you that it was a specification for the project that I made this on, I don't really remember. I'm not saying you need to take it out, I'm just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

without text nodes being wrapped in divs, it causes issues for calculating where the caret is. I remember you telling me they were supposed to be divs, no?

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea that works, totally reasonable

var newNode = document.createElement('div');
RE.createWrapper(contents[i], newNode);
RE.focus();
}
}
}


RE.createWrapper = function(elms, node) {
var child = node.cloneNode(true);
var el = elms;

var parent = el.parentNode;
var sibling = el.nextSibling;

child.appendChild(el);

if (sibling) {
parent.insertBefore(child, sibling);
} else {
parent.appendChild(child);
}

};

/* retrieve caret vertical position */

RE.getCaretPosition = function() {
var x=0, y=0;
var newLine = false;
var result = [];
var sel=window.getSelection();
if (sel.rangeCount) {
var range = sel.getRangeAt(0);
var needsWorkAround = (range.startOffset == 0)
/* Removing fixes bug when node name other than 'div' */
// && range.startContainer.nodeName.toLowerCase() == 'div');
if (needsWorkAround) {
console.log(range);
x=range.startContainer.offsetLeft;
y=range.startContainer.offsetTop; // add range.startContainer.clientHeight if want bottom of caret;
newLine = true; // position is on new line with no content
} else {
if (range.getClientRects) {
var rects=range.getClientRects();
if (rects.length > 0) {
x = rects[0].left;
y = rects[0].top;
newLine = false;
}
}
}
}
var json = JSON.stringify({height: y, newLine: newLine});
console.log(json);
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need to keep logs sticking around.

return json;
};
11 changes: 10 additions & 1 deletion RichEditorView/Assets/editor/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,19 @@ html {

body {
min-height: 100%;
overflow: hidden;
overflow: auto;
}

body, h1, p {
margin: 0;
}

div {
display: block;
position: static;
min-height: 28px;
Copy link
Owner

Choose a reason for hiding this comment

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

What is this 28px about? Is it necessary? I actually don't know the implications of much of this CSS that's being added. I hope it doesn't change the way it looks for people who are already using it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made the <div> height the same height as the line-height for #editor

}

#editor {
-webkit-user-select: auto !important;
-webkit-tap-highlight-color: rgba(0, 0, 0, 0);
Expand All @@ -47,6 +53,9 @@ body, h1, p {

#editor {
min-height: 100%;
height: 100%;
overflow: auto;
display: block;
font-family: 'HelveticaNeue';
Copy link
Owner

Choose a reason for hiding this comment

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

it looks like it wasn't your fault, but could you get this indention to be the same?

color:#666666;
line-height:28px;
Expand Down
76 changes: 76 additions & 0 deletions RichEditorView/Classes/RichEditorView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,12 @@ extension RichEditorView {
return runJS("RE.rangeOrCaretSelectionExists();") == "true" ? true : false
}

/** Returns caret vertical position */

public func getCaretPosition() -> String {
return runJS("RE.getCaretPosition();")
}

/**
* If the current selection's parent is an anchor tag, get the href.
* @returns nil if href is empty, otherwise a non-empty String
Expand Down Expand Up @@ -439,6 +445,75 @@ extension RichEditorView: UIGestureRecognizerDelegate {
// MARK: - Utilities
extension RichEditorView {

/** Sets content offset for scrollView based on caret location **/

private func fixScrollView(editor: RichEditorView) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is an instance method, you don't need to pass in the RichEditorView.

Also fixScrollView() is pretty vague. Maybe something like scrollCursorIntoView() ? I don't know, maybe there's something better, but it'd be nice to have something more clear about what it does.


var scrollView = editor.webView.scrollView

var contentHeight: CGFloat?
let htmlHeight = editor.runJS("document.getElementById('editor').clientHeight;")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator Author

@jesiegel1 jesiegel1 Apr 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

if let n = NSNumberFormatter().numberFromString(htmlHeight) {
Copy link
Owner

Choose a reason for hiding this comment

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

In Swift you can just do Float(htmlHeight) without having to get an NSNumberFormatter involved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cjwirth http://stackoverflow.com/questions/27595799/convert-string-to-cgfloat-in-swift

let floatValue = CGFloat(n)
contentHeight = floatValue
} else {
contentHeight = scrollView.frame.height
}
scrollView.contentSize = CGSizeMake(scrollView.frame.width, contentHeight!)

/* Caret above scrollView bounds */

// Usually getCaretPosition() returns the amount the caret is offset from the current contentOffset, so "-40" would
// indicate that the contentOffset would need to be decreased by 40.
// However, when the cursor is on a new line with no html content, getCaretPosition() returns the absolute position
// of the caret. The resulting code differentiates between caret position values greater than 0 that could either mean
// the caret position is beyond the scrollView lower bounds, or above the scrollView upper bounds and on a new line.
// The code also checks if the caret position is within the scrollView bounds to avoid scrolling when caret is
// not on the first line of the scrollView.

let data = convertStringToDictionary(editor.getCaretPosition())

// Checks if caret is on new line with no content
let newLine = data!["newLine"] as! Bool
if let caretPositionNumeric = data!["height"] as? NSNumber {
Copy link
Owner

@cjwirth cjwirth Apr 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make sense to have some (x, y) instead of (height, newLine) ? Either way, I'd rather have editor.getCaretPosition() return something typed, rather than a dictionary. Either something like

func getCaretPosition() -> CGPoint?

or

func getCaretPosition() -> (height: CGFloat, newLine: Bool)?

or something. In this second case, it's not really the "caret position" either...

Also, stay away from implicitly unwrapped optionals and Objective-C classes like NSNumber when you can.

I also like CGPoint(x: x, y: y) better than CGPointMake(x, y), it's more Swifty 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so a few things - height and newLine aren't x and y values. The x value of the caret is irrelevant for vertical scrolling. newLine indicates whether the caret is on a new line or not, and therefore which branch of the conditional to execute. The only way I could return those two values was using a dictionary, because it seemed from the other JS that the return value had to be a String.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and yea, I should just use Int(), I don't really remember why I went that route.


let caretFloat = CGFloat(caretPositionNumeric) - scrollView.contentOffset.y

var CGFloatCaretPositionNumeric = CGFloat(caretPositionNumeric.floatValue)
if newLine {
if (caretFloat + 24.0) > (scrollView.bounds.size.height) {
Copy link
Owner

Choose a reason for hiding this comment

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

What is the significance of 24? Will it change if people change to have some custom CSS instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, so basically the value returned by the JS caret position method will either give you the point at the bottom of the caret or the top of the caret (you could have it return both in a dict). so depending on whether the caret is above or below, one of them depending on how you code it will have to add or subtract the caret height to get the proper x,y value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cjwirth If the user can change the font size, then I need to add a new method that gets the current caret height, and uses that value instead of the hardcoded 24.0.

let bottomOffset = CGPointMake(0, (CGFloat(caretPositionNumeric.floatValue) - (scrollView.bounds.size.height + scrollView.contentOffset.y)) + scrollView.contentOffset.y + 28.0)
scrollView.setContentOffset(bottomOffset, animated: true)
} else if (CGFloatCaretPositionNumeric < scrollView.contentOffset.y) && scrollView.contentOffset.y > 0 {
let bottomOffset = CGPointMake(scrollView.contentOffset.x, CGFloatCaretPositionNumeric)
scrollView.setContentOffset(bottomOffset, animated: true)
}
} else {
if CGFloatCaretPositionNumeric + 24.0 > scrollView.bounds.size.height {
let bottomOffset = CGPointMake(0, ((CGFloatCaretPositionNumeric - scrollView.bounds.size.height) + scrollView.contentOffset.y + 28.0))
scrollView.setContentOffset(bottomOffset, animated: true)
} else if (CGFloatCaretPositionNumeric < 0) {
var amount = scrollView.contentOffset.y + CGFloatCaretPositionNumeric
amount = amount < 0 ? 0 : amount
let bottomOffset = CGPointMake(scrollView.contentOffset.x, amount)
scrollView.setContentOffset(bottomOffset, animated: true)
}
}
}

}

private func convertStringToDictionary(text: String) -> [String:AnyObject]? {
if let data = text.dataUsingEncoding(NSUTF8StringEncoding) {
do {
return try NSJSONSerialization.JSONObjectWithData(data, options: []) as? [String:AnyObject]
} catch let error as NSError {
print(error)
}
}
return nil
}

/**
Runs some JavaScript on the UIWebView and returns the result
If there is no result, returns an empty string
Expand Down Expand Up @@ -513,6 +588,7 @@ extension RichEditorView {
updateHeight()
}
else if method.hasPrefix("input") {
fixScrollView(self)
let content = runJS("RE.getHtml()")
contentHTML = content
updateHeight()
Expand Down
4 changes: 2 additions & 2 deletions RichEditorViewSample/RichEditorViewSample/KeyboardManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ class KeyboardManager: NSObject {
Starts monitoring for keyboard notifications in order to show/hide the toolbar
*/
func beginMonitoring() {
NSNotificationCenter.defaultCenter().addObserver(self, selector: #selector(KeyboardManager.keyboardWillShowOrHide(_:)), name: UIKeyboardWillShowNotification, object: nil)
NSNotificationCenter.defaultCenter().addObserver(self, selector: #selector(KeyboardManager.keyboardWillShowOrHide(_:)), name: UIKeyboardWillHideNotification, object: nil)
NSNotificationCenter.defaultCenter().addObserver(self, selector: Selector("KeyboardManager.keyboardWillShowOrHide:"), name: UIKeyboardWillShowNotification, object: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

The project is already using Swift 2.2 selector syntax, doesn't it make sense to use here too?

Copy link
Owner

Choose a reason for hiding this comment

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

Nice catch. Now that Xcode 7.3 official is out, I think we should migrate everything to being Swift 2.2 only. Although -- this file is just in the sample project. Do you think we should make a major version change if we have changes which break in Xcode 7.2 but work in 7.3? I feel like I want to say yes...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I simply switched it back since 7.3 wasn't out and switching it back every time i updated a branch was getting annoying

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah it would be nice to have these in Xcode 7.3 form

NSNotificationCenter.defaultCenter().addObserver(self, selector: Selector("KeyboardManager.keyboardWillShowOrHide:"), name: UIKeyboardWillHideNotification, object: nil)
}

/**
Expand Down