[Webkit-unassigned] [Bug 45866] execCommand insertorderedlist creates a list inside p (violates content-model).

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45866





--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org>  2010-12-16 17:44:22 PST ---
(From update of attachment 76834)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.