Bug 14062 - REGRESSION (r21212): "Remove Format" transforms spaces to U+00A0
Summary: REGRESSION (r21212): "Remove Format" transforms spaces to U+00A0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh Intel OS X 10.4
: P1 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: GoogleBug, InRadar, Regression
Depends on: 26598
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-10 04:48 PDT by Alfonso Martínez de Lizarrondo
Modified: 2009-07-20 16:19 PDT (History)
6 users (show)

See Also:


Attachments
Simplified testcase (676 bytes, text/html)
2007-06-10 04:51 PDT, Alfonso Martínez de Lizarrondo
no flags Details
Undo the changeset 21212 since rebalanceWhitespaceAt has been fixed, the workaround introduced in 21212 is no longer needed (169.91 KB, patch)
2009-06-19 19:27 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Undo the changeset 21212 (deleted)
2009-06-19 19:46 PDT, Ryosuke Niwa
justin.garcia: review+
Details | Formatted Diff | Diff
fixes the bug, removed the ChangeLog change in the global directory (168.56 KB, patch)
2009-06-23 15:52 PDT, Ryosuke Niwa
justin.garcia: review+
Details | Formatted Diff | Diff
fix the expected result for editing/execCommand/5142012-3.html (1.10 KB, patch)
2009-06-23 18:27 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alfonso Martínez de Lizarrondo 2007-06-10 04:48:46 PDT
Calling RemoveFormat on Safari transforms the spaces (U+0020) to NON-BREAK SPACE (U+00A0).

it's a regression in r21214. r21200 works fine
Comment 1 Alfonso Martínez de Lizarrondo 2007-06-10 04:51:19 PDT
Created attachment 14925 [details]
Simplified testcase

This testcase shows the escaped contents of the document before and after calling RemoveFormat.
Comment 2 Frederico Caldeira Knabben 2007-09-04 15:57:53 PDT
This one doesn't block FCKeditor anymore, as we have implemented a custom
formatting system. Please remove the block to 9915, as it is not anymore valid (I'm not able to do it).
Comment 3 Alexey Proskuryakov 2007-09-04 21:42:12 PDT
<http://trac.webkit.org/projects/webkit/changeset/21212> is the only suspicious revision in this timeframe.

The attached test case also shows that innerHTML now ends with two %0A's, while there aren't any in the source.
Comment 4 David Kilzer (:ddkilzer) 2007-12-18 09:59:07 PST
<rdar://problem/5653315>
Comment 5 Ryosuke Niwa 2009-06-19 19:27:08 PDT
Created attachment 31584 [details]
Undo the changeset 21212 since rebalanceWhitespaceAt has been fixed, the workaround introduced in 21212 is no longer needed

This patch changes the behavior of WebCore::InsertTextCommand::input.  It used to replace all spaces by &nbsp; but after this patch is applied, WebCore::InsertTextCommand::input no longer replaces the spaces.  Since this would affect the way inserted text wraps, it may affect other third party applications or Web applications that assumes the existence of the bug.

This patch also replaces the several expected results.  This is due to the fact that those expected results had "unexpected" non-breaking spaces.  Since this patch fixes the problem, we must also fix those expected results.
Comment 6 Ryosuke Niwa 2009-06-19 19:46:36 PDT
Created attachment 31587 [details]
Undo the changeset 21212

The previous patch contained an irrelevant change.
Comment 7 Eric Seidel (no email) 2009-06-21 23:35:45 PDT
Comment on attachment 31587 [details]
Undo the changeset 21212

PrettyPatch seems to fail with this attachment.  I filed bug 26598.
Comment 8 Eric Seidel (no email) 2009-06-21 23:44:57 PDT
This is really something for Justin to review.
Comment 9 Ryosuke Niwa 2009-06-22 11:57:30 PDT
I forgot to mention that this patch lacks the fix for qt platform expected results.  I appreciate if someone could supply that later.
Comment 10 Justin Garcia 2009-06-22 13:41:19 PDT
Comment on attachment 31587 [details]
Undo the changeset 21212

Nice work.  I like how your test case will run in FF too.  r=me

It looks like r21212 was a super aggressive workaround for https://bugs.webkit.org/show_bug.cgi?id=9441 which now appears to be fixed.  I'll close it out.
Comment 11 Julie Parent 2009-06-23 15:41:19 PDT
I just went to commit this change, but I think something is wrong with the patch.  Specifically, it lists a file in the top level ChangeLog that is also in the WebCore/Changelog.  The entry in the top level ChangeLog shouldn't be there right?
Comment 12 Ryosuke Niwa 2009-06-23 15:52:07 PDT
Created attachment 31749 [details]
fixes the bug, removed the ChangeLog change in the global directory
Comment 13 Justin Garcia 2009-06-23 15:56:51 PDT
Comment on attachment 31749 [details]
fixes the bug, removed the ChangeLog change in the global directory

r=me
Comment 14 Justin Garcia 2009-06-23 15:57:56 PDT
Comment on attachment 31587 [details]
Undo the changeset 21212

Ryosuke has a new patch.
Comment 15 Julie Parent 2009-06-23 16:59:10 PDT
Commited as http://trac.webkit.org/changeset/45016
Comment 16 Ryosuke Niwa 2009-06-23 18:27:26 PDT
Created attachment 31763 [details]
fix the expected result for  editing/execCommand/5142012-3.html
Comment 17 Ryosuke Niwa 2009-06-23 18:28:17 PDT
(In reply to comment #16)
> Created an attachment (id=31763) [review]
> fix the expected result for  editing/execCommand/5142012-3.html
> 

Just note that since all my checkouts are broken, this patch hasn't been tested yet.
Comment 18 Julie Parent 2009-06-23 18:32:46 PDT
Comment on attachment 31763 [details]
fix the expected result for  editing/execCommand/5142012-3.html

Marking this patch as obsolete - it was hand generated and not complete.  Ojan checked in the proper fix.
Comment 19 Ojan Vafai 2009-06-23 18:34:45 PDT
Followup commit for layout test result: http://trac.webkit.org/changeset/45027