Bug 51082 - execCommand('JustifyCenter') adds extra BR
Summary: execCommand('JustifyCenter') adds extra BR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-14 18:16 PST by Ryosuke Niwa
Modified: 2010-12-16 14:08 PST (History)
7 users (show)

See Also:


Attachments
fixes the bug (15.30 KB, patch)
2010-12-14 18:55 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-12-14 18:16:10 PST
execCommand('JustifyCenter') and any other editing command that modifies the CSS block properties will add an extra BR at the end of block.

e.g. justifying center: "hello" yields <div style="text-align: center;">hello<br></div> as supposed to <div style="text-align: center;">hello<br></div>.

This is a primary reason WebKit is scoring so low on http://www.browserscope.org/richtext2/test.
Comment 1 Ryosuke Niwa 2010-12-14 18:55:15 PST
Created attachment 76612 [details]
fixes the bug
Comment 2 Darin Adler 2010-12-15 18:57:22 PST
Comment on attachment 76612 [details]
fixes the bug

Seems inelegant to me to add a <br> and then later remove it.
Comment 3 Ryosuke Niwa 2010-12-15 19:14:37 PST
Thanks for the review!

(In reply to comment #2)
> (From update of attachment 76612 [details])
> Seems inelegant to me to add a <br> and then later remove it.

It really is. I'm not happy about that either. But we do this all ever the place in editing whenever we move paragraphs because ReplaceSelectionCommand and others require a placeholder. We could avoid doing this if we had used moveParagraphWithClones instead.

On a side note, we should really be using ApplyBlockElementCommand here because all we need to do is to wrap each paragraph with a div and ApplyBlockElementCommand does exactly that.  However, when I tried to deploy ApplyBlockElementCommand while working on this bug, I encountered numerous crashes and failures and I had to give up.  I'll come back to this issue later though.
Comment 4 Ryosuke Niwa 2010-12-16 14:08:14 PST
Comment on attachment 76612 [details]
fixes the bug

Clearing flags on attachment: 76612

Committed r74214: <http://trac.webkit.org/changeset/74214>
Comment 5 Ryosuke Niwa 2010-12-16 14:08:20 PST
All reviewed patches have been landed.  Closing bug.