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
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 :)
The code on odnoklassniki.ru looks like it has been directly generated by GWT, I can't find any indication of TinyMCE use.
(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?
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.
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?
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 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
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.
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.
(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?
(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.
Created attachment 82151 [details] Patch
> 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.
(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
> 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?
Specifically, I imagine there could be urge to undo your change due to <strike> being deprecated in HTML5.
Created attachment 82364 [details] Patch
I added a test case I hope does what you want. Care to sign off again? :)
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.
(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!
Comment on attachment 82364 [details] Patch Clearing flags on attachment: 82364 Committed r78515: <http://trac.webkit.org/changeset/78515>