REGRESSION(3.2.3 - 4.0.2): Message composing: when I undo a color change to text, undo/redo behaves strangely

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

0) Start any HTML editor
1) Type text like "1 2 3"
3) Select the middle text (the "2" in this case)
4) Show Colors
5) Change the color of "2" to green (or whatever)
6) Hide Colors 
7) Undo (color of "2" goes back to black)
8) Redo

Expected results:

1 2 3 [with the "2" changed to green again]

Actual results:

1 2 [the text after the previous selection is gone]

[Enrica Casucci]

Created attachment 42074 [details]
Patch

[Adele Peterson]

Comment on attachment 42074 [details]
Patch

This generally looks pretty good to me.  Is it possible to reuse the doApply code for these commands and make them use the saved m_dummySpan or m_element1 if those elements are available?  It would be nice if we didn't have to replicate the logic.

[Enrica Casucci]

(In reply to comment #2)
> (From update of attachment 42074 [details])
> This generally looks pretty good to me.  Is it possible to reuse the doApply
> code for these commands and make them use the saved m_dummySpan or m_element1
> if those elements are available?  It would be nice if we didn't have to
> replicate the logic.

I thought about that initially and discussed it with Dan. We both agreed that it was a cleaner design to have separate entry points for apply and reapply, since the architecture has provision for this.
If you prefer, I could still have separate entry points but call a new private method that implements the common code.

[Adele Peterson]

OK, separate entry points makes sense.  I like the idea of a new private method to share the code.

[Darin Adler]

Comment on attachment 42074 [details]
Patch

Good approach!

> +void SplitElementCommand::doReapply()
> +{
> +    if (!m_element1)
> +        return;
> +    
> +    if (m_atChild->parentNode() != m_element2)
> +        return;
> +    
> +    Vector<RefPtr<Node> > children;
> +    for (Node* node = m_element2->firstChild(); node != m_atChild; node = node->nextSibling())
> +        children.append(node);
> +    
> +    ExceptionCode ec = 0;
> +    
> +    Node* parent = m_element2->parentNode();
> +    if (!parent)
> +        return;
> +    parent->insertBefore(m_element1.get(), m_element2.get(), ec);
> +    if (ec)
> +        return;
> +    
> +    size_t size = children.size();
> +    for (size_t i = 0; i < size; ++i)
> +        m_element1->appendChild(children[i], ec);
> +}

This needs to share more code with doApply. It's not good to have all that code copied and pasted. I suggest moving most of doApply into a new function and have both doApply and doReapply call it.

Same comment for WrapContentsInDummySpanCommand.

review- because of the copied and pasted code

[Enrica Casucci]

Created attachment 42138 [details]
Patch2

added executeApply to wrap code in common between doApply and doReapply.

[Adele Peterson]

Committed revision 50310.