Calling setContent in Firefox on an edited scribe will create a new

chanon commented 10 years ago

Demonstrated here: http://jsfiddle.net/8g6ezbsb/3/

First, edit the text in the scribe instance, then click "Set Content".

The resulting html in the scribe instance is different from the html sent to setContent. It has an additional  in front.

This happens only in Firefox (at least not in Chrome), only when the scribe instance was edited before calling setContent.

chanon commented 10 years ago

Also occurs when DOM content changes are picked up by the MutationObserver. That is when

a scribe instance is edited by the user
then its DOM contents are directly changed by outside code
in Firefox, a new  will be added on top

chanon commented 10 years ago

This issue makes using setContent (or other means) to programmatically make changes to the contents of scribe instances problematic / unusable.

chanon commented 10 years ago

After some tracing, it looks like what's happening is enforce-p-elements.js is wrapping a stray  that gets created at some point in the formatting process.

hmgibson23 commented 10 years ago

Hi. If you look at the source here, https://github.com/guardian/scribe/blob/aa7376e261fffc0476c9edffb3cee00b0b0245f8/src/scribe.js#L193. You can see we insert another   in Firefox, which will in turn an extra paragraph. This should in turn be removed as https://github.com/guardian/scribe/blob/aa7376e261fffc0476c9edffb3cee00b0b0245f8/src/scribe.js#L201 shows. That might have something to do with it.

chanon commented 10 years ago

Maybe, but at https://github.com/guardian/scribe/blob/aa7376e261fffc0476c9edffb3cee00b0b0245f8/src/scribe.js#L193 its for ! this.allowsBlockElements() but in this case it is with allowBlockElements = true. Also that adds a   at the end, but this is a new paragraph at the beginning.

hmgibson23 commented 10 years ago

Wrote the above without seeing latest comment. In that case, as per your other comment, it might be that the scribe-markers are not being selected properly, in which it will get left behind and possibly wrapped in a P. I'll have a look.

chanon commented 10 years ago

Thanks! In enforce-p-elements.js I tried adding some tracing at line 57:

    consecutiveInlineElementsAndTextNodes.forEach(function (nodes) {
      console.log('enfore p around nodes: ');
      console.log(nodes);
      var pElement = document.createElement('p');

What I got out in the console was:

"enfore p around nodes: "
Array [ <em.scribe-marker> ]

So that is what I think is the cause.

chanon commented 10 years ago

Its kind of funny how s get dropped all over the document. So this might be related to #153. However in this case the marker is in a scribe instance .. just not in the  where it should be I guess.

hmgibson23 commented 10 years ago

The markers are dropped to work on the nodes between the selection. They basically mark the selection. Do you mean they're appearing in the head and places like that? Because that is definitely not right!

chanon commented 10 years ago

Yes, not in <head> but in other random places in the document. Its a separate bug covered by #153 I guess. In my jsfiddle above if you open inspector to inspect elements and click somewhere outside the editor, then click 'Set Contents', you'll see a new  inserted where the cursor was.

Edit: actually just click the Set Contents button many times and you'll get lots of markers in the DOM.

hmgibson23 commented 10 years ago

My guess is that somewhere the Scribe.selection.selectMarkers() https://github.com/guardian/scribe/blob/master/src/api/selection.js is not removing the set marker. You could have a look at that and see if that helps - I'll look at the PR :). Although I've not been able to look at the remaining scribe-marker

lucthev commented 10 years ago

Out of curiosity, are the selection markers used for anything other than saving the selection? If not, the selection could just as easily be encoded as a couple of integer pairs; for example,

<!-- The ‘|’ denote the endpoints of a left-to-right selection. -->
<div class="scribe" contenteditable="true">
  <p>S|ome</p>
  <p>Tex|t</p>
</div>

The selection can be encoded as two pairs, [0, 1] and [1, 3], representing the start and end points of the selection, respectively. This removes the need for markers and their associated workarounds, while also enabling functionality like that suggested in #199. It might also fix the above bug.

hmgibson23 commented 10 years ago

They're particularly useful for working between the selection. This would still be possible with pairs but maybe slightly more confusing. Maybe @theefer has some thoughts about this?

chanon commented 10 years ago

Yes, something other than placing marker elements into the DOM might be a cleaner solution so if possible would be great, but I guess this is a hard problem in any case due to the browser inconsistencies.

I've been able to fix #153 with #248, but it didn't fix this issue as here the marker is in the scribe element but somehow in the wrong place at the wrong time causing it to be wrapped by a .

I tried looking at why this happens but couldn't figure it out. So my hackish workaround is/was to just fix the code in enforce-p-elements.js so that it ignores s when finding elements to wrap in s. I'm not even sure if I did that right, but it seems to have fixed this particular issue.

OliverJAsh commented 10 years ago

@lucthev Re. https://github.com/guardian/scribe/issues/247#issuecomment-53894150, I believe that range.startOffset and selection.anchorOffset have browser inconsistencies, so we can't rely on them.

See https://github.com/guardian/scribe/blob/e6d32c7abb6187f8dbe246effc78206110692e5d/BROWSERINCONSISTENCIES.md#other (the second point).

lucthev commented 10 years ago

@OliverJAsh that might be a problem with using Range#setStartBefore/setEndAfter to set the selection, and not necessarily with Selection.anchorOffset/Range.startOffset; the right nodes and offsets are logged after user events (see this jsbin, for example). Firefox does occasionally report the nodes and offsets as being in the parent when the caret is at the end of the paragraph, but those are not technically incorrect and can still be used to find the offset using an algorithm similar to the one the authors of Quill came up with. I’ve been using a module for saving/restoring selections that uses these ideas, and, in practice, have not yet encountered any problems.

Digging a little deeper into Scribe, it seems the selections are used for more than just saving/restoring the selection, so perhaps this model is not very well suited for Scribe; adopting it would probably entail rewriting large portions of the codebase.

johannish commented 9 years ago

I can duplicate this (using the jsfiddle and my own application) in Chrome (Windows and OS X), Firefox (OS X), and Safari (OS X), but not in IE 11. In my application, setContent is being called when loading data from the server into the scribe-enabled element. If the cursor has been placed once (the user clicked the field) before the data is loaded, this extra paragraph tag appears.

Through my own debugging, I agree with the cause described in https://github.com/guardian/scribe/issues/247#issuecomment-53853502.

johannish commented 9 years ago

@chanon Did you ever find a workaround for this issue?

chanon commented 9 years ago

As noted, my workaround was to

" fix the code in enforce-p-elements.js so that it ignores s when finding elements to wrap in s."

johannish commented 9 years ago

Doh! Didn't read carefully enough. Thanks! That works like a charm:


function wrapChildNodes(scribe, parentNode) {
  var groups = Array.prototype.reduce.call(parentNode.childNodes, function (accumulator, binChildNode) {
    if (binChildNode.nodeName !== "EM") { //HACK to fix https://github.com/guardian/scribe/issues/247
      var group = last(accumulator);
      if (!group) {
        startNewGroup();
      } else {
        var isBlockGroup = scribe.element.isBlockElement(group[0]);
        if (isBlockGroup === scribe.element.isBlockElement(binChildNode)) {
          group.push(binChildNode);
        } else {
          startNewGroup();
        }
      }
    }
    return accumulator;
    ...

guardian / scribe

Calling setContent in Firefox on an edited scribe will create a new <p></p> #247