Bug 32079 - Undo should not affect elements that are not editable
Summary: Undo should not affect elements that are not editable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-12-02 11:21 PST by Enrica Casucci
Modified: 2009-12-03 11:22 PST (History)
2 users (show)

See Also:


Attachments
patch (13.57 KB, patch)
2009-12-02 11:56 PST, Enrica Casucci
adele: 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-12-02 11:21:46 PST
If an element changes from editable to non-editable, all undo commands that apply to it and its descendant should be ignored.
Same goes for redo.
Comment 1 Enrica Casucci 2009-12-02 11:56:37 PST
Created attachment 44170 [details]
patch
Comment 2 WebKit Review Bot 2009-12-02 21:10:58 PST
style-queue ran check-webkit-style on attachment 44170 [details] without any errors.
Comment 3 Adele Peterson 2009-12-02 22:52:13 PST
Comment on attachment 44170 [details]
patch

Are there cases where the parent isn't editable, and its not attached, where we would want to allow the AppendNodeCommand?

>  void AppendNodeCommand::doApply()
>  {
> +    if (!m_parent->isContentEditable() && m_parent->attached())
> +        return;
> +        
>      ExceptionCode ec;
>      m_parent->appendChild(m_node.get(), ec);
>  }
Comment 4 Enrica Casucci 2009-12-03 10:10:40 PST
(In reply to comment #3)
> (From update of attachment 44170 [details])
> Are there cases where the parent isn't editable, and its not attached, where we
> would want to allow the AppendNodeCommand?
> 
> >  void AppendNodeCommand::doApply()
> >  {
> > +    if (!m_parent->isContentEditable() && m_parent->attached())
> > +        return;
> > +        
> >      ExceptionCode ec;
> >      m_parent->appendChild(m_node.get(), ec);
> >  }

I don't think so. Look at the ASSERT in the constructor.
    ASSERT(m_parent->isContentEditable() || !m_parent->attached());
Comment 5 Adele Peterson 2009-12-03 10:42:05 PST
ok, makes sense
Comment 6 Enrica Casucci 2009-12-03 11:22:27 PST
Committed revision 51645.