Bug 28722 – REGRESSION ( 4.0.3- TOT ) Gmail: Undoing a indent removes text instead of its formatting

• Component: HTML Editing
• Status: RESOLVED FIXED

[Chris Petersen]

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.

[Chris Petersen]

<rdar://problem/7169206>

[Chris Petersen]

Using webkit nightly builds, I was able to isolate to a range where the regression occurred :

r46126 (work ) - r46193 (fails )

[Chris Petersen]

I tracked the regression to http://trac.webkit.org/changeset/46143

[Eric Seidel (no email)]

You must mean http://trac.webkit.org/changeset/46142.

[Chris Petersen]

Yes, http://trac.webkit.org/changeset/46142

[Ryosuke Niwa]

Outdent and Command+z work.  I don't quite understand why Command+z works and Edit->Undo doesn't.

[Eric Seidel (no email)]

I suspect that docs intercepts command+z, but they can't intercept "undo" from the menu.

[Enrica Casucci]

Created attachment 40957 [details]
Patch

This patch fixes the bug and provides a test case for bug 23995.

[WebKit Commit Bot]

Comment on attachment 40957 [details]
Patch

Clearing flags on attachment: 40957

Committed r49405: <http://trac.webkit.org/changeset/49405>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

(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?

[Enrica Casucci]

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.

[Adele Peterson]

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.

[Ryosuke Niwa]

Agreed.