search

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

Handle LocalStorage unavailability properly #21

Closed caffodian closed 8 years ago

caffodian commented 8 years ago

Some of our customers have LocalStorage disabled as an IT policy, which results in a trace like this:

Error: Access is denied.

   at hasPersonal (https://sh-absmc.policystat.com/site_media/lib/ckeditor-new/plugins/nanospell/plugin.js:839:4)
   at validWordToken (https://sh-absmc.policystat.com/site_media/lib/ckeditor-new/plugins/nanospell/plugin.js:876:4)
   at getUnknownWords (https://sh-absmc.policystat.com/site_media/lib/ckeditor-new/plugins/nanospell/plugin.js:616:6)
   at scanWordsInRange (https://sh-absmc.policystat.com/site_media/lib/ckeditor-new/plugins/nanospell/plugin.js:643:6)
   at scanWords (https://sh-absmc.policystat.com/site_media/lib/ckeditor-new/plugins/nanospell/plugin.js:411:5)
   at h (https://d2zk9fgwitlpui.cloudfront.net/compress/js/624753d82da0.js:1256:2803)
   at Anonymous function (https://d2zk9fgwitlpui.cloudfront.net/compress/js/624753d82da0.js:1256:3675)
   at CKEDITOR.editor.prototype.fire (https://d2zk9fgwitlpui.cloudfront.net/compress/js/624753d82da0.js:1256:4343)
   at checkNow (https://sh-absmc.policystat.com/site_media/lib/ckeditor-new/plugins/nanospell/plugin.js:401:6)
caffodian commented 8 years ago

We currently use store.js. so one initial take on this would be to determine if the store wrapper is present, if so, use that. else use the localstorage API directly, and try to handle errors properly.

caffodian commented 8 years ago

This will be quite difficult to test.

I'm not convinced that disabling DOM Storage is actually the same as disallowing it. Disabling DOM Storage makes localStorage null. But the production stacktrace has something like Access is denied

caffodian commented 8 years ago

By spec,

it actually throws an exception:

https://mathiasbynens.be/notes/localstorage-pattern#comment-9

caffodian commented 8 years ago

So one thing to verify is whether or not store.js uses this pattern.

If so, great, let's just swap.

If not, this plugin needs to keep a state of whether or not localStorage is available, based on the results of that ugly try/catch idea.

caffodian commented 8 years ago

Not currently a huge fan of the existing "roll your own suggestions by just putting everything into a string, separated by nbsps" implementation...

caffodian commented 8 years ago

Possible way to manually test it: go into private browsing mode in Chrome?

caffodian commented 8 years ago

Found #23 while testing this, which makes manual testing quite annoying, since you can't consistently typo a word without the wrapped nbsps.

caffodian commented 8 years ago

Next problem: how to properly mock the different localstorage states. sinon doesn't seem very happy with me attempting to mock localStorage.getItem, but I may be doing something wrong. error message very cryptic.

caffodian commented 8 years ago

Possible way to test this: customers reported it when we were not a trusted site in IE.

caffodian commented 8 years ago

fixed in #24