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.

Sign up for GitHub

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

Jump to bottom

fix FF contenteditable backspace bug. #1502

Merged
merged 5 commits into from
Feb 10, 2014
Merged

fix FF contenteditable backspace bug. #1502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ca88c78
fix FF contenteditable backspace bug.
Dec 16, 2013
f1a1c7f
tweaks per YUI team advice on pull request.
Jan 10, 2014
a3f1dea
tweaks per YUI team advice on pull request.
Jan 10, 2014
f8f8126
tweaks per https://github.com/yui/yui3/pull/1502/files#r8857828
Feb 6, 2014
3dfd41f
tweaks per https://github.com/yui/yui3/pull/1502/files#r8852815
Feb 6, 2014
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
49 changes: 43 additions & 6 deletions src/editor/js/editor-para.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,17 +247,20 @@
}
}
}

if (Y.UA.gecko) {
/*
/*
* This forced FF to redraw the content on backspace.
* On some occasions FF will leave a cursor residue after content has been deleted.
* Dropping in the empty textnode and then removing it causes FF to redraw and
* remove the "ghost cursors"
*/
d = e.changedNode;
t = inst.config.doc.createTextNode(' ');
d.appendChild(t);
d.removeChild(t);
// d = e.changedNode;
// t = inst.config.doc.createTextNode(' ');
// d.appendChild(t);
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this was removed? Are you assuming this problem isn't occurring anymore, or does _fixGeckoOnBackspace inadvertently fix this same problem?

Copy link
Author

Choose a reason for hiding this comment

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

I was unable to repro this ever, so I'm assuming it's not occurring.

Copy link
Contributor

Choose a reason for hiding this comment

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

All right. In that case, let's simply comment it out... just in case.

Copy link
Contributor

Choose a reason for hiding this comment

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

And because I'm paranoid.

// d.removeChild(t);

this._fixGeckoOnBackspace(inst);
}
break;
}
Expand All @@ -271,6 +274,41 @@
}

},

//If we just backspaced into a P on FF, we have to put the cursor
//before the BR that FF (usually) had injected when we used <ENTER> to
//leave the P.
_fixGeckoOnBackspace: function (inst) {
var sel = new inst.EditorSelection(),
node,
childNodes;

//not a cursor, not in a paragraph, or anchored at paragraph start.
if (!sel.isCollapsed || sel.anchorNode.get('nodeName') !== 'P' ||
sel.anchorOffset === 0) {
return;
}

//cursor not on the injected final BR
childNodes = sel.anchorNode.get('childNodes');
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of childNodes, use children. There are instances in IE where childNodes may contain non-element nodes. So,

childNodes = sel.anchorNode.get('children');

Copy link
Author

Choose a reason for hiding this comment

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

The way anchorOffset works is that it offsets across element nodes and text nodes. I want non-element nodes, so I'm using get('childNodes')

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. I did not know that. Thanks.

node = sel.anchorNode.get('lastChild');
if (sel.anchorOffset !== childNodes.size() || node.get('nodeName') !== 'BR') {
return;
}

//empty P (only contains BR)
if (sel.anchorOffset === 1) {
sel.selectNode(sel.anchorNode, true);
return;
}

//We only expect injected BR behavior when last Node is text
node = node.get('previousSibling');
if (node.get('nodeType') === Node.TEXT_NODE) {
sel.selectNode(node, true, node.get('length'));
}
},

initializer: function() {
var host = this.get(HOST);
if (host.editorBR) {
Expand Down Expand Up @@ -303,4 +341,3 @@
Y.namespace('Plugin');

Y.Plugin.EditorPara = EditorPara;

138 changes: 102 additions & 36 deletions src/editor/tests/unit/assets/editor-content-editable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,14 @@ YUI.add('editor-tests', function(Y) {

template = {
name: 'Inline Editor Tests',
setUp : function() {},

tearDown : function() {},
setUp : function() {
this.editorBaseUse = Y.EditorBase.USE.concat('node-event-simulate',
'dd');
},

tearDown : function() {
},

'test_load <inline>': function() {
Y.Assert.isObject(Y.Plugin.ContentEditable, 'ContentEditable was not loaded');
Expand Down Expand Up @@ -347,8 +352,6 @@ YUI.add('editor-tests', function(Y) {
Y.Assert.isFalse(Y.one('#editor').hasAttribute('contenteditable'), 'iframe DOM node was not destroyed');
},
'test_editor <inline>': function() {
Y.EditorBase.USE.push('dd');
Y.EditorBase.USE.push('node-event-simulate');
var iframeReady = false;

editor = new Y.EditorBase({
Expand All @@ -358,7 +361,7 @@ YUI.add('editor-tests', function(Y) {
{
fn: Y.Plugin.ContentEditable,
cfg: {
use: Y.EditorBase.USE
use: this.editorBaseUse
}
}
]
Expand Down Expand Up @@ -613,7 +616,7 @@ YUI.add('editor-tests', function(Y) {
{
fn: Y.Plugin.ContentEditable,
cfg: {
use: Y.EditorBase.USE
use: this.editorBaseUse
}
}
]
Expand All @@ -631,52 +634,114 @@ YUI.add('editor-tests', function(Y) {
Y.Assert.areEqual(Y.one('#editor iframe'), null, 'Second Frame was not destroyed');
Y.Assert.isFalse(Y.one('#editor').hasAttribute('contenteditable'), 'iframe DOM node was not destroyed');
},
'test_para_plugin <inline>': function() {

makeEditorWithParaPlugin : function (initialContent, onReady) {
var _this = this;

editor = new Y.EditorBase({
content: 'Hello <b>World</b>!!',
content: initialContent,
extracss: 'b { color: red; }',
plugins: [
{
fn: Y.Plugin.ContentEditable,
cfg: {
use: Y.EditorBase.USE
use: this.editorBaseUse
}
}
]
});
Y.Assert.isInstanceOf(Y.EditorBase, editor, 'Third EditorBase instance can not be created');

Y.Assert.isInstanceOf(Y.EditorBase, editor, 'EditorBase instance not created?');
editor.plug(Y.Plugin.EditorPara);
Y.Assert.isInstanceOf(Y.Plugin.EditorPara, editor.editorPara, 'EditorPara was not plugged..');
editor.render('#editor');
editor.set('content', '<br><b>Test This</b>');
Y.Assert.isInstanceOf(Y.Plugin.EditorPara, editor.editorPara, 'EditorPara was not plugged.');

var inst = editor.getInstance();
if (onReady) {
editor.after('ready', function () {
_this.resume(onReady);
});
}

var str = '<b>foo</b>';
var out = editor.frame.exec._wrapContent(str);
Y.Assert.areEqual('<p><b>foo</b></p>', out);
//force to always be async onReady call
setTimeout( function () {
editor.render('#editor');
}, 0);
},

out = editor.frame.exec._wrapContent(str, true);
Y.Assert.areEqual('<b>foo</b><br>', out);
'test_para_plugin <inline>': function() {
var inst, str, out;

fireKey(editor, 13);
fireKey(editor, 8);
editor.editorPara._fixFirstPara();
editor.editorPara._afterPaste();
editor.editorPara._onNodeChange({
changedEvent: {},
changedNode: inst.one('b'),
changedType: 'enter-up'
});
editor.editorPara._onNodeChange({
changedEvent: {},
changedNode: inst.one('br'),
changedType: 'enter'
});
editor.destroy();
Y.Assert.areEqual(Y.one('#editor iframe'), null, 'Third Frame was not destroyed');
function onEditorReady() {
editor.set('content', '<br><b>Test This</b>');

inst = editor.getInstance();

str = '<b>foo</b>';
out = editor.frame.exec._wrapContent(str);
Y.Assert.areEqual('<p><b>foo</b></p>', out);

out = editor.frame.exec._wrapContent(str, true);
Y.Assert.areEqual('<b>foo</b><br>', out);

fireKey(editor, 13);
fireKey(editor, 8);
editor.editorPara._fixFirstPara();
editor.editorPara._afterPaste();
editor.editorPara._onNodeChange({
changedEvent: {},
changedNode: inst.one('b'),
changedType: 'enter-up'
});
editor.editorPara._onNodeChange({
changedEvent: {},
changedNode: inst.one('br'),
changedType: 'enter'
});

editor.detachAll();
editor.destroy();
Y.Assert.areEqual(Y.one('#editor iframe'), null, 'Third Frame was not destroyed');
}

this.makeEditorWithParaPlugin('<p> content<br></p>', onEditorReady);
this.wait();
},

'test para plugin gecko fix <inline>' : function () {
function onEditorReady() {
var inst = editor.frame.getInstance(),
container = editor.frame.get('container'),
node = container.getDOMNode(),
sel = new inst.EditorSelection(),
text = 'here is the initial text';

//Simulate gecko state where: P is terminated by BR, then we
//backspace into it and the cursor (collapsed selection) is
//after the BR. See: https://github.com/yui/yui3/issues/1376
editor.set('content', '<p>' + text + '</p>');
container.one('p').appendChild('<br>');
//place cursor 2 nodes after P start (1st is text node, 2nd is BR)
node = container.one('p');
sel.selectNode(node, true, 2);

editor.editorPara._fixGeckoOnBackspace(inst);

//Cursor should now be before the BR
sel = new inst.EditorSelection();
Y.Assert.isTrue(sel.isCollapsed, 'expected cursor, not range for selection');
Y.Assert.areEqual(1, container.all('p').size());
node = container.one('p');
Y.Assert.areEqual('P', sel.anchorNode.get('nodeName'),
'selection anchorNode wrong');
Y.Assert.areEqual('P', sel.focusNode.get('nodeName'),
'selection focusNode wrong');
Y.Assert.areEqual(text.length, sel.anchorOffset,
'selection anchorOffset wrong');
}

this.makeEditorWithParaPlugin('<p> content</p>', onEditorReady);
this.wait();
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should be ignored in everything but Firefox (ignore list is closer to the bottom of this file). So,

_should: {
    ...
    ignore: {
        'test para plugin gecko fix <inline>': !Y.UA.gecko
    },
   ...
}


'test_double_plug_setup <inline>': function() {
editor = new Y.EditorBase({
content: 'Hello <b>World</b>!!',
Expand Down Expand Up @@ -914,7 +979,8 @@ YUI.add('editor-tests', function(Y) {
ignore: {
'test: EditorSelection <inline>': Y.UA.phantomjs,
'test_selection_methods <inline>': Y.UA.phantomjs,
'test_br_plugin <inline>': Y.UA.phantomjs
'test_br_plugin <inline>': Y.UA.phantomjs,
'test para plugin gecko fix <inline>': !Y.UA.gecko
},
error: { //These tests should error
'test_selection_methods <inline>': (Y.UA.ie ? true : false),
Expand Down