Bug 45866 – execCommand insertorderedlist creates a list inside p (violates content-model).

• Component: HTML Editing
• Status: NEW

[Nobody]

On selecting multiple paragraphs, using execCommand("insertorderedlist") or execCommand("insertunorderedlist") creates <ol> or <ul> element INSIDE the <p> element, which is invalid HTML.

[Ryosuke Niwa]

Could you attach a sample HTML file that demonstrates this bug?  Or detailed reproduction steps?

[Nobody]

Created attachment 68792 [details]
WYSIWYG editor test page

I've cooked up a quick sample at the following URL : http://juneoh.net/lab/wysiwyg.html
To reproduce, select all of the iframe content and press insertorderedlist button,
and a inspector will show a <ol> created inside <p>.

[Ryosuke Niwa]

Created attachment 68800 [details]
demo

New demo since the attachment 68792 [details] loads blank.html which doesn't exist.

[Ryosuke Niwa]

(In reply to comment #2)
> Created an attachment (id=68792) [details]
> WYSIWYG editor test page
> 
> I've cooked up a quick sample at the following URL : http://juneoh.net/lab/wysiwyg.html
> To reproduce, select all of the iframe content and press insertorderedlist button,
> and a inspector will show a <ol> created inside <p>.

Thanks for the demo, I have a reduction now.  I suppose we have a similar problems with indentation and format block as well.

[Nobody]

Created attachment 68801 [details]
blank.html file to go with WYSIWYG demo

Oops, thanks for the notice. Here's the blank.html that I missed.

[Raffaele]

Created attachment 76834 [details]
Proposed patch

I am a newbie in c++ and WebKit development, so maybe there is a better way to write this patch.
However, if I can improve the code and make it accepted, I will be happy to reshape the patch following your advices.
Thanks.

Raffaele

[WebKit Review Bot]

Attachment 76834 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/execCommand/insert-list-inside-p-expected.txt', u'LayoutTests/editing/execCommand/insert-list-inside-p.html', u'WebCore/ChangeLog', u'WebCore/editing/InsertListCommand.cpp']" exit_code: 1
WebCore/editing/InsertListCommand.cpp:376:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 76834 [details]
Proposed patch

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

Thanks for tackling this bug! I'm impressed by how close you've got in this first patch. But I think we can improve a little.

> WebCore/editing/InsertListCommand.cpp:373
> +        if (enclosingBlockFlowElement(start)->hasTagName(pTag)) {
> +            // 'p' elements can contain only 'phrasing content'.
> +            // Therefore we must move the list insertion point outside the 'p' element.
> +            Element* pElement = static_cast<Element*>(enclosingBlockFlowElement(start));

This bug is for p but I think there are other elements we need to avoid.  Also, p could be an ancestor of some other block flow element, right?  e.g.
<p><span style="display:block;">hello</span></p>.

Also, we don't prefix pointer by p.  And it's not ideal that we're calling enclosingBlockFlowElement twice here.  We could just define it outside the if statement.

> WebCore/editing/InsertListCommand.cpp:377
> +            if (!pElement->parentNode()->isContentEditable())
> +                //If 'p' is the root editable element, don't create the list at all.
> +                return 0;

Mn... I'm not sure not listifying is the correct behavior here but I don't have an alternative to offer.

> WebCore/editing/InsertListCommand.cpp:384
> +                splitElement(pElement, insertionPos.anchorNode());
> +                removeNode(insertionPos.anchorNode());

For the reason I stated above, this should probably be splitting the tree, not just the element.

> LayoutTests/editing/execCommand/insert-list-inside-p-expected.txt:2
> +Dump of markup 1:

We should add some description here so that the correct output is obvious.

> LayoutTests/editing/execCommand/insert-list-inside-p.html:2
> +<p>This test execute 'InsertList' with the caret inside a 'p' paragraph. You should see the list replacing the p element.</p>

Please put this in Markup.description as in:
Markup.description("This test execute...");

Of course, you can add it to the script element below.  No need to insert a new script element here.

> LayoutTests/editing/execCommand/insert-list-inside-p.html:15
> +Markup.dump("test1");

You can add the description as to what each test is doing by:
Markup.dump("test1", "description");

> LayoutTests/editing/execCommand/insert-list-inside-p.html:29
> +range.setStart(p.firstChild.nextSibling.nextSibling, 3);
> +range.setEnd(p.lastChild.previousSibling.previousSibling, 4)

Why don't you just use p.childNodes[2] and p.childNodes[3] instead?

[Ryosuke Niwa]

Comment on attachment 76834 [details]
Proposed patch

Please address the above points I made.

[Raffaele]

Sorry for not replying sooner.

> Also, p could be an ancestor of some other block flow element, right?

You are perfectly right of course. I guess the correct behavior, here, is to select the nearest ancestor that isn't a phrasing element (http://www.w3.org/TR/html5/content-models.html#phrasing-content).
I can add a new function to htmlediting.cpp, something like:
 
bool isPhrasingElement(const Node* node)

and then use it together with "enclosingNodeOfType" to select the ancestor.
Is this the right approach?

Thanks

Raffaele

[Ryosuke Niwa]

(In reply to comment #10)
> You are perfectly right of course. I guess the correct behavior, here, is to select the nearest ancestor that isn't a phrasing element (http://www.w3.org/TR/html5/content-models.html#phrasing-content).
> I can add a new function to htmlediting.cpp, something like:
> 
> bool isPhrasingElement(const Node* node)
> 
> and then use it together with "enclosingNodeOfType" to select the ancestor.
> Is this the right approach?

That sounds right.  You may be interested in looking at isBlock function.  It's called at various places in the place of !isPhrasingElement.

[Raffaele]

> You may be interested in looking at isBlock function.  It's called at various places in the place of !isPhrasingElement.

But 'isBlock' returns 'true' for phrasing element with the display style property set to 'block'. It's for that reason I wanted to add the new function 'isPhrasingElemenet'.

Raffeale

[Raffaele]

I'm thinking of adding a new public virtual method to the HTMLElement class:

virtual bool isPhrasingContent() const    { return false; }

and reimplementing it within HTMLElement subclasses as appropriate.
I guess such a method can be useful even beyond the necessities of my patch.
Can I follow this "path"?

Raffaele

[Ryosuke Niwa]

(In reply to comment #13)
> I'm thinking of adding a new public virtual method to the HTMLElement class:
> 
> virtual bool isPhrasingContent() const    { return false; }

I like the idea of it but adding a new member function to Node will require careful consideration. +darin, eseidel, othermaciej, & weinig because they might have something to say about such a change.

[Adam Barth]

The parser might already know which tag names are phrasing.

[Ahmad Saleem]

Sharing updated test result based on "demo":

*** Safari Technology Preview 152 & Chrome Canary 107 ***

<p><ol><li>hello</li></ol></p> <p>world</p>

*** Firefox Nightly 107 ***

<ol><li> <br></li><li>hello</li></ol> <p>world</p> 

______

Just wanted to share updated test results. Thanks!