r63230 MediaWiki - Code Review archive

-					} else if ( range.startContainer.nodeName == '#text' &&
-							range.startOffset == range.startContainer.nodeValue.length ) {
-						// Apparently this happens when splitting text nodes
+					}
+					if ( ( range.endOffset == 0 && range.endContainer.nodeValue == null ) ||
+							( range.endContainer.nodeName == '#text' &&
+									range.endOffset == range.endContainer.nodeValue.length ) ||
+							( range.endContainer.nodeName == 'P' && range.endContainer.nodeValue == null ) ) {

This doesn't just add a case, it also changes one (from startContainer to endContainer; why was that)?

I changed it because it didn't seem to make sense the other way. We're looking to see if the selection is at the end of a line, so it seems you'd want to use endContainer to do your case checking. Is there a reason it was using startContainer before? I was testing against some particular cases, but this change didn't seem to negatively impact anything for me.

When checking whether the "selection" is at the start/end of the line, we're really checking whether the start of the selection is at the start/end of the line, so we should be using startContainer throughout. For zero-length selections (i.e. cursor positions without selections), startContainer and endContainer will be equal anyway.

My thinking was that if we do our checks for atStart using startOffset and startContainer, and use endOffset and endContainer for checking for atEnd we will be able to catch both collapsed and non-collapsed selections. As you said, for collapsed selections, the end and start variables will be the same, so this change shouldn't affect those cases at all, but gives us new support for non-collapsed selections.

This change was inspired by one specific case that was not working how I wanted it to. The case, starting with a blank article was:

1. Add a bulleted list item with the toolbar. 2. Add a numbered list item with the toolbar.

Before this change, you'd get "*
# Bulleted list item
" and the cursor would be on a line below the inserted text. After this change we get a much nicer "*
# Bulleted list item" with the cursor right at the end of the text, where it should be. Because encapsulateSelection correctly recognizes the selection being at the end of the line when you click the numbered list item button, the extra line break is not inserted after the text.

A better formatted version of what I just posted.

My thinking was that if we do our checks for atStart using startOffset and startContainer, and use endOffset and endContainer for checking for atEnd we will be able to catch both collapsed and non-collapsed selections. As you said, for collapsed selections, the end and start variables will be the same, so this change shouldn't affect those cases at all, but gives us new support for non-collapsed selections. This change was inspired by one specific case that was not working how I wanted it to. The case, starting with a blank article was:

1. Add a bulleted list item with the toolbar.
2. Add a numbered list item with the toolbar.

Before this change, you'd get:

"*<br /># Bulleted list item<br />"

and the cursor would be on a line below the inserted text. After this change we get a much nicer:

"*<br /># Bulleted list item"

with the cursor right at the end of the text, where it should be. Because encapsulateSelection correctly recognizes the selection being at the end of the line when you click the numbered list item button, the extra line break is not inserted after the text.