Bug 28722 - REGRESSION ( 4.0.3- TOT ) Gmail: Undoing a indent removes text instead of its formatting
Summary: REGRESSION ( 4.0.3- TOT ) Gmail: Undoing a indent removes text instead of its...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Critical
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-08-25 15:38 PDT by Chris Petersen
Modified: 2009-10-13 10:27 PDT (History)
8 users (show)

See Also:


Attachments
Patch (39.15 KB, patch)
2009-10-09 12:07 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Petersen 2009-08-25 15:38:10 PDT
Undoing a indent removes text instead of it's formatting. Reproduces in both Gmail and Google Docs with Webkit Nightly r47686

* STEPS TO REPRODUCE
1. Launch Webkit Nightly r47686 and sign into your gmail account.
2. Compose a new message and type hello in the message body
3. Click the indent more text icon to indent this text
4. After indenting, select Edit - Undo Indent. Notice word is removed rather removing the indent. Also focus is removed from message body as well.
Comment 1 Chris Petersen 2009-08-26 12:05:07 PDT
<rdar://problem/7169206>
Comment 2 Chris Petersen 2009-08-26 12:56:05 PDT
Using webkit nightly builds, I was able to isolate to a range where the regression occurred :

r46126 (work ) - r46193 (fails )
Comment 3 Chris Petersen 2009-08-26 13:25:38 PDT
I tracked the regression to http://trac.webkit.org/changeset/46143
Comment 4 Eric Seidel (no email) 2009-08-26 14:56:12 PDT
You must mean http://trac.webkit.org/changeset/46142.
Comment 5 Chris Petersen 2009-08-26 15:05:43 PDT
Yes, http://trac.webkit.org/changeset/46142
Comment 6 Ryosuke Niwa 2009-08-26 20:17:36 PDT
Outdent and Command+z work.  I don't quite understand why Command+z works and Edit->Undo doesn't.
Comment 7 Eric Seidel (no email) 2009-08-26 23:33:11 PDT
I suspect that docs intercepts command+z, but they can't intercept "undo" from the menu.
Comment 8 Enrica Casucci 2009-10-09 12:07:51 PDT
Created attachment 40957 [details]
Patch

This patch fixes the bug and provides a test case for bug 23995.
Comment 9 WebKit Commit Bot 2009-10-09 15:01:16 PDT
Comment on attachment 40957 [details]
Patch

Clearing flags on attachment: 40957

Committed r49405: <http://trac.webkit.org/changeset/49405>
Comment 10 WebKit Commit Bot 2009-10-09 15:01:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Ryosuke Niwa 2009-10-09 15:27:43 PDT
(In reply to comment #8)
> Created an attachment (id=40957) [details]
> Patch
> 
> This patch fixes the bug and provides a test case for bug 23995.

This patch reverted my changes. And I think that's reasonable because I didn't present any reasonable solution to it on time, but now all my test cases are useless (they are not testing what it is supposed to be testing). Could you file a bug for it or explain why those changes are acceptable?
Comment 12 Enrica Casucci 2009-10-12 09:52:05 PDT
I had a long chat on IRC with Ryosuke and I explained the reasons for the changes.
I've also pointed out that the solution he implemented was not correct because it was breaking the undo mechanism.
The undo mechanism relies on commands being built on a stack to perform the undo. The previous implementation was only performing DOM operations, without placing commands on the stack.
I also pointed out that removing moveParagraph was not the solution to the wrong selection problem, because that issue was caused by a missing call to updateLayout that made TextIterator fail to retrieve the correct positions for the selection.
As far as the other changes that were part of the previous patch, even though I do see the point of not stripping out divs that have an id, I don't think that it was the primary goal and can be addressed in another patch. Furthermore, that solutions creates other selection problems.

In my opinion this bug should be marked as fixed.
Comment 13 Adele Peterson 2009-10-12 10:24:09 PDT
I agree that this bug should be resolved.  This change fixed the regression described here, while retaining good indentation behavior.  Let's open new bugs about any other related issues we discover.
Comment 14 Ryosuke Niwa 2009-10-13 10:27:53 PDT
Agreed.