909352 - Etherpad should not have spellcheck="false" on its body

Status: VERIFIED FIXED

(Infrastructure & Operations :: IT-Managed Tools, task)

Description

[(no longer active)]

This breaks spell checking in Firefox as of Firefox 24 (on beta now) and other browsers, such as Chrome.  See bug 908570.

Comment 1

[Dão Gottwald [:dao]]

This isn't specific to etherpad.mozilla.com.

Comment 2

[(no longer active)]

But it can and should be fixed in our instance.  Please file another bug for the evangelism side of things? :-)

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> But it can and should be fixed in our instance.  Please file another bug for
> the evangelism side of things? :-)

I agree, upstream etherpad org is not interested in this version anymore (Etherpad Lite codebase is the official etherpad now), once we get upgraded to Etherpad Lite that will be applicable.

Comment 4

[Robert Helmer [:rhelmer]]

Our fork lives at https://github.com/mozilla/pad

Comment 5

[Robert Helmer [:rhelmer]]

Looks like this is done in:
https://github.com/mozilla/pad/blob/master/infrastructure/ace/www/ace2_outer.js
https://github.com/mozilla/pad/blob/master/infrastructure/ace/www/ace2_wrapper.js

("ace" is the name of etherpad's conteneditable-based editor, no relation to the cloud9 "ACE" editor, confusingly)

Comment 6

[Robert Helmer [:rhelmer]]

It's worth checking if this is still the case in etherpad-lite, and asking upstream why this was done.

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #6)
> It's worth checking if this is still the case in etherpad-lite, and asking
> upstream why this was done.

Answering the first part - it is still done:
https://github.com/ether/etherpad-lite/blob/stackato/src/static/js/ace.js

I'll ask around (I am technically part of the upstream org FWIW :P)

Comment 8

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #7)
> (In reply to Robert Helmer [:rhelmer] from comment #6)
> > It's worth checking if this is still the case in etherpad-lite, and asking
> > upstream why this was done.
> 
> Answering the first part - it is still done:
> https://github.com/ether/etherpad-lite/blob/stackato/src/static/js/ace.js

Oops - that link should be:
https://github.com/ether/etherpad-lite/blob/master/src/static/js/ace.js#L267

Comment 9

[Robert Helmer [:rhelmer]]

Well the fix is easy enough:
https://github.com/mozilla/pad/pull/3

:jakem do you think it's worth taking this?

For etherpad-lite, I think we should make this configurable (possibly in the UI, at least just sitewide). I'll file a ticket upstream to track that.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #9)
> For etherpad-lite, I think we should make this configurable (possibly in the
> UI, at least just sitewide). I'll file a ticket upstream to track that.

https://github.com/ether/etherpad-lite/issues/1863

Comment 11

[Jake Maul [:jakem]]

I have not found any good reason why this is currently hard-coded to spellcheck being off... that predates its initial import into github, so the relevant commit message is meaningless.

I suspect our usage of etherpad is such that spellcheck would generally be a good thing rather than a hindrance, so I'm happy to merge this. I don't know when we can feasibly deploy it, however, since that requires kicking everyone off of etherpad to restart it. Probably some time after hours, at least... and possibly during the weekend even.

Comment 12

[Jake Maul [:jakem]]

Merged... all that's left now is to update and restart the production instance. There's a staging instance, but it's currently nonfunctional and I don't recall why... might just need started or some other simple fix, but might be horribly broken.

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to Jake Maul [:jakem] from comment #11)
> I have not found any good reason why this is currently hard-coded to
> spellcheck being off... that predates its initial import into github, so the
> relevant commit message is meaningless.
> 
> I suspect our usage of etherpad is such that spellcheck would generally be a
> good thing rather than a hindrance, so I'm happy to merge this. I don't know
> when we can feasibly deploy it, however, since that requires kicking
> everyone off of etherpad to restart it. Probably some time after hours, at
> least... and possibly during the weekend even.

I discussed w/ upstream, nobody is sure there either (it's still off in the frontend editor code, which was imported from the old etherpad to etherpad-lite pretty much untouched and is the same except for bugfixes)

John McLear mentions in https://github.com/ether/etherpad-lite/issues/1863#issuecomment-23304565 that there is a plugin to turn this off for etherpad-lite, which is probably good enough.

Comment 14

[:glob ✱]

(In reply to Jake Maul [:jakem] from comment #12)
> Merged... all that's left now is to update and restart the production
> instance.

can you please schedule etherpad to be restarted to pick up this change?

Comment 15

[Jason Crowe [:jd]]

Etherpad was restarted yesterday for other reasons. This should now be complete.

Regards

Comment 16

[:glob ✱]

i'm still seeing spellcheck="false" on etherpad.m.o.

Comment 17

[(no longer active)]

(In reply to comment #16)
> i'm still seeing spellcheck="false" on etherpad.m.o.

Same here.

Comment 19

[Jake Maul [:jakem]]

Looks like this was merged but never actually deployed! I've just deployed it. It's the end of the day and I verified no *large* meetings were using it, so I'm not too worried about having booted people off.

Comment 20

[:glob ✱]

thank you!