Bug 53475

Summary: Strikethrough disappears when posting a message on odnoklassniki.ru
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: ap, jparent, justin.garcia, leviw, leviw, ojan, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://odnoklassniki.ru
Attachments:
Description Flags
Patch
none
Patch none

Alexey Proskuryakov
Reported 2011-01-31 22:49:16 PST
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
Attachments
Patch (34.52 KB, patch)
2011-02-11 11:27 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (37.75 KB, patch)
2011-02-14 14:30 PST, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Levi Weintraub
Comment 1 2011-02-09 15:02:28 PST
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
Comment 2 2011-02-09 15:16:59 PST
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
Comment 3 2011-02-09 15:45:35 PST
(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
Comment 4 2011-02-09 16:38:11 PST
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
Comment 5 2011-02-10 12:29:04 PST
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
Comment 6 2011-02-10 12:38:22 PST
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
Comment 7 2011-02-10 16:50:48 PST
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
Comment 8 2011-02-10 19:55:04 PST
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
Comment 9 2011-02-10 20:13:45 PST
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
Comment 10 2011-02-10 20:51:07 PST
(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
Comment 11 2011-02-10 21:35:57 PST
(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
Comment 12 2011-02-11 11:27:33 PST
Created attachment 82151 [details] Patch
Alexey Proskuryakov
Comment 13 2011-02-11 11:49:48 PST
> 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
Comment 14 2011-02-11 12:08:13 PST
(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
Comment 15 2011-02-11 12:29:16 PST
> 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
Comment 16 2011-02-11 12:30:44 PST
Specifically, I imagine there could be urge to undo your change due to <strike> being deprecated in HTML5.
Levi Weintraub
Comment 17 2011-02-14 14:30:33 PST
Created attachment 82364 [details] Patch
Levi Weintraub
Comment 18 2011-02-14 14:31:49 PST
I added a test case I hope does what you want. Care to sign off again? :)
Alexey Proskuryakov
Comment 19 2011-02-14 15:08:38 PST
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
Comment 20 2011-02-14 15:11:10 PST
(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 21 2011-02-14 15:15:30 PST
Comment on attachment 82364 [details] Patch Clearing flags on attachment: 82364 Committed r78515: <http://trac.webkit.org/changeset/78515>
Note You need to log in before you can comment on or make changes to this bug.