Bug 30892 - REGRESSION(3.2.3 - 4.0.2): Message composing: when I undo a color change to text, undo/redo behaves strangely
Summary: REGRESSION(3.2.3 - 4.0.2): Message composing: when I undo a color change to t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2009-10-28 17:58 PDT by Enrica Casucci
Modified: 2009-10-29 16:37 PDT (History)
3 users (show)

See Also:


Attachments
Patch (12.94 KB, patch)
2009-10-28 18:18 PDT, Enrica Casucci
darin: review-
Details | Formatted Diff | Diff
Patch2 (14.18 KB, patch)
2009-10-29 13:54 PDT, Enrica Casucci
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2009-10-28 17:58:11 PDT
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]
Comment 1 Enrica Casucci 2009-10-28 18:18:14 PDT
Created attachment 42074 [details]
Patch
Comment 2 Adele Peterson 2009-10-29 11:00:02 PDT
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.
Comment 3 Enrica Casucci 2009-10-29 11:04:54 PDT
(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.
Comment 4 Adele Peterson 2009-10-29 11:07:49 PDT
OK, separate entry points makes sense.  I like the idea of a new private method to share the code.
Comment 5 Darin Adler 2009-10-29 13:16:48 PDT
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
Comment 6 Enrica Casucci 2009-10-29 13:54:11 PDT
Created attachment 42138 [details]
Patch2

added executeApply to wrap code in common between doApply and doReapply.
Comment 7 Adele Peterson 2009-10-29 16:37:35 PDT
Committed revision 50310.