execCommand FormatBlock creates lots of blockquotes

• Component: HTML Editing
• Status: RESOLVED FIXED

[Nick Santos]

Repro steps:
1) Select a few lines of text in a contentEditable block
2) run document.execCommand('FormatBlock', false, 'blockquote')

Expected:
1 blockquote

Actual:
lots o' blockquotes

[Mark Rowe (bdash)]

<rdar://problem/6041710>

[Ryosuke Niwa]

Created attachment 68840 [details]
work in progress

In this patch, I tried to match the code of FormatBlock as closely as possible to that of IndentOutdentCommand.  As a result much of FormatBlockCommand is rewritten but I think this is a good direction because we can probably share a lot of code between FormatBlockCommand and IndentOutdentCommand.  Now the question is how we share the code.  One option is to extract the shared code and move it to CompositeEditCommand.  Another option is to add a base class for IndentOutdentCommand, FormatBlockCommand, & InsertListCommand that inherits from CompositeEditCommand.  Since these three commands do very similar things, we might find this approach valuable if we wanted to avoid adding specialized functions to CompositeEditCommand.

Darin, Enrica, Ojan, & Tony, any thoughts on this?

[Darin Adler]

(In reply to comment #2)
>  Another option is to add a base class for IndentOutdentCommand, FormatBlockCommand, & InsertListCommand that inherits from CompositeEditCommand. Since these three commands do very similar things, we might find this approach valuable if we wanted to avoid adding specialized functions to CompositeEditCommand.

Either approach sounds fine to me.

If there is some data involved then we definitely would want a new class. If it’s just functions, then I think putting it in CompositeEditCommand is OK for now.

[Ryosuke Niwa]

(In reply to comment #3)
> Either approach sounds fine to me.
> 
> If there is some data involved then we definitely would want a new class. If it’s just functions, then I think putting it in CompositeEditCommand is OK for now.

I don't think there's any data involved (except possibly tagName for FormatBlock) but I'd really like to use virtual functions.  For example, indentIntoBlockquote and indentIntoBlockNode share very similar overall structure but it needs to do slightly different things at two places.  It'll really help to isolate those two logics into helper functions and override them in IndentOutdentCommand and FormatBlockCommand.

[Ryosuke Niwa]

Created attachment 70194 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 70208 [details]
added minor fix to ApplyBlockElementCommand

[Tony Chang]

Comment on attachment 70208 [details]
added minor fix to ApplyBlockElementCommand

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:64
> +| <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:180
> +|   <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:202
> +| <div>
> +|   contenteditable=""
> +|   id="test9"
> +|   "

Did you mean to include all this in this test?

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

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

[Ryosuke Niwa]

Thanks for the review!

(In reply to comment #7)
> > 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.

No, this is called in WebCore/editing/EditorCommand.cpp where we invoke FormatBlockCommand.

> > 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.

Sure.  I could do that.

> > LayoutTests/editing/execCommand/format-block-multiple-paragraphs-expected.txt:64
> > +| <div>
> > +|   <p>
> > +|     "<#selection-anchor>hello"
> > +|     <br>
> > +|     "world<#selection-focus>"
> 
> Why doesn't <p> replace the <div> in this case?

We should.  But we can't do that right now because we can't figure out whether or not div is fully selected.

> > LayoutTests/editing/execCommand/format-block-multiple-paragraphs-expected.txt:180
> > +|   <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>.

I suspect that it's a bug in text iterator.

> > LayoutTests/editing/execCommand/format-block-multiple-paragraphs-expected.txt:202
> > +| <div>
> > +|   contenteditable=""
> > +|   id="test9"
> > +|   "
> 
> Did you mean to include all this in this test?

No. Will fix.

> > LayoutTests/editing/execCommand/format-block-multiple-paragraphs-expected.txt:339
> > +|   <ul>
> > +|     <li>
> > +|       "<#selection-anchor>hello"
> > +|   <ul>
> 
> The merging of these ul seems like something we can try to fix in a follow up change.

Yes, I'm intending to fix a bunch of bugs like that in the bug 44918.  In this patch, I wanted to preserve the existing behavior as much as possible since I'm basically rewriting the entire function.

[Ryosuke Niwa]

Committed r69639: <http://trac.webkit.org/changeset/69639>

[Ryosuke Niwa]

Created attachment 70586 [details]
fixed moveParagraphWithClones for fast/html/nav-element.html

[Ryosuke Niwa]

(In reply to comment #9)
> Committed r69639: <http://trac.webkit.org/changeset/69639>

It turned out that this broke fast/html/nav-element.html because moveParagraphWithClones adds extra br.  My new patch fixes this problem as well.

[Ryosuke Niwa]

Reopening the bug since the first commit has been rolled out.

[Darin Adler]

Comment on attachment 70586 [details]
fixed moveParagraphWithClones for fast/html/nav-element.html

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

> WebCore/editing/EditorCommand.cpp:445
> +    if (!FormatBlockCommand::isElementToApplyInFormatBlockCommand(qualifiedTagName))
> +        return false;
> +
>      applyCommand(FormatBlockCommand::create(frame->document(), qualifiedTagName));

If I was doing this, I would have put this logic into FormatBlockCommand::create and had it return 0 if it’s an element it should not apply to. That seems easier to use than a separate function you have to know to call.

> WebCore/editing/FormatBlockCommand.cpp:78
> +// FIXME: We should consider mering this function with isElementForFormatBlockCommand in Editor.cpp

Typo: "mering".

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

I don’t understand the phrase “element to apply in command”. What does that mean?

[Ryosuke Niwa]

(In reply to comment #13)
> > WebCore/editing/EditorCommand.cpp:445
> > +    if (!FormatBlockCommand::isElementToApplyInFormatBlockCommand(qualifiedTagName))
> > +        return false;
> > +
> >      applyCommand(FormatBlockCommand::create(frame->document(), qualifiedTagName));
> 
> If I was doing this, I would have put this logic into FormatBlockCommand::create and had it return 0 if it’s an element it should not apply to. That seems easier to use than a separate function you have to know to call.

I like that design better too.  I was considering to do it in this patch as well but then this patch was already pretty big so I avoided it.  I'll probably do that when I merge isElementToApplyInFormatBlockCommand and isElementForFormatBlockCommand.

> > WebCore/editing/FormatBlockCommand.cpp:78
> > +// FIXME: We should consider mering this function with isElementForFormatBlockCommand in Editor.cpp
> 
> Typo: "mering".

Thanks.  Will fix.

> > WebCore/editing/FormatBlockCommand.h:41
> > +    static bool isElementToApplyInFormatBlockCommand(const QualifiedName& tagName);
> 
> I don’t understand the phrase “element to apply in command”. What does that mean?

We're checking whether or not an element can be applied using format block command or whether or not the element should be replaced by new element.  I think isElementForFormatBlockCommand is a better name but I didn't want to use the same name.  I'll merge this function with isElementForFormatBlockCommand so this function should be gone in no time.

[Ryosuke Niwa]

Committed r69836: <http://trac.webkit.org/changeset/69836>