webodf / WebODF

WebODF - JavaScript Document Engine
http://webodf.org/
788 stars 166 forks source link

empty span left behind #37

Open thz opened 11 years ago

thz commented 11 years ago

Is it considered a bug if an empty text:span is left behind?

reproduce:

  1. load welcome.odt (http://www.webodf.org/demo/ci/webodf-0.4.2-1160-g8c8a8c0/editor/localeditor.html)
  2. move cursor into italic word webmail in first paragraph
  3. hit enter twice + hit backspace twice
  4. observe the empty span at the word-split-point
peitschie commented 11 years ago

I've been pondering this behaviour a bit.

If there is a cursor within the empty span, the span should definitely remain behind. This allows the next letter to be typed to have the expected italics font-style.

But.. there are a lot of situations where direct styling will inadvertently leave behind empty spans. The key challenge with cleaning these up however is how to synchronise this across all clients. Consider the following scenario:

Original DOM:

<text:p>A<text:span>BCD</text:span></text:p>

(Client A) Execute OpRemoveText({position: 1, length: 3) (Client B) Execute OpInsertText({position: 4, text: "E"})

With empty span cleanup:

From Client A's perspective:

<text:p>A<text:span>BCD</text:span></text:p>
(Client A) Execute OpRemoveText({position: 1, length: 3)
<text:p>A</text:p>
(Client B) Execute OT(OpInsertText({position: 4, text: "E"}))
<text:p>AE</text:p>

From Client B's perspective:

<text:p>A<text:span>BCD</text:span></text:p>
(Client B) Execute OpInsertText({position: 4, text: "E"})
<text:p>A<text:span>BCDE</text:span></text:p>
(Client A) Execute OT(OpRemoveText({position: 1, length: 3))
<text:p>A<text:span>E</text:span></text:p>

Without empty span cleanup:

From Client A's perspective:

<text:p>A<text:span>BCD</text:span></text:p>
(Client A) Execute OpRemoveText({position: 1, length: 3)
<text:p>A<text:span></text:span></text:p>
(Client B) Execute OT(OpInsertText({position: 4, text: "E"}))
<text:p>AE<text:span></text:span></text:p>

From Client B's perspective:

<text:p>A<text:span>BCD</text:span></text:p>
(Client B) Execute OpInsertText({position: 4, text: "E"})
<text:p>A<text:span>BCDE</text:span></text:p>
(Client A) Execute OT(OpRemoveText({position: 1, length: 3))
<text:p>A<text:span>E</text:span></text:p>

This scenario will still fall over because TextPositionFilter does not consider an empty span to be a desirable place to put a cursor. So once all the text content is removed and the cursor is moved to the nearest walkable position (just after the "A" I think), the empty span won't ever be navigated back into on Client A. Client B though has kept it open because the span always had content inside it (i.e., the remove wouldn't have automatically cleaned it up).

I am not able to properly figure out how to make Client A and Client B's DOMs converge when the execution order of the operations changes. Perhaps this scenario simply shows my lack of understanding as to how operational transformation works though :(.

A random idea: If OpInsertText carried the expected insertion style, this problem would go away as the container could be always be styled. In this design, the specifics of which span a given text node is in would start to vary between clients, but the formatting should still stay consistent.

OpApplyDirectStyling might actually be re-written using this logic, as in this scenario direct styling is equivalent to OpRemoveText & OpInsertText({style: ...}). There are other issues with this however (e.g., the complexity of inserting a fragment which WebODF can't do yet).

thz commented 11 years ago

My take on this: The problem is that we try to remove the empty span implicitly based on a condition which might appear on one client only (emptiness). To make this work we need to make it less implicit. Removing the empty span needs to happen through an Operation instead of implicit. empty_span_ot

Except for the dotted lines the situation would be quite clear and doable. Then we would need to decide. Either the "+(DEF, 1)" transforming along the "-(span, 1)" would be annotated in a way that it adds the span back, or the "-(span, 1" needs to receive some magic when it transforms along the "+(DEF, 1)" so that it will remove the span after it is spanned around the inserted "DEF". In my opinion both converging outcomes would be reasonable for the respective user.

RFC!

peitschie commented 11 years ago

I like the diagrams. I was wondering how people kept sanity when dealing with OT!

One major thing I realised yesterday... empty spans are not actually valid positions. So, once a span is empty, it cannot be accessed via the current cursor positioning behaviour (as it appears between two valid positions, but is not a valid position itself). Your proposal only works if we fix this issue first.

I recall some discussion a while back about potentially needing a more granular coordinate system than simply re-using the cursor, but the decision at the time was to continue to use cursor positions.

peitschie commented 11 years ago

An alternative view on the problem: Rather than the problem being that empty spans are removed, perhaps the problem is that inserted text implicitly relies on the local style information contained in the span. If OpInsertText was instead expected to supply the applied style details for the character style (e.g., style name or direct props for an auto style?), the presence or absence of the span becomes less of an issue. An empty span should always be cleaned up under this model, as a new span would be created as necessary for the new operation.