Bug 53475 – Strikethrough disappears when posting a message on odnoklassniki.ru

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alexey Proskuryakov]

I've been shown a bug on odnoklassniki.ru (a popular social network) that I couldn't fully reduce, and then I found surprising reports of the same about other rich text editors. The steps seem to be the same in each case:
- type some text in an input field;
- use whatever editing toolbar to set a strikethrough style on it;
- post/submit/save.

Results: there is no strikethrough in Safari or Chrome in posted text. There is no problem in other browsers. Other styles (like bold or underline) work fine.

There are people who consider this a huge issue - strikethrough is a popular effect, and losing it _after_ posting is gross.

In the case of odnoklassniki.ru, I know that the site uses GWT, which makes it hard to debug. I suspect that there is a whitelist of tags somewhere, and <s> is not one of them (Firefox uses CSS styling to implement execCommand, so it's not affected).

Other reports that I found, and which are not necessarily related at all:

TinyMCE: http://dev.plone.org/plone/ticket/11438
Wordpress: http://wordpress.org/support/topic/strikethrough-doesnt-work-with-safari-5
http://losingit.me.uk/2010/04/05/the-case-of-the-disappearing-strikethrough-tags

[Levi Weintraub]

This is a TinyMCE (the editor used in Wordpress and possibly odnoklassniki.ru?) bug, and not a WebKit one. Old versions of TinyMCE used <s> tags for strikethrough WebKit and <span style="text-decoration: line-through;"> on others, and were formerly stripping <s> tags out themselves. You can validate on old versions of TinyMCE by modifying the html source in Firefox. Manually adding an <s>foo</s> loses the style when you go back to WYSIWYG mode.

Current versions of TinyMCE use the span with style on WebKit, but I manually pasted <s>hello</s> html into their "Try It" editor (http://tinymce.moxiecode.com/tryit/full.php) and the <s> survived viewing the HTML source as well as saving. They've fixed their bug :)

[Alexey Proskuryakov]

The code on odnoklassniki.ru looks like it has been directly generated by GWT, I can't find any indication of TinyMCE use.

[Levi Weintraub]

(In reply to comment #2)
> The code on odnoklassniki.ru looks like it has been directly generated by GWT, I can't find any indication of TinyMCE use.

Well I'm fairly certain about what's going on in TinyMCE. Any chance I could get a test login for odnoklassniki.ru and some instructions on where to go to find the editor?

[Alexey Proskuryakov]

Levi looked tat this, and we're both pretty sure that the issue at odnoklassniki.ru is also a site bug, but there still doesn't seem to be any indication that they use TinyMCE.

I wish we could just stop being different and switch to styling with CSS, but that would probably break more sites than it would fix.

[Levi Weintraub]

http://trac.webkit.org/changeset/40560

It's a conscious decision on our part. I gave this a try with IE9 and it's totally busted unless you put on "Compatibility Mode" in which case it inserts <STRIKE>. Is the legacy behavior really what we want?

[Alexey Proskuryakov]

OK, now I'm wondering if we should just change execCommand to use <strike>, not <s>. In fact, I'm almost sure that we should!

[Levi Weintraub]

I'm not fundamentally opposed to that for parity with IE, but I do worry that this could potentially  introduce other page incompatibilities, though I'll admit it's unlikely.

The fix is trivial, and would be made here:
http://trac.webkit.org/browser/trunk/Source/WebCore/editing/ApplyStyleCommand.cpp#L1899

[Alexey Proskuryakov]

Levi, would you be willing to make a patch for this?

We'll need a regression test that clearly explains why <strike> is used, so that someone wouldn't change it back to a "more modern" alternative without due consideration.

[Levi Weintraub]

Of course I will, it's as easy as it gets :) I'll get it done and make sure it behaves correctly on all the rich text editors I can come up with.

[Ryosuke Niwa]

(In reply to comment #6)
> OK, now I'm wondering if we should just change execCommand to use <strike>, not <s>. In fact, I'm almost sure that we should!

I'm afraid that we're going to break more websites than we fix.  Ojan & Julie, could you comment on this point?

[Ojan Vafai]

(In reply to comment #10)
> (In reply to comment #6)
> > OK, now I'm wondering if we should just change execCommand to use <strike>, not <s>. In fact, I'm almost sure that we should!
> 
> I'm afraid that we're going to break more websites than we fix.  Ojan & Julie, could you comment on this point?

Gecko and IE both use <strike>. Seems like it'd be relatively safe to switch. We should do so.

[Levi Weintraub]

Created attachment 82151 [details]
Patch

[Alexey Proskuryakov]

> We'll need a regression test that clearly explains why <strike> is used, so that someone wouldn't change it back to a "more modern" alternative without due consideration.

Sad that you didn't add one, and we'll have to rely on memory or (unlikely) svn log checking.

[Levi Weintraub]

(In reply to comment #13)
> > We'll need a regression test that clearly explains why <strike> is used, so that someone wouldn't change it back to a "more modern" alternative without due consideration.
> 
> Sad that you didn't add one, and we'll have to rely on memory or (unlikely) svn log checking.

I'd prefer not to disappoint!

There's certainly test coverage for this code path, are you suggesting a test that exists only to stop anyone who would update the test result from regressing this code? I can certainly write that if you think it's necessary! Maybe I have too much faith in our developers to hope that they'll look into the background of this before changing it again :p

[Alexey Proskuryakov]

> are you suggesting a test that exists only to stop anyone who would update the test result from regressing this code?

Yes. As a mental experiment, imagine that someone intentionally changed strike to s five years ago - how would we learn that by looking at the tests whose results you had to change?

[Alexey Proskuryakov]

Specifically, I imagine there could be urge to undo your change due to <strike> being deprecated in HTML5.

[Levi Weintraub]

Created attachment 82364 [details]
Patch

[Levi Weintraub]

I added a test case I hope does what you want. Care to sign off again? :)

[Alexey Proskuryakov]

Comment on attachment 82364 [details]
Patch

OK, thanks! I advise against using separate .js files for tests though - there is no good in that, only confusion and wasted time when trying to match counterparts.

[Levi Weintraub]

(In reply to comment #19)
> (From update of attachment 82364 [details])
> OK, thanks! I advise against using separate .js files for tests though - there is no good in that, only confusion and wasted time when trying to match counterparts.

Thanks for the review and advice!

[Levi Weintraub]

Comment on attachment 82364 [details]
Patch

Clearing flags on attachment: 82364

Committed r78515: <http://trac.webkit.org/changeset/78515>