Bug 34608 – Deleting text leaves the style of text in the typing style

• Component: HTML Editing
• Status: NEW

[Will Stokes]

-go to http://webkit.org/demos/editingToolbar/
-enter the following string "Begin with normal text. Then some bold text. More normal text. Some italic text. And finally some more normal text."
-select "Then some bold text" and click the bold button
-select "Some italic text" and click the italic button
-place the cursor at the end of the editor
-repeatedly tap the backspace key to delete through the normal and italic regions into the "More normal text region"
-start typing text -> italic characters are entered, even though a normal character was the last one deleted
-repeatedly tap the backspace key to delete through the bold region and into the first normal region
-start typing -> bold characters are entered, even though a normal character was the last one deleted

It appears the bold or underline region towards the end of the document is not being deleted when entirely backspacing through a bold/underline region. Instead it's begin tugged back toward the beginning. The cursor is actually within the bold/underline empty block, and as a result when typing new text bold/underline is used as a complete surprise to the user.

[Ryosuke Niwa]

Created attachment 72151 [details]
demo

[Darin Adler]

I can reproduce this bug on that test page, and also in another simple contenteditable test, but I cannot reproduce the incorrect behavior in Mac OS X Mail even though it is running with the same version of WebKit. Not sure why there’s a difference.

[Johan "Spocke" Sörlin]

Created attachment 105614 [details]
Another example where backspace produces odd results

[Ryosuke Niwa]

Also see the bug 7360.

[Pravin D]

Created attachment 146379 [details]
Proposed Patch

[Ryosuke Niwa]

Comment on attachment 146379 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146379&action=review

> Source/WebCore/editing/DeleteSelectionCommand.cpp:493
> +                    // Remove any style belonging to this node from typing style.
> +                    m_typingStyle->removeStyleAddedByNode(node.get());

This is very inefficient. A better way of fixin this bug is to set the typing style at the end of the whole deletion.

> LayoutTests/editing/deleting/delete-reset-typing-style.html:1
> +<title> Test case for bug https://bugs.webkit.org/show_bug.cgi?id=34608</title>

No DOCTYPE? no html? and no head?
Why does this test need to use quirks mode?

> LayoutTests/editing/deleting/delete-reset-typing-style.html:26
> +function Deletebold(){
> +var test = document.getElementById('bold');
> +window.getSelection().setPosition(test, test.innerHTML.length);
> +for (var i = 0; i < 11; i++)
> +    document.execCommand('Delete', false, null);
> +document.execCommand('InsertText', false, 'This should not be BOLD');
> +}

Please put a space after (). and indent the function definition.
Also use CamelCase or camelCase for the function name.

[Pravin D]

Comment on attachment 146379 [details]
Proposed Patch

When a node is deleted the associated style is not completely deleted in 
saveTypingStyleState(). 

saveTypingStyleState() only deletes the style the enclosing Anchor node(node with a link). In case there is no enclosing anchor node, the typing style is not updated properly.  

In the patch the style associated with the node is deleted only when the node is completely removed. Not sure if it is the correct location.
Will add more test case once am sure of the patch.

[Pravin D]

(In reply to comment #6)
> (From update of attachment 146379 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146379&action=review
> 
> > Source/WebCore/editing/DeleteSelectionCommand.cpp:493
> > +                    // Remove any style belonging to this node from typing style.
> > +                    m_typingStyle->removeStyleAddedByNode(node.get());
> 
> This is very inefficient. A better way of fixin this bug is to set the typing style at the end of the whole deletion.
>
 I was not sure if the styles will be properly calculated by the end of our deletion process. I guess it shud be. Let me check.

> > LayoutTests/editing/deleting/delete-reset-typing-style.html:1
> > +<title> Test case for bug https://bugs.webkit.org/show_bug.cgi?id=34608</title>
> 
> No DOCTYPE? no html? and no head?
> Why does this test need to use quirks mode?
> 
Tot it was not required. Will add the doctype and <head>

> > LayoutTests/editing/deleting/delete-reset-typing-style.html:26
> > +function Deletebold(){
> > +var test = document.getElementById('bold');
> > +window.getSelection().setPosition(test, test.innerHTML.length);
> > +for (var i = 0; i < 11; i++)
> > +    document.execCommand('Delete', false, null);
> > +document.execCommand('InsertText', false, 'This should not be BOLD');
> > +}
> 
> Please put a space after (). and indent the function definition.
> Also use CamelCase or camelCase for the function name.
Will incorporate in the next patch.

[Pravin D]

Created attachment 146685 [details]
Test Failure diff

I tried the approach suggested in Comment #6 (compute the style after all deletions)... This causes 2 editing layout test cases to fail. Attaching the diff files for the same. Need to know if the failures are expected or the fix is not proper....

[Pravin D]

(In reply to comment #9)
> Created an attachment (id=146685) [details]
> Test Failure diff
> 
> I tried the approach suggested in Comment #6 (compute the style after all deletions)... This causes 2 editing layout test cases to fail. Attaching the diff files for the same. Need to know if the failures are expected or the fix is not proper....
> 

As per our discusion on IRC I checked if the test case LayoutTests\editing\style\5084241.html has made a wrong assumption that the styling must be retained in a paragraph even after all the text in it has been deleted (refer Test failure https://bugs.webkit.org/attachment.cgi?id=146685).  
https://dvcs.w3.org/hg/editing/rev/0e6c95119654 seems so suggest that the assumption is correct... 

My guess is that we are suppose to retain the typing style before deletion in some cases as assumed/tested by  LayoutTests\editing\style\5084241.html and LayoutTests\editing\style\style-3681552-fix-002.html.

Currently we are always using the typing style before deletion. Using typing style after the deletion also might not entirely correct either. 

Plz let me know your opinion on this...

[Pravin D]

Created attachment 147558 [details]
Proposed Patch

[Ryosuke Niwa]

Comment on attachment 147558 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147558&action=review

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This should appear before the long description but after the bug URL.

> Source/WebCore/editing/FrameSelection.h:244
> +    bool shouldUpdateTypingStyle() { return m_updateTypingStyle; }
> +    void setUpdateTypingStyle(bool v) { m_updateTypingStyle = v; }
> +

I don't think we want to be adding these member functions and variables just for DeleteSelectionCommand. r-.

[Pravin D]

Created attachment 148162 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 148162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148162&action=review

> Source/WebCore/editing/DeleteSelectionCommand.cpp:672
> +    if (m_upstreamStart.deprecatedNode() == m_downstreamEnd.deprecatedNode() && m_upstreamStart.deprecatedNode()->isTextNode())

This is not right. Even if upstreamStart and downstreamEnd were in the same node, they could have a different offset.

[WebKit Review Bot]

Comment on attachment 148162 [details]
Patch

Attachment 148162 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12986016

New failing tests:
editing/style/5084241.html
editing/style/style-3681552-fix-002.html

[WebKit Review Bot]

Created attachment 148272 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pravin D]

Summary on the issue:

The issue seems to due to the check 
if (m_upstreamStart.deprecatedNode() == m_downstreamEnd.deprecatedNode() && m_upstreamStart.deprecatedNode()->isTextNode())
            return;

in DeleteSelectionCommand::saveTypingStyleState();

According to spec --
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#delete-the-selection

"If wrappers are being stripped, the empty inline element will be removed...But callers like the insertText command that intend to immediately insert new contents want to leave the wrappers, so the new contents are wrapped by the same thing as the old." 

Use case which works according to spec(to some extent I guess):

We have 2 deletions from start from B.

{text node 1}(1)<i>(2)B(3)</i>(4){text node 2}
During the 1st deletion,
upstreamStart = (1)
downstreamEnd = (4)
m_selectionToDelete.start() = (2)

B is deleted, saveTypingStyle() is called, style at (2) is saved as typing style and the node <i> is deleted.
After 1st deletion
{text node1 (1)}(2)(4){text node 2}

During 2nd deletion,
upstreamStart = (1)
downstreamEnd = (4)
m_selectionToDelete.start() = (2)
1 char from text node 1 is deleted, saveTypingStyle() is called. As upstreamStart and downstreamEnd point to different text nodes, new typing style at (2) is calculated/set. 
When we call a insert command now style from <i> will not be present.

However in our case
https://bug-34608-attachments.webkit.org/attachment.cgi?id=72151

{text node 1}(1)<i>(2)B(3)</i>(4){text node 2}
text node 2 is completely deleted. Then 
{text node 1}(1)<i>(2)B(3)</i>
Now we do 2 more deletions
During 1st deletion
upstreamStart = (1)
downstreamEnd = (3)
m_selectionToDelete.start() = (2)

B is deleted, saveTypingStyle() is called, style at (2) is saved as typing style and the node <i> is deleted.
After 1st deletion
{text node1 (1)(3)}

During 2nd deletion,
upstreamStart = (1)
downstreamEnd = (3)
m_selectionToDelete.start() = (1)

1 char from text node 1 is deleted, saveTypingStyle() is called. Here the new typing style is not calculated/set as upstreamStart and downstreamEnd point to the same text node. However we should have set the new typing style here. Subsequent deletions also do not reset the typing style in all of them  upstreamStart and downstreamEnd  belong to the same text node. Thus the issue.

We need to reset the typing style atleast when deleting the 1st seletion of a new text node.

[Ahmad Saleem]

I am getting different behavior across both test cases so I am not sure whether both test cases are trying to do same or not:

On Demo:

** Safari 15.6 on macOS 12.5 ***

It shows following text - "hellothis should not be bold" and when I try to type, it just type normal (not bold)

** Firefox Nightly 104 ***

It shows following text where 'world' is bold - hello world WebKit (but it types normally after 'Webkit' but does not type bold after 'world' even caret being next to 'd' character to inherit bold from previous word

** Chrome Canary 106 ***

It shows following text where 'world' is bold - hello world WebKit (but it types normally after 'Webkit' but type bold after 'world' (if caret is after 'd' but does not if it before 'W')

On "Another example...":

** Safari 15.6 on macOS 12.5 ***

If I backspace once - both 'Header' and 'Text' merge and become bold

** Firefox Nightly 104 ***

I had to backspace twice but after that both 'Header' and 'Text' merge and become bold

** Chrome Canary 106 ***

If I backspace once - both 'Header' and 'Text' merge and become bold

______

I am not sure on web-spec or expected behaviour but just wanted to share updated results across both test cases. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/97620948>