PolicyStat / ckeditor-spell-check-plugin-js-dev-edge

User interface enhancements and beta features for http://ckeditor-spellcheck.nanospell.com . This repo is for nanospell developers and trusted partners to modify and fork plugin.js
Other
0 stars 0 forks source link

Fix spellcheck repeating nested list nodes. #1

Closed caffodian closed 8 years ago

caffodian commented 8 years ago

Broke this out as a self-PR for review / work separation purposes. Ignore all the mess with dealing with node_modules and ckeditor paths for now.

Nested list nodes get read twice - once as part of their parent list item, once again as single list items. This appears to be due to some special casing inside CKEditor range iterators, although it probably makes sense for some previous case.

https://github.com/ckeditor/ckeditor-dev/blob/12f0de314fd6fbee0bc4d35d541123d283fdecc9/core/dom/iterator.js#L324

                    // LIs are returned as is, with all their children (due to the
                    // nested lists). But, the next node is the node right after
                    // the current range, which could be an <li> child (nested
                    // lists) or the next sibling <li>.

I didn't perfectly break off all the test setup into here. Some of it happened as part of moving my work over to this repo.

This does NOT deal with the table content destruction issue I ran into (still haven't investigated)

caffodian commented 8 years ago

Hmm. Really the "correct" fix for this isn't to do it in walker, but is to make the walker stop when exiting list items, and then have the thing passing stuff into the walker, treat list items as separate blocks.

caffodian commented 8 years ago

The weird thing is, rangeIterator is supposed to return list items. Hm.

caffodian commented 8 years ago

This must be a special case with nested lists, perhaps?

Testing needs to be backed up a bit, now we need to test MarkTypos and how the blocks get passed into our Wordwalker, instead.

caffodian commented 8 years ago

Try it initially with a spy - though maybe markTypos needs to be mocked so as to not actually crash the browser while trying to fix this.

caffodian commented 8 years ago

Ah.

This is not really about nested list items, it's about lists in the form of:

<ol>
<li> something something
<ol><li>nested</li></ol>
</ol>

It's the li which has text followed by another list, which has a problem with walking.

caffodian commented 8 years ago

So this is fun. According to this, list items are returned as is:

https://github.com/ckeditor/ckeditor-dev/blob/12f0de314fd6fbee0bc4d35d541123d283fdecc9/core/dom/iterator.js#L324

                    // LIs are returned as is, with all their children (due to the
                    // nested lists). But, the next node is the node right after
                    // the current range, which could be an <li> child (nested
                    // lists) or the next sibling <li>.

Which actually means we get repeats. For example in the following:

<ul><li>fud<ol><li>foo</li><li>bar</li></ol></li><li>baz</li></ul>

The inner foo bar items are returned twice, first as part of one giant list, then as part of the inside.

["<li>fud<ol><li>foo</li><li>bar</li></ol></li>", "<li>foo</li>", "<li>bar</li>", "<li>baz</li>"]

One possible fix may be to filter out ol and ul, either in the iterator or afterwards.

caffodian commented 8 years ago

Because getNextParagraph returns actual blocks, and not ranges, one of the following functions, which walks, needs to know to avoid inner lists since they will later be covered.

winhamwr commented 8 years ago

Reviewing this with the ?w=1 param helps a ton.

winhamwr commented 8 years ago

But making all of the whitespace changes in a separate PR would have been much more kind. With ?w=1, you lose the ability make inline comments.

caffodian commented 8 years ago

Sorry about that. For this repo I basically copied over most of CKEditor's whitespace settings for consistency, which then conflicts with when it was in pstat. :/

winhamwr commented 8 years ago

I'm not super-confident in my review, here, but this definitely seems like a pareto improvement.

caffodian commented 8 years ago

I'm not super-confident in my review, here, but this definitely seems like a pareto improvement.

Thanks for giving it a shot. Still a few things to make better but I'm hoping this structure makes it easier going forward (and possibly eventually open source contributions, if the other option doesn't work out.)