[webkit-reviews] review granted: [Bug 19795] execCommand FormatBlock creates lots of blockquotes : [Attachment 70208] added minor fix to ApplyBlockElementCommand

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 12 12:32:51 PDT 2010


Tony Chang <tony at chromium.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 19795: execCommand FormatBlock creates lots of blockquotes
https://bugs.webkit.org/show_bug.cgi?id=19795

Attachment 70208: added minor fix to ApplyBlockElementCommand
https://bugs.webkit.org/attachment.cgi?id=70208&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70208&action=review

Some questions to help my understanding of this code.

> WebCore/editing/FormatBlockCommand.h:41
> +    static bool isElementToApplyInFormatBlockCommand(const QualifiedName&
tagName);

Is this part of FormatBlockCommand because you want to merge it with the code
in Editor.cpp?	Seems like it could just be in the cpp file.

> WebCore/editing/FormatBlockCommand.h:47
> +    Node* enclosingBlockToSplitTreeTo(Node* startNode) const;

This method also seems like it could be static and fully in the cpp file.

>
LayoutTests/editing/execCommand/format-block-multiple-paragraphs-expected.txt:6
4
> +| <div>
> +|   <p>
> +|	 "<#selection-anchor>hello"
> +|	 <br>
> +|	 "world<#selection-focus>"

Why doesn't <p> replace the <div> in this case?

>
LayoutTests/editing/execCommand/format-block-multiple-paragraphs-expected.txt:1
80
> +|   <br>
> +|   "worl<#selection-focus>d"

This looks like it's off by a letter.  Maybe just file a bug for this case
since it's <pre>.

>
LayoutTests/editing/execCommand/format-block-multiple-paragraphs-expected.txt:2
02
> +| <div>
> +|   contenteditable=""
> +|   id="test9"
> +|   "

Did you mean to include all this in this test?

>
LayoutTests/editing/execCommand/format-block-multiple-paragraphs-expected.txt:3
39
> +|   <ul>
> +|	 <li>
> +|	   "<#selection-anchor>hello"
> +|   <ul>

The merging of these ul seems like something we can try to fix in a follow up
change.


More information about the webkit-reviews mailing list