support for lists, images and Webkit

[johanneswilm]

This has support for images, lists, H-elements, etc. and makes ICE compatible with Webkit (with the exception of Copy-and-paste handeling). It also makes cleaner deletion in selection mode (it deletes each node individually rather than wrapping everything within the same block in one single delete node).

Tests are provided for image and list deletion.

See also #48 #42

[delambo]

Whoa, that's a lot of changes! Thanks for adding tests - after quickly running your branch one of the tests seemed to fail: "Delete left through paragraphs with images."

It might take me some time to check it all out but I'm going to make it a priority to get this in before tackling the other 0.4.2 stuff that I have tagged.

It looks like a lot of @lleirborras stuff from #47 is merged into this pull as well, is that right?

Big thanks!

[johanneswilm]

Edit: fixed

Yes, I looked through lleirborras's commits and took everything in that was still relevant and had not been replaced.

The only exception is this line, which I wasn't sure what is for: ifad@c5d69f4#L0R1014

As for the test, it's strange. In Webkit it works when I try it out locally and when I add the same text to an editor and do it manually, but fails when I try doing it on the version I uploaded to github. In firefox it always seems to work. I will have another look at that right now.

[lleirborras]

I created a comment on the commits that introduce significant changes, please take a look at them and tell me if there´s still anything you don´t understand. Also I remerged your master and applied some missing changes. Sorry for that.

[johanneswilm]

Edit: fixed

Ok, so it appears that I have mertged everything relevant from lleirborras.

@delambo As for the failing test: It works about 50% of the time. It probably has to do with whether the image is cached or not. In the actual editor it always works. It's kind of difficult to investiagte, because whenever I introduce console.log lines, it has a tendency to work. That makes it incredibly hard to investiagte. The issue is basically with an image at the end of a paragraph with the cursor being at the start of the next paragraph, like this:
<p>text <img></p><p>|Some other text.<p>

Now if you hit backspace, it does either:
1. <p>text <img>|</p><p>Some other text.<p>
or:
2. <p>text |<img></p><p>Some other text.<p>

1 is correct and is what happens in Firefox and real-wrold Webkit and some times during the test. 2 is what sometimes happens during the test.

Do you have any suggestions for better tools for investigating this? I generally rely on console.log statements when working with the editor, as I cannot lose focus in the editor.

[lleirborras]

I use to put a debug point on some part of the code and debug from that
point, it's true that it's tweaky to deal with the cursor loosing the
focus, but It can be done in mac using chrome and some pad movements .

2013/2/6 Johannes Wilm notifications@github.com

Ok, so it appears that I have mertged everything relevant from
lleirborras.

@delambo https://github.com/delambo As for the failing test: It works
about 50% of the time. It probably has to do with whether the image is
cached or not. In the actual editor it always works. It's kind of difficult
to investiagte, because whenever I introduce console.log lines, it has a
tendency to work. That makes it incredibly hard to investiagte. The issue
is basically with an image at the end of a paragraph with the cursor being
at the start of the next paragraph, like this:

text

|Some other text.

Now if you hit backspace, it does either:

1. text |

   Some other text.

or:
2.

text |

Some other text.

1 is correct and is what happens in Firefox and real-wrold Webkit and some
times during the test. 2 is what sometimes happens during the test.

Do you have any suggestions for better tools for investigating this? I
generally rely on console.log statements when working with the editor, as I
cannot lose focus in the editor.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/49#issuecomment-13199927.

Lleïr Borràs i Metje
http://lleirborras.blogspot.com
http://twitter.com/lleirborras
http://github.com/lleirborras

[johanneswilm]

I have fixed the last outstanding test.

Lists can be inserted and tracked without a problem. I am not so sure about what extra functionality is needed to have image insertion working, but at least image deletion is working now.

[lleirborras]

Uau!

2013/2/6 Johannes Wilm notifications@github.com

I have fixed the last outstanding test.

Lists can be isnerted and tracked without a problem. I am not so sure
about what extra functionality is needed to have image insertion working,
but at least image deletion is working now.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/49#issuecomment-13204518.

Lleïr Borràs i Metje
http://lleirborras.blogspot.com
http://twitter.com/lleirborras
http://github.com/lleirborras

[johanneswilm]

The last commit adds some tests on combining lists with images. I honestly don't know how much of a use case it is to place images in lists, but at least in theory someone may do that. At least placing images before and after lists should work.

With this I feel my work on the deletion part is done.

[delambo]

Thanks Johannes and lleirborras. From what I tested, the usability for these additions is great!

Regarding image insertion - I think we can let the browser/editor take care of it, as you have done in the demo, and leave it untracked. One of my goals for ice is to make sure that it doesn't become a text/wysiwyg editor so I don't want ice to create these block elements. I wouldn't be opposed to enhancing insert to handle images - that way we can tag it with insert criteria.

[johanneswilm]

I see your point and I agree that this should stay being a plugin, not a WYSIWYG editor itself. The main addition I had to make in the deletion code is that whenever it was dealing with textNodes (for example checking if the text length of an object is zero), I added a test to see if it has any stub elements. I saw there was an earlier effort to handle stub elements which seems to have been half lost in the soruces. It added a class to the stub element itself. I instead wrapped the stub element (really just images for now) in a delete node instead.

I haven't looked at the hooks that text editors provide, but I imagine that they hand over something like the node of an element that was just inserted. If we could just make insert take such a node and wrap it in an insert, I think that would be enough.

[johanneswilm]

@delambo I've added something to try out adding images in the demo editor. With it Ifound another firefox bug where it would delete images inserted by another user correctly. I fixed it and created two more test cases. Webkit does fine.

[delambo]

Thanks, Johannes - I pulled in your branch and was just working on adding a little bit of support for avoided elements and some minor fixes.

[johanneswilm]

ok, so I should just leave the code for now? No problem. You can take a look at my last editions and at them by hand. Or we can wait until you are doen with the fixing/merging. The insert function seems to work with images the way it was.

[delambo]

I got your latest changes merged in ok - hack away.

[delambo]

I'm trying to add support for stubs/imgs between blocks. What do you think about deleting left in the following scenario (range is "|"):

<div><p>text</p><img><p>|second</p></div>

I don't think it would work to wrap the img in an inline delete tag. The easiest handling would be to delete it with no track changing (the implementation I'm leaning toward). Another option would be to add tracking attributes on the img node. For instance, after the delete was handled:

<div><p>text|</p><img class="del" cid="#" ...><p>second</p></div>

This seems like an ideal solution but ice would probably need some refactoring to handle this.

[johanneswilm]

Yes, I think it's fine to just delete it. Sayign we "support images within paragraphs" should be enough. Visually you can get the same done (placing only an image into a paragraph, like this <p><img></p>), and the current state of contentEditable is so buggy that we are likely to run into about another million other problems trying to move the caret into the right position.
Of course, if you have the time to find all those cases and code for them, the second solution would be perfect.

[johanneswilm]

...actually. Thinking about your second model: how would you show that an image has been inserted by user 1 and deleted by user 3?

[delambo]

Yeah, you're right - I was thinking out loud and obviously that idea would need a lot more time to flesh-out.

For now, I'm going with the simple implementation. I think it is a common use case to stick images between two paragraphs which is why I'm trying to support it.

[johanneswilm]

What about just (optionally) forcing a paragraph around it? Meaning the user can have it in-between two paragraphs by tiself, but once our deletion tracking comes along it, it is inserted inside a p. Taking your example, something like this:
before backspace:
<div><p>text</p><img><p>|second</p></div>

after backspace:
<div><p>text</p><p>|<delete><img></delete></p><p>second</p></div>

At any rate. just removing it seems more than fine for now to me.

[johanneswilm]

everything merged in.

@delambo The avoid elements are to be treated as if they are not there, right? Could we also have a way of definign elements that can only be deleted as a while with all children? For example, a specific site may auto-generate some content in their editor (picture with caption) and will only want the user to be able to remove the whole thing or keep it. So for example the user shouldn't be able to just delete the picture and keep the caption, or the other way round. If we create such an option, then I would be ok with just deleting things that are not contenteditable. unless they are part of one of these container elements.

[delambo]

On master - big thanks @johanneswilm and @lleirborras.

I think some of the open issues at the top are duplicates or may have been fixed in this ticket, so I'm going to start cleaning that queue out now. I'm going to commit this into our cms and see what our QA finds. Hopefully, we can get a couple of other issues done by that time (~3 weeks in our release schedule) and then release 0.4.2.