Lists are being embedded in paragraphs on chrome 16 #314

Merged
merged 12 commits into from Jan 12, 2012

Projects

None yet

3 participants

@scusack

While using 1.0.0a3 demo: http://wymeditor.no.de/wymeditor/examples/01-basic.html

I hit enter after "Hello World" and then selected the "Unordered List" from the toolbar, the resultant list is wrapped in the Paragraph.

I checked the behaviour in the Firefox 8 and it behaved as expected, ie, the paragraph turned into the list block element.

I was using Chrome version "16.0.912.63 m".

My understanding is that the Firefox behaviour is correct, in other words Wymeditor (at least version 1.0) doesn't let you nest block elements.

@winhamwr
wymeditor member

Thanks so much for the bug report. I'm not seeing this behavior on Chrome 15. Looks like I need to do an apt-get update though because Chrome 16 is actually the version in the stable channel.

I'll give this some more investigation shortly.

@winhamwr
wymeditor member

Yuck. Looks like this is only an issue with Chrome 16. I was hoping their frequent releases wouldn't be a problem.

Thanks again for the bug report. We'll get this fixed.

@jkrcma

I have Chrome 17 beta and the issue still remains.

And I have a suspicion, that this is not only a problem with lists, but common block HTML tags also. In our company we use custom-made button for inserting <div>s with particular attributes (calls exec with INSERT_HTML command). After clicking the button, <div> is inserted into <p> and after cursor move Chrome auto-corrects the DOM and replaces <div> with <p>. Hope that helps and thanks!

@winhamwr
wymeditor member

@DragonJake thanks for the Chrome 17 report. Sounds like it's a big shift with 16 that we'll have to figure out. From what I can tell, you're correct that it effects more than just lists. I've searched through the chromium project tracker and I haven't been able to find anything related to this that might explain it.

I'm hoping to close out another couple of nagging list indent/outdent issues I've been working on for the #302 ticket and then switch and get this figured out.

@winhamwr winhamwr Refs #314. Proof of concept fix for new chrome bug.
Still need to handle restoring the selection
3c9a3c9
@winhamwr
wymeditor member

I have a proof of concept fix in this branch: https://github.com/wymeditor/wymeditor/tree/issue_314

I'm still not quite sure what changed to cause this to break, but I've at least got an angle on a fix. I think I also know what's happening with your div insertion and it's related.

Do you have any code I could look at so I can get a specific unit test on your div insertion case?

winhamwr added some commits Jan 10, 2012
@winhamwr winhamwr Merge remote branch 'origin/master' into issue_314 35e7dd6
@winhamwr winhamwr Refs #314. Added comments for some WYMeditor constants 836f80d
@winhamwr winhamwr Refs #314. Added failing tests for converting blocks to lists
Webkit has problems converting root elements and firefox has problems
converting sublists. Looks like we'll need another pure-DOM solution to
work around the buggy/inconsistent execCommand implementations.
ece1f22
@winhamwr winhamwr Refs #314. More list creation/conversion unit tests c666bfe
@winhamwr winhamwr Refs #314. Fixed remaining test cases for list conversion/creation on…
… FF.

Might need to look in to some kind of performance optimization if the
conversion is too slow for large lists on slow browsers like ie6.
c7f4e75
@winhamwr winhamwr Refs #314. Added changelog entries fcd85a9
@winhamwr
wymeditor member

@DragonJake I think this branch should fix the new chrome issues (as well as fixing a few other minor list bugs). I'm going to let it simmer for a day or so before merging it in for a 1.0.0a6 release.

Let me know if the div => p problem is still around for your custom button. If I can reproduce it, I'd like to fix it in core.

@winhamwr
wymeditor member

Grr. Scratch that. Some more funny business in chrome.

@jkrcma

Hi, thanks for trying, but unfortunately after short test my problem still persists. I tried lists and they seem to be fixed, but divs don't. From observation I think it's harder to force Chrome to reconstruct the DOM (and convert div to p) now.

I'll provide you our particular code example when I'm back at work tomorrow. Thank you!

winhamwr added some commits Jan 11, 2012
@winhamwr winhamwr Refs #314. In your face ie6. Passing tests
Evidently jquery wrapAll doesn't work when passed an already-existing
dom node in ie6, even though that works everywhere else.
08025e7
@winhamwr winhamwr Refs #314. Stabilized selection restoration after list operations.
Added the rangy selectionsaverestore library. It's awesome.
cb2acf6
@winhamwr winhamwr Merge remote branch 'origin/master' into issue_314 6908ef8
@winhamwr winhamwr Refs #314. Added a changelog entry about the selection bugs. 8682d89
@jkrcma

Ok, after I walked through some company source code, I realized that the important part is pretty short. This is what I have:

var element = '<div class="poll" id="poll-4941"><em>Some test poll?</em></div>';
//wym.insertAtCaret(element);
// ^^^ this was some older workaround, but in the end it calls the following
wym._exec(WYMeditor.INSERT_HTML, element);

And what I get (notice what happens when I try to get out of the <div> "jail", a new one is inserted):

<p>
    <div class="poll" id="poll-4941"><em>Some test poll?</em></div>
    <div class="poll" id="poll-4941"><em><br></em></div>
</p>

Screenshot

The only way to continue in writing is to type some letters, wait until the <div> is fixed to <p> by Chrome, and delete it. Unfortunately you get stuck in the second level of <p>s.

@winhamwr
wymeditor member

@DragonJake Thanks for the example. I'll use that for a test case.

I'll probably split the div case off in to a separate issue since this list-related issue seems to be fixed. I think it's going to be fairly straight-forward to fix, and if that's the case, I'll include it in the planned 1.0.0a6 release for today. I'll throw you an @ on the new issue to make sure you can track it.

Thanks for the feedback and example code.
-wes

@winhamwr winhamwr merged commit fd9d475 into master Jan 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment