Bug 82401

Summary: Deleting a paragraph of text should not add elements for typing style
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: New BugsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, enrica, justin.garcia, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-02
none
Removed platform-specific results for 5994480-2.html enrica: review+

Description Ryosuke Niwa 2012-03-27 17:18:54 PDT
Deleting a paragraph of text should reset typing style
Comment 1 Ryosuke Niwa 2012-03-27 17:26:59 PDT
Created attachment 134181 [details]
Patch
Comment 2 Darin Adler 2012-03-27 17:41:59 PDT
Comment on attachment 134181 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Deleting a paragraph of text should reset typing style

I don’t understand this bug title. Deleting a paragraph does not “reset typing style” after this patch. You delete the paragraph and then type and the typed text has the correct typing style.

What this patch does is get rid of the “persistent typing style” that was here but wasn’t needed. That change seems fine. It’s the bug title I object to.
Comment 3 Ryosuke Niwa 2012-03-27 18:24:05 PDT
yeah, the bug title is quite misleading as is. Can I just insert "persisttent" there? Or maybe "Deleting a paragraph of text should not leave style"?
Comment 4 WebKit Review Bot 2012-03-27 18:40:11 PDT
Comment on attachment 134181 [details]
Patch

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

New failing tests:
editing/inserting/5994480-2.html
Comment 5 WebKit Review Bot 2012-03-27 18:40:18 PDT
Created attachment 134193 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 6 Ryosuke Niwa 2012-03-28 11:40:15 PDT
Created attachment 134354 [details]
Removed platform-specific results for 5994480-2.html
Comment 7 Enrica Casucci 2012-03-28 12:33:17 PDT
Comment on attachment 134354 [details]
Removed platform-specific results for 5994480-2.html

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

> Source/WebCore/ChangeLog:9
> +        So change our behavior to match TextEdit and Firefox.

"We are changing our behavior" sounds better to me,

> LayoutTests/editing/deleting/delete-and-cleanup-expected.txt:8
> +PASS confirmedMarkup is '<b><div style="border: solid red"><br></div></b>'

Why does <b> stay? In all the other cases the markup is completely gone.

> LayoutTests/editing/deleting/delete-and-cleanup.html:41
> +testDelete("div", "<div><b><div style=\"border: solid red\"><i>Hello</i></div></b></div>", "<b><div style=\"border: solid red\"><br></div></b>");

Ditto.
Comment 8 Ryosuke Niwa 2012-03-28 12:35:28 PDT
Comment on attachment 134354 [details]
Removed platform-specific results for 5994480-2.html

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

>> LayoutTests/editing/deleting/delete-and-cleanup-expected.txt:8
>> +PASS confirmedMarkup is '<b><div style="border: solid red"><br></div></b>'
> 
> Why does <b> stay? In all the other cases the markup is completely gone.

That's because b happens to be outside of div, which is a block element. We try not to modify things outside of the enclosing block.
In the long term, we might be able to use ApplyStyleCommand's algorithm to push down styles here. The algorithm can handle these cases pretty well.

>> LayoutTests/editing/deleting/delete-and-cleanup.html:41
>> +testDelete("div", "<div><b><div style=\"border: solid red\"><i>Hello</i></div></b></div>", "<b><div style=\"border: solid red\"><br></div></b>");
> 
> Ditto.

Ditto.
Comment 9 Enrica Casucci 2012-03-28 12:47:17 PDT
Comment on attachment 134354 [details]
Removed platform-specific results for 5994480-2.html

Looks good to me.
Comment 10 Ryosuke Niwa 2012-03-28 14:38:12 PDT
Thanks for the review! Landing it now.
Comment 11 Ryosuke Niwa 2012-03-28 14:41:40 PDT
Committed r112444: <http://trac.webkit.org/changeset/112444>