Pasting word in Firefox closes paragraph prematurely

[tommiv]

See it in action: open kitchen sink in FF (I tried 33.0, 34.0 and latest 36.0.1 versions), insert few words, "Three words here." for example. Copy any single word from paragraph and paste it somewhere in the same paragraph.

Initial state

editor:

Three words here.

markup:

<p>Three words here.<br></p>

What I've expected after paste:

editor:

Three words words here.

markup:

<p>Three words words here.<br></p>

What I see:

editor:

Three words

words

here.

markup:

<p>Three words </p><p>words</p><p>here.<br></p>

Other browsers: Chrome, both stable and canary: works well; Safari 8: works well, but inserts nbsp furiosly; IE10: I don't see any markup actually, but I don't care too much.

[lucthev]

That’s a known browser inconsistency regarding the insertHTML command — Firefox is too eager to create new paragraphs. To ensure consistent behaviour across all browsers, Scribe would have to roll their own way of inserting HTML, which is tricky business — we’ll see what the folks at the Guardian think of this.

[rrees]

I've been having a ponder and all I can think of is that we could have a browser-specific formatter that strips the outer paragraph if there is no inner markup. I'm not sure there are any obvious rules to apply though.

[tommiv]

Sorry, but I don't really got it. Here is minimal fiddle that works fine in FF. Also, I see only one bullet about this FF behavior in "browser inconsistency" doc, but

Given content of <p>1|</p>, when inserting HTML of <p>2</p>, Chrome will merge

I believe I'm not trying to paste paragraph since I only copied one word.

[regiskuckaertz]

Couldn't this be related to the fact that the normal paste operation is canceled in favour of insertHTML/insertPlainText? In this case, perhaps FF interprets the clipboard content as plain text, which eventually leads to:

Scribe.prototype.insertPlainText = function (plainText) {
  this.insertHTML('<p>' + this._plainTextFormatterFactory.format(plainText) + '</p>');
};

I was thinking about it this morning and wondered why, instead, the clipboard data couldn't just be overwritten on-the-fly and the browser would handle the rest. That is:

scribe.el.addEventListener('paste', function handlePaste(event) {
  if (event.clipboardData) {
    var dataType = event.clipboardData.types.indexOf('text/html') !== -1 ? 'text/html' : 'text/plain';
    var formatter =  dataType === 'text/html' ? '_htmlFormatterFactory' : '_plainTextFormatterFactory';
    var data = event.clipboardData.getData(dataType);
    event.clipboardData.setData(dataType, scribe[formatter].format(data));
  } else {
    ...
  }
});

What do you think?

[rrees]

@regiskuckaertz hmmm, interesting it would certainly be worth looking at

[regiskuckaertz]

BTW, in FF (I have v38) it looks like clipboardData.types is a DOMStringList which has a contains method but no indexOf. DOMStringList is obsolete and will eventually be replaced with an array of strings, but in the meantime it seems like the two will have to cohabit:

var dataType = (event.clipboardData.types.contains && event.clipboardData.types.contains('text/html')) ||
  (event.clipboardData.types.indexOf && event.clipboardData.types.indexOf('text/html') !== -1) ?
  'text/html' :
  'text/plain';

This is related to https://github.com/guardian/scribe/issues/401

[regiskuckaertz]

@rrees Doing some tests right now, will keep you posted

[regiskuckaertz]

I was wrong, the copy-pasted text passes through enforce-p-element which wraps it in a paragraph. When insertHTML is invoked, it seems Chrome and FF differ in their behaviour: Chrome will silently insert the text content of the paragraph while FF will break the surrounding paragraph and insert the new one.

The culprit is: https://github.com/guardian/scribe/blob/master/src/plugins/core/formatters/html/enforce-p-elements.js#L70

When it's commented out, FF behaves correctly. Is that line really necessary?

[rrees]

Paging @theefer, do you know the reasoning or history here?

[theefer]

cc @OliverJAsh.

I can't remember. From looking at the code, it ensures that if the html contains text that isn't a block-level element (e.g. text node or a span), then it gets wrapped into a P. Does that behaviour still work if the line is removed?

[regiskuckaertz]

@theefer Nope. What do you think of putting that line behind a guard, e.g. testing whether the insertion point is scribe.el? The copy-pasting of words seems like a normal use case.

[theefer]

So when would the wrapping be happening once guarded?

I must admit this is quite a fiddly part of the code and the kind of wrapping/tidying up may be better done as a post-paste cleanup nowadays. Maybe we should try changing to remove the wrapping and do a lot of manual QA of copy/pasting from various sources (Word, GDocs, email, other site), see what happens, and write new tests for any cases we want to cover in the future.

[shaunnetherby]

This has been a problem for me. I have a solution that seems to work, but I would be curious if anyone has a better idea. I made an insertHTML patch that unwraps the outer p tag if in Firefox. It then calls the default insertHTML patch with that value.

return function (scribe) {
        var insertHTMLCommandPatch = new scribe.api.CommandPatch('insertHTML');

        var defaultInsertHTMLCommand = scribe.commandPatches.insertHTML;

        insertHTMLCommandPatch.execute = function (value) {
            scribe.transactionManager.run(function () {
                var firefox = navigator.userAgent.toLowerCase().indexOf('firefox') > -1;
                if (firefox && value.match(/^<p>.*<\/p>$/)) {
                    value = value.replace(/^<p>|<\/p>$/g, '');
                }
                defaultInsertHTMLCommand.execute(value);
            }.bind(this));
        };

        scribe.commandPatches.insertHTML = insertHTMLCommandPatch;
    };
};

[tommiv]

Thanks, it works. I will use it as temporary solution.