Bug 43166 - InsertOrderedList does not switch the list type properly when it has an inner list.
: InsertOrderedList does not switch the list type properly when it has an inner...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: All All
: P2 Blocker
Assigned To: Nobody
:
Depends on:
Blocks: 33668
  Show dependency treegraph
 
Reported: 2010-07-28 18:17 PDT by Ryosuke Niwa
Modified: 2010-07-29 23:43 PDT (History)
7 users (show)

See Also:


Attachments
fixes the bug (17.42 KB, patch)
2010-07-28 19:47 PDT, Ryosuke Niwa
darin: review-
Details | Formatted Diff | Diff
fixed per darin's comments (19.63 KB, patch)
2010-07-29 12:27 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
made the call to endingSelection().firstRange().get() inline (19.58 KB, patch)
2010-07-29 15:26 PDT, Ryosuke Niwa
darin: review-
Details | Formatted Diff | Diff
fixed mergeWithNeighboringLists (19.78 KB, patch)
2010-07-29 17:34 PDT, Ryosuke Niwa
darin: review+
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-07-28 18:17:27 PDT
Reproduction steps:
1. Go to http://www.mozilla.org/editor/midasdemo/
2. Type in <ul><li>hello</li><ol><li>world</li></ol><li>webkit</li></ul> in HTML mode.
The HTML is an unordered list with ordered list in the middle.
3. Insert ordered list.

Expected result:
<ol><li>hello</li><ol><li>world</li></ol><li>webkit</li></ol>
(outer list is converted to an ordered list)

Actual result:
<ol><li>hello</li></ol><ul><li>world</li></ul><ol><li>webkit</li></ol>
(outer list is split into two ordered list and inner list is converted to unordered list)
Comment 1 Ryosuke Niwa 2010-07-28 19:47:02 PDT
Created attachment 62913 [details]
fixes the bug
Comment 2 Darin Adler 2010-07-29 11:37:41 PDT
Comment on attachment 62913 [details]
fixes the bug

Looks good. A few comments.

> +    if (previousList && previousList->isHTMLElement() && canMergeLists(previousList, listElement))

> +    if (nextList && nextList->isHTMLElement() && canMergeLists(listElement, nextList)) {

If we need an isHTMLElement check here, then why does canMergeLists take an Element*?

If canMergeLists takes an Element*, then should it do the isHTMLElement check itself?

> +    const QualifiedName listTag = (m_type == OrderedList) ? olTag : ulTag;

This should be const QualifiedName& rather than const QualifiedName, to generate slightly more efficient code and avoid a ref/deref of the tag.

> +    RefPtr<Range> currentSelection = endingSelection().firstRange();
> +    doApplyForSingleParagraph(false, listTag, currentSelection.get());

You could do this without a local variable if you think that would be more readable.

    doApplyForSingleParagraph(false, listTag, endingSelection().firstRange().get());

> +    bool selectionHasListOfType(const VisiblePosition& start, const VisiblePosition& end, const QualifiedName&);

Too bad we have to pass two VisiblePosition. It would be nice if "selection" was a single argument.

> +    HTMLElement* mergeWithNeighboringLists(HTMLElement*);

Using raw pointers in cases like this can be unsafe if there are DOM mutation events involved. You might need the result to be PassRefPtr in case the list is destroyed during the merging process.

Do the new tests really cover all the branches in the code? I see lots of branches in the code and only a couple test cases. I don't think there is enough test coverage here.

review- so you can at least fix the const QualifiedName& thing.
Comment 3 Ryosuke Niwa 2010-07-29 12:27:56 PDT
Created attachment 62975 [details]
fixed per darin's comments

(In reply to comment #2)
> (From update of attachment 62913 [details])
> Looks good. A few comments.

Thanks for the comments.

> > +    if (previousList && previousList->isHTMLElement() && canMergeLists(previousList, listElement))
> 
> > +    if (nextList && nextList->isHTMLElement() && canMergeLists(listElement, nextList)) {
> 
> If we need an isHTMLElement check here, then why does canMergeLists take an Element*?

Good point. So the reason canMergeLists takes two Element* is because previousSiblingElement/nextSiblingElement returns Element and having to check that they are indeed HTMLElement is usually redundant since we check the tag name in canMergeLists, and one of the list is created by us or returned by enclosingList.  Added a check to make sure they're indeed HTMLElement in canMergeLists.

> If canMergeLists takes an Element*, then should it do the isHTMLElement check itself?
> 
> > +    const QualifiedName listTag = (m_type == OrderedList) ? olTag : ulTag;
> 
> This should be const QualifiedName& rather than const QualifiedName, to generate slightly more efficient code and avoid a ref/deref of the tag.

Ah! I didn't catch that. Fixed.

> > +    RefPtr<Range> currentSelection = endingSelection().firstRange();
> > +    doApplyForSingleParagraph(false, listTag, currentSelection.get());
> 
> You could do this without a local variable if you think that would be more readable.
> 
>     doApplyForSingleParagraph(false, listTag, endingSelection().firstRange().get());

Mn.. but doesn't that lead to a memory leak since firstRange() returns a PassRefPtr?  Because I'm passing the same range object to multiple doApplyForSingleParagraph in the case of range selection, I don't really want doApplyForSingleParagraph to take PassRefPtr.

> > +    bool selectionHasListOfType(const VisiblePosition& start, const VisiblePosition& end, const QualifiedName&);
> 
> Too bad we have to pass two VisiblePosition. It would be nice if "selection" was a single argument.

Changed to take VisibleSelection.

> > +    HTMLElement* mergeWithNeighboringLists(HTMLElement*);
> 
> Using raw pointers in cases like this can be unsafe if there are DOM mutation events involved. You might need the result to be PassRefPtr in case the list is destroyed during the merging process.

Fixed.

> Do the new tests really cover all the branches in the code? I see lots of branches in the code and only a couple test cases. I don't think there is enough test coverage here.

Sorry, I forgot to fix the test.  I'm adding two tests here because the list child added by fixOrphanedListChild is supposed to be merged with surrounding lists and this was not tested.
Comment 4 Darin Adler 2010-07-29 14:46:23 PDT
(In reply to comment #3)
> > > +    RefPtr<Range> currentSelection = endingSelection().firstRange();
> > > +    doApplyForSingleParagraph(false, listTag, currentSelection.get());
> > 
> > You could do this without a local variable if you think that would be more readable.
> > 
> >     doApplyForSingleParagraph(false, listTag, endingSelection().firstRange().get());
> 
> Mn.. but doesn't that lead to a memory leak since firstRange() returns a PassRefPtr?  Because I'm passing the same range object to multiple doApplyForSingleParagraph in the case of range selection, I don't really want doApplyForSingleParagraph to take PassRefPtr.

No, it does not lead to a memory leak. The PassRefPtr that is the result of the firstRange function goes out of scope after the function returns, and that's when it calls deref.
Comment 5 Ryosuke Niwa 2010-07-29 15:26:28 PDT
Created attachment 63000 [details]
made the call to endingSelection().firstRange().get() inline

(In reply to comment #4)
> > Mn.. but doesn't that lead to a memory leak since firstRange() returns a PassRefPtr?  Because I'm passing the same range object to multiple doApplyForSingleParagraph in the case of range selection, I don't really want doApplyForSingleParagraph to take PassRefPtr.
> 
> No, it does not lead to a memory leak. The PassRefPtr that is the result of the firstRange function goes out of scope after the function returns, and that's when it calls deref.

I see. Thanks for the clarification. Fixed.
Comment 6 Darin Adler 2010-07-29 16:30:13 PDT
Comment on attachment 63000 [details]
made the call to endingSelection().firstRange().get() inline

> +PassRefPtr<HTMLElement> InsertListCommand::mergeWithNeighboringLists(HTMLElement* listElement)
> +{
> +    Element* previousList = listElement->previousElementSibling();
> +    Element* nextList = listElement->nextElementSibling();
> +    if (canMergeLists(previousList, listElement))
> +        mergeIdenticalElements(previousList, listElement);
> +    if (canMergeLists(listElement, nextList)) {
> +        mergeIdenticalElements(listElement, nextList);
> +        return static_cast<HTMLElement*>(nextList);
> +    }
> +    return listElement;
> +}

Thanks for changing the function result into a PassRefPtr.

If DOM mutation events can run while we make changes, then this code is unsafe. After either of the two calls to mergeIdenticalElements returns, listElement or nextList might be pointing to a deleted object. Thus, they should both be RefPtr or PassRefPtr.

Calling nextElementSibling after handling previousList would be another way of avoiding using a possibly-bad nextList pointer, but it wouldn't help you safely return nextList after calling mergeIdenticalElements, so I think you need a RefPtr.

The patch otherwise looks great.

review- because I'd like you to use RefPtr more to avoid potential use-after-free as explained above
Comment 7 Ryosuke Niwa 2010-07-29 17:34:13 PDT
Created attachment 63017 [details]
fixed mergeWithNeighboringLists

(In reply to comment #6)
> If DOM mutation events can run while we make changes, then this code is unsafe. After either of the two calls to mergeIdenticalElements returns, listElement or nextList might be pointing to a deleted object. Thus, they should both be RefPtr or PassRefPtr.

A good point.

> Calling nextElementSibling after handling previousList would be another way of avoiding using a possibly-bad nextList pointer, but it wouldn't help you safely return nextList after calling mergeIdenticalElements, so I think you need a RefPtr.

Oh right, I feel like I encountered a bug like that a year ago.
Comment 8 Darin Adler 2010-07-29 20:03:32 PDT
Comment on attachment 63017 [details]
fixed mergeWithNeighboringLists

> +PassRefPtr<HTMLElement> InsertListCommand::mergeWithNeighboringLists(PassRefPtr<HTMLElement> listElement)
> +{
> +    RefPtr<HTMLElement> listNode = listElement;

More conventional naming would be to name the PassRefPtr passedListElement and name the local variable listElement. But given the context, I would use the name passedList and list. In this function everything is an element and there's no need to use the word element.

> +        return listNode;

It's slightly more efficient if you return listNode.release() here.

> +    return listNode;

It's slightly more efficient if you return listNode.release() here.

> +            RefPtr<HTMLElement> listElement = createHTMLElement(document(), listTag);
> +            insertNodeBefore(listElement, listNode);

The local variable names here are not all that good. Here, listNode is an enclosing HTML list element and listElement is a newly created HTML list element. Both are lists, both are elements, both are nodes, so it's confusing to have one named listNode and the other named listElement. It's like naming one variable startNumber and another variable startInteger when they contain two distinct values.
Comment 9 Ryosuke Niwa 2010-07-29 20:56:27 PDT
Will commit with new expected results since http://trac.webkit.org/changeset/64303 has changed the format of dumpAsMarkup tests.

(In reply to comment #8)
> (From update of attachment 63017 [details])
> > +PassRefPtr<HTMLElement> InsertListCommand::mergeWithNeighboringLists(PassRefPtr<HTMLElement> listElement)
> > +{
> > +    RefPtr<HTMLElement> listNode = listElement;
> 
> More conventional naming would be to name the PassRefPtr passedListElement and name the local variable listElement. But given the context, I would use the name passedList and list. In this function everything is an element and there's no need to use the word element.

Fixed.

> > +        return listNode;
> 
> It's slightly more efficient if you return listNode.release() here.
> 
> > +    return listNode;
> 
> It's slightly more efficient if you return listNode.release() here.

Fixed.

> > +            RefPtr<HTMLElement> listElement = createHTMLElement(document(), listTag);
> > +            insertNodeBefore(listElement, listNode);
> 
> The local variable names here are not all that good. Here, listNode is an enclosing HTML list element and listElement is a newly created HTML list element. Both are lists, both are elements, both are nodes, so it's confusing to have one named listNode and the other named listElement. It's like naming one variable startNumber and another variable startInteger when they contain two distinct values.

Yeah, I agree. That was very confusing. Renamed listElement as newList.
Comment 10 Ryosuke Niwa 2010-07-29 23:43:05 PDT
The patch landed as http://trac.webkit.org/changeset/64337, closing the bug.