Patch for proper re-setting of frame selection's typing style.

Patch for proper re-setting of frame selection's typing style.

When moving from one text node to the other while deleting characters,
the deleted text node's style was being applied to any new characters
added into the existing text node.

Typing Style in the frame selection maintains the editing style for the
deleted text node, so that if new characters are inserted at the position
of the deleted text node, it's style can be applied to them.

However, when moving into another text node, we should reset or clear the
typing style maintained by the frame selection. This ensures that any new
content inserted within the existing node does not take on any uncleared
style of the deleted node.

WebKit patch: http://trac.webkit.org/changeset/155425

BUG=286219
R=eseidel@chromium.org,leviw@chromium.org,yosin@chromium.org,ojan@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157663

[yosin_UTC9, 2013-09-06 08:10:16]

https://codereview.chromium.org/23950006/diff/1/LayoutTests/editing/deleting/...
File LayoutTests/editing/deleting/maintain-style-after-delete.html (right):

https://codereview.chromium.org/23950006/diff/1/LayoutTests/editing/deleting/...
LayoutTests/editing/deleting/maintain-style-after-delete.html:6: <div
id="test">H <span style="color: blue;">W</span></div>
Please add test case for multiple text nodes.

testElement.appendChild(document.createTextNode("foo"));
testElement.appendChild(document.createTextNode("bar"));

Please caret between "foo" and "bar". This isn't true for
m_upstreamStart.deprecatedNode() == m_downstreamEnd.deprecatedNode() &&
m_upstreamStart.deprecatedNode()->isTextNode().

https://codereview.chromium.org/23950006/diff/1/Source/core/editing/DeleteSel...
File Source/core/editing/DeleteSelectionCommand.cpp (right):

https://codereview.chromium.org/23950006/diff/1/Source/core/editing/DeleteSel...
Source/core/editing/DeleteSelectionCommand.cpp:287: // early return in
calculateTypingStyleAfterDelete).
We should update this comment.

https://codereview.chromium.org/23950006/diff/1/Source/core/editing/DeleteSel...
Source/core/editing/DeleteSelectionCommand.cpp:289:
document()->frame()->selection()->clearTypingStyle();
nit: It seems you are working on old source code. Frame::selection() now returns
reference instead of pointer of FrameSelection.

https://codereview.chromium.org/23950006/diff/1/Source/core/editing/DeleteSel...
Source/core/editing/DeleteSelectionCommand.cpp:289:
document()->frame()->selection()->clearTypingStyle();
Should we call clearTypingStyle() in calculateTypingStyleAfterDelete()?

void DeleteSelectionCommand::calculateTypingStyleAfterDelete()
{
    if (!m_typingStyle) {
        document()->frame()->selection().clearTypingStyle();
        return;
    }
    ...
}

[arpitab_, 2013-09-06 14:01:32]

Thanks for the review yosin. Have uploaded another patch as per the review
comments.

https://codereview.chromium.org/23950006/diff/1/LayoutTests/editing/deleting/...
File LayoutTests/editing/deleting/maintain-style-after-delete.html (right):

https://codereview.chromium.org/23950006/diff/1/LayoutTests/editing/deleting/...
LayoutTests/editing/deleting/maintain-style-after-delete.html:6: <div
id="test">H <span style="color: blue;">W</span></div>
On 2013/09/06 08:10:16, Yoshifumi Inoue wrote:
> Please add test case for multiple text nodes.
> 
> testElement.appendChild(document.createTextNode("foo"));
> testElement.appendChild(document.createTextNode("bar"));
> 
> Please caret between "foo" and "bar". This isn't true for
> m_upstreamStart.deprecatedNode() == m_downstreamEnd.deprecatedNode() &&
> m_upstreamStart.deprecatedNode()->isTextNode().

Have added another test scenario (the second one) in the added layout testcase
which verifies the behavior for the scenario you have mentioned, i.e. to place
the caret between the two text nodes and then type.

https://codereview.chromium.org/23950006/diff/1/Source/core/editing/DeleteSel...
File Source/core/editing/DeleteSelectionCommand.cpp (right):

https://codereview.chromium.org/23950006/diff/1/Source/core/editing/DeleteSel...
Source/core/editing/DeleteSelectionCommand.cpp:289:
document()->frame()->selection()->clearTypingStyle();
On 2013/09/06 08:10:16, Yoshifumi Inoue wrote:
> nit: It seems you are working on old source code. Frame::selection() now
returns
> reference instead of pointer of FrameSelection.
My bad! Have made the appropriate change in the new patch-set.

https://codereview.chromium.org/23950006/diff/1/Source/core/editing/DeleteSel...
Source/core/editing/DeleteSelectionCommand.cpp:289:
document()->frame()->selection()->clearTypingStyle();
On 2013/09/06 08:10:16, Yoshifumi Inoue wrote:
> Should we call clearTypingStyle() in calculateTypingStyleAfterDelete()?
> 
> void DeleteSelectionCommand::calculateTypingStyleAfterDelete()
> {
>     if (!m_typingStyle) {
>         document()->frame()->selection().clearTypingStyle();
>         return;
>     }
>     ...
> }
Yes! Although I don't see much difference between the two by way of
functionality. However, this seems more appropriate and in keeping with what we
do in Editor.cpp as well.
Have moved the clearTypingStyle() call to this place in patch-set 2.

[yosin_UTC9, 2013-09-09 01:40:54]

ACK
Thanks!

[ojan, 2013-09-12 02:01:21]

lgtm

[commit-bot: I haz the power, 2013-09-12 02:01:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.bah@samsung.com/23950006/8001

[commit-bot: I haz the power, 2013-09-12 03:52:19]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-09-12 06:09:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.bah@samsung.com/23950006/8001

[commit-bot: I haz the power, 2013-09-12 06:55:49]

Change committed as 157663