fix #39 - don't use a mutation-insensitve bookmark when performing mutations - Githubissues

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 #39 - don't use a mutation-insensitve bookmark when performing mutations #40

Closed caffodian closed 8 years ago

caffodian commented 8 years ago

It has some...problems with the current approach. This is a quick fix, that won't take too much thought, until I wrangle all the other bookmarking issues in #38

This also fixes the problem where multiple selection bookmarks are created in the shortcut case.

caffodian commented 8 years ago

Although this normally shouldn't matter, there seems to be a strange interaction between:

"ignore a word if it has a bookmark in it + skip to the next word" + non-normalized text nodes
whatever craziness happens when the span gets removed, resulting in non-normalized text nodes

caffodian commented 8 years ago

Also, more importantly, marking doesn't actually work if you try to use a mutation insensitive bookmark. Which I knew, but I missed the short circuit case.

caffodian commented 8 years ago

..we can't refactor it because it makes the check vs. render decision on a block to block basis, as well. will get better when we refactor things out.

caffodian commented 8 years ago

summary of my rambling, so that I remember why I can't test this easily:

This is actually an interaction of a few things:

there are no new words, because scanning works correctly with bookmarks2
bookmarks 2 is incorrectly being used for marking, if there are no new words
selection goes crazy because the rest of the block gets marked with an selection insensitive bookmark

caffodian commented 8 years ago

how this could be tested better: pass the output of getHtmlWithSelection into spellcheck cases, hopefully it doesn't get broken by testcase html normalization.

caffodian commented 8 years ago

Argh. Because getData() cleans the html of spellcheck spans, it is not possible to assert where the spans are + ensure selection is stable in the same call.

caffodian commented 8 years ago

I am glad I persisted in trying to get it tested, because there is actually a related bug related to having two bookmarks beside each other. :/

caffodian commented 8 years ago

Looks good! Hooray for fewer selection manipulations.

technically this is more (vs. the previous version, but not the version before that.) This does fix the nasty gross bugs around double-bookmarking that i accidentally caused in the previous version, though, while keeping most of the performance benefits

caffodian commented 8 years ago

Ugh. There is a weird undo bug around adding more to a typo, waiting for spellcheck, and then undoing it

winhamwr commented 8 years ago

technically this is more

Hrm. Maybe I'm misunderstanding, then. I thought you're now storing a bookmark to the current selection more often, but no longer manipulating the selection (e.g. modifying the current selection to match the selection that was stored in the bookmark).

caffodian commented 8 years ago

Hrm. Maybe I'm misunderstanding, then. I thought you're now storing a bookmark to the current selection more often, but no longer manipulating the selection (e.g. modifying the current selection to match the selection that was stored in the bookmark).

Oh, yea. From that perspective, there is less selection manipulation. I sometimes mix up "bookmark derived from selection" & selection itself

caffodian commented 8 years ago

I feel like there are still some things we are doing wrong, but they can be more easily improved when we also stop doing bookmarks when not needed. ( #38 )