Bug 88793 – Implement undoscope attribute.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Rakesh]

Implement undoscope attribute specified at http://dvcs.w3.org/hg/undomanager/raw-file/tip/undomanager.html.

[Rakesh]

Created attachment 146883 [details]
Proposed patch

Initial patch.

[Ryosuke Niwa]

Comment on attachment 146883 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146883&action=review

> Source/WebCore/dom/Element.idl:146
> +        attribute [Conditional=UNDO_MANAGER] boolean undoScope;

This should just "reflect".

> Source/WebCore/dom/Element.cpp:2126
> +void Element::setUndoScope(bool undoScope)
> +{
> +    setAttribute(undoscopeAttr, undoScope ? "true" : "false");
> +    if (!undoScope) {
> +        if (undoManager())
> +            rareData()->m_undoManager.clear();
> +        return;
> +    }
> +
> +    if (!isRootEditableElement())
> +        return;
> +    createUndoManager();
> +}

I think this is backwards. parsedAttribute should be calling this function instead.

> Source/WebCore/html/HTMLElement.cpp:218
> +    if (attribute.name() == undoscopeAttr && equalIgnoringCase(attribute.value(), "true") && isRootEditableElement())

undoscope attribute doesn't take boolean values. It's a boolean attribute, meaning that it's set iff we have undoscope=undoscope or undoscope.

> Source/WebCore/html/HTMLElement.cpp:221
> +    if (attribute.name() == contenteditableAttr && (attribute.isEmpty() || !equalIgnoringCase(attribute.value(), "false")) && equalIgnoringCase(fastGetAttribute(HTMLNames::undoscopeAttr), "true"))
> +        createUndoManager();

When contenteditable attribute is set, we need to disconnect existing undo manager in the subtree.

[Rakesh]

Created attachment 147552 [details]
Updated patch

[Ryosuke Niwa]

Comment on attachment 147552 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147552&action=review

> Source/WebCore/dom/Element.cpp:2111
> +void Element::createUndoManager()
> +{
> +    if (!ensureRareData()->m_undoManager)
> +        rareData()->m_undoManager = UndoManager::create(this);
> +}

This function is only called in setUndoScope.
I'm failing to see why we need to have a separate function for this.

> Source/WebCore/dom/Element.cpp:2121
> +    if (!fastHasAttribute(undoscopeAttr)) {

Why does a function called setUndoScope checks if it has undoscope attribute or not?
I would have merged this function into createUndoManager and checked the existence of undoscope attribute in restoreDescendentsUndoManagers.

> Source/WebCore/dom/Element.cpp:2124
> +            undoManager()->disconnect();
> +            clearUndoManager();

Instead of forcing users to call disconnect before clearUndoManager.
I would have just made clearUndoManager() automatically call disconnect.
In fact, I would have added disconnectUndoManager() instead of clearUndoManager.

> Source/WebCore/dom/Element.cpp:2137
> +    for (Node* node = firstChild(); node && node->isElementNode(); node = node->traverseNextNode(this)) {
> +        Element* element = toElement(node);

This is not right. You can certainly encounter a text node before seeing the first element. e.g. <div>hello<div undoScope></div></div>
r- because of this bug.

> Source/WebCore/dom/Element.cpp:2148
> +    for (Node* node = firstChild(); node && node->isElementNode(); node = node->traverseNextNode(this)) {
> +        Element* element = toElement(node);

Ditto.

> Source/WebCore/editing/UndoManager.cpp:58
> +        // FIXME Set INVALID_ACCESS_ERR exception.

Missing : after FIXME.

> LayoutTests/editing/undomanager/undoscope-attribute.html:5
> +<meta charset="utf-8">
> +<script src="../../fast/js/resources/js-test-pre.js"></script>

For undo manager tests, we're going to use testharness.js instead of js-test-pre.js so that we may submit them to W3C later.

> LayoutTests/editing/undomanager/undoscope-attribute.html:37
> +shouldBe('div2.contentEditable = false; Object.prototype.toString.call(document.getElementById("newlyAdded").undoManager)', "'[object UndoManager]'");

The name newlyAdded is inappropriate here.
Also, we need to test a case where we have multiple undo scope hosts, and a case where we have a bunch of text nodes, and other random nodes around them.

> LayoutTests/editing/undomanager/undoscope-attribute-expected.txt:7
> +PASS div1.undoScope is true
> +PASS Object.prototype.toString.call(div1.undoManager) is '[object UndoManager]'

This output isn't helpful because I can't see what div1 is. I would have added a function like createElement and used evalAndLog like this:
evalAndLog("div1 = createElement('div', {'undoscope':null, 'contenteditable: 'true'});");
Also, why do we need to use Object.prototype.toString.call? Can't we just do div1.undoManager.toString() ?

[Rakesh]

(In reply to comment #4)
Thanks for reviewing this patch.

> This function is only called in setUndoScope.
> I'm failing to see why we need to have a separate function for this.

> Why does a function called setUndoScope checks if it has undoscope attribute or not?
> I would have merged this function into createUndoManager and checked the existence of undoscope attribute in restoreDescendentsUndoManagers.

May be I will merge createUndoManager and setUndoScope into a fucntion with name handleUndoScopeAttributeChange.

> Instead of forcing users to call disconnect before clearUndoManager.
> I would have just made clearUndoManager() automatically call disconnect.
> In fact, I would have added disconnectUndoManager() instead of clearUndoManager.

Will do.

> > Source/WebCore/dom/Element.cpp:2137
> > +    for (Node* node = firstChild(); node && node->isElementNode(); node = node->traverseNextNode(this)) {
> > +        Element* element = toElement(node);
> 
> This is not right. You can certainly encounter a text node before seeing the first element. e.g. <div>hello<div undoScope></div></div>
> r- because of this bug.
> 
My bad, will change this.

> > Source/WebCore/editing/UndoManager.cpp:58
> > +        // FIXME Set INVALID_ACCESS_ERR exception.
> 
> Missing : after FIXME.
> 
Will add ':'.

> > LayoutTests/editing/undomanager/undoscope-attribute.html:5
> > +<meta charset="utf-8">
> > +<script src="../../fast/js/resources/js-test-pre.js"></script>
> 
> For undo manager tests, we're going to use testharness.js instead of js-test-pre.js so that we may submit them to W3C later.

Initially I tried with testharness.js, some how I could not get the output, will give a try once again.

> The name newlyAdded is inappropriate here.
> Also, we need to test a case where we have multiple undo scope hosts, and a case where we have a bunch of text nodes, and other random nodes around them.
> 
> This output isn't helpful because I can't see what div1 is. I would have added a function like createElement and used evalAndLog like this:
> evalAndLog("div1 = createElement('div', {'undoscope':null, 'contenteditable: 'true'});");
> Also, why do we need to use Object.prototype.toString.call? Can't we just do div1.undoManager.toString() ?

Will try all these in the new test.

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
> Thanks for reviewing this patch.
> 
> > This function is only called in setUndoScope.
> > I'm failing to see why we need to have a separate function for this.
> 
> > Why does a function called setUndoScope checks if it has undoscope attribute or not?
> > I would have merged this function into createUndoManager and checked the existence of undoscope attribute in restoreDescendentsUndoManagers.
> 
> May be I will merge createUndoManager and setUndoScope into a fucntion with name handleUndoScopeAttributeChange.

I don't think that's a good name. These two functions together create a new undo manager, but does not destroy the existing undo managers. If a function is named like handleUndoScopeAttributeChange, I would expect it to handle both. Note I'm not saying that you should add handleUndoScopeAttributeChange. I'm just objecting to the name. createUndoManager is a much better name here.

[Rakesh]

Created attachment 148329 [details]
patch3

Addressed review comments.

> This output isn't helpful because I can't see what div1 is. I would have added a function like createElement and used evalAndLog like this:
> evalAndLog("div1 = createElement('div', {'undoscope':null, 'contenteditable: 'true'});");

Do we have any way in testharness.js to log something like this. As of now I have added some description to each step.

[Ryosuke Niwa]

Comment on attachment 148329 [details]
patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=148329&action=review

> Source/WebCore/dom/Element.idl:146
> +        // Undo Manager

This comment is a pure noise. Please remove.

> Source/WebCore/dom/Element.cpp:2111
> +    return fastHasAttribute(HTMLNames::undoscopeAttr) && undoManager();

The call to fastHasAttribute(HTMLNames::undoscopeAttr) is redundant here because undoManager() returns false when undoscope is not set.

> Source/WebCore/dom/Element.cpp:2118
> +    if (hasRareData())
> +        return rareData()->m_undoManager;
> +    return 0;

Why don't we use ternary operator here?

> Source/WebCore/dom/Element.cpp:2121
> +void Element::createUndoManager()

createUndoManager is a misnomer here it doesn't always create undo manager.
Maybe createUndoManagerIfPossible?

I think it would be cleaner if the check for isContentEditable() && !isRootEditableElement() was in the caller instead.

> Source/WebCore/dom/Element.cpp:2143
> +        if (!node->isElementNode() || !toElement(node)->undoManager())
> +            continue;
> +        toElement(node)->disconnectUndoManager();

Here, you're obtaining the node rare data twice by calling undoManager() before calling disconnectUndoManager().
Since disconnectUndoManager bails out when there isn't undo manager, it's probably better to always call disconnectUndoManager instead.

Also, it would have been cleaner if the call to disconnectUndoManager as in the if statement as in:
if (node->isElementNode())
    toElement(node)->disconnectUndoManager();

> Source/WebCore/html/HTMLElement.cpp:226
> +        if (equalIgnoringCase(attribute.value(), "false"))
> +            return restoreDescendentsUndoManagers();
> +        return disconnectDescendentsUndoManagers();

I don't think this works. contenteditable could also be set to "inherent".
Also, value() could be null when the attribute had been removed.
See http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#contenteditable

r- because of this.

> LayoutTests/editing/undomanager/undoscope-attribute.html:22
> +var div1 = document.getElementById("divid1");
> +var div2 = document.getElementById("divid2");
> +var div3 = document.getElementById("divid3");

Instead of 1-3, use some descriptive name instead.

[Caio Marcelo de Oliveira Filho]

We should also consider the case when a node with undoscope is removed from an editing host that also has undoscope. It seems to me that in this case the undomanager of the removed node must be recreated.

[Rakesh]

(In reply to comment #8)
Thanks for review.
> 
> I think it would be cleaner if the check for isContentEditable() && !isRootEditableElement() was in the caller instead.
> 

I am not sure if the present check for isContentEditable() or isRootEditableElement() in parsing code is fine.
Even if updateLayoutIgnoringPendingStylesheets (as discussed on IRC) is called before checking, the present renderer may still be not created and we may not get the correct result for isContentEditable/isRootEditableElement. Correct me if I am missing something.

Can we check for editing host/editable by checking for contenteditable attribute  in the tree or something else we can do?

[Rakesh]

For the following example:

<div id=divCE contenteditable=true >
    <div id=divUS undoscope >  </div>
</div>

when undoscope is being parsed we need to know if divUS is editable element or root editable element to decide if we have to create an UndoManager for div2 or not.

The issue is when isContentEditable() and isRootEditableElement() are based on renderers of the node and at time of parsing we are not sure if renderers are created.

I tried invoking updateLayoutIgnorePendingStylesheets() before the above checks but it does not help, renderer's still not created.

Please suggest what could be the better approach for solving this?

[Ryosuke Niwa]

(In reply to comment #11)
> For the following example:
> 
> <div id=divCE contenteditable=true >
>     <div id=divUS undoscope >  </div>
> </div>
> 
> when undoscope is being parsed we need to know if divUS is editable element or root editable element to decide if we have to create an UndoManager for div2 or not.
> 
> The issue is when isContentEditable() and isRootEditableElement() are based on renderers of the node and at time of parsing we are not sure if renderers are created.
> 
> I tried invoking updateLayoutIgnorePendingStylesheets() before the above checks but it does not help, renderer's still not created.

Yeah... I don't know how to solve that problem. Maybe we can wait until the parser yields? We layout & paint when the parse yields. Eric might have some idea on this.

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #11)
> > For the following example:
> > 
> > <div id=divCE contenteditable=true >
> >     <div id=divUS undoscope >  </div>
> > </div>
> > 
> > when undoscope is being parsed we need to know if divUS is editable element or root editable element to decide if we have to create an UndoManager for div2 or not.
> > 
> > The issue is when isContentEditable() and isRootEditableElement() are based on renderers of the node and at time of parsing we are not sure if renderers are created.
> > 
> > I tried invoking updateLayoutIgnorePendingStylesheets() before the above checks but it does not help, renderer's still not created.
> 
> Yeah... I don't know how to solve that problem. Maybe we can wait until the parser yields? We layout & paint when the parse yields. Eric might have some idea on this.

Oh noes.  Please don't depend on renderers during parsing.  You'll slow down parsing:
https://bugs.webkit.org/show_bug.cgi?id=70009

In any case, you should design for the dynamic case, and then that tends to not be any different than parsing.

I'm happy to answer further questions if you need?  I fear I wasn't very helpful.

[Sukolsak Sakshuwong]

Created attachment 154214 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 154217 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154217 [details]
Patch

Attachment 154217 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13342083

New failing tests:
fast/loader/loadInProgress.html
fast/inline/positionedLifetime.html
fast/loader/unload-form-post-about-blank.html

[WebKit Review Bot]

Created attachment 154226 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Sukolsak Sakshuwong]

Created attachment 154370 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 154370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154370&action=review

> Source/WebCore/dom/Element.cpp:2211
> +    if (!undoScope() || (isContentEditable() && !isRootEditableElement()))
> +        return 0;

Since we're doing this lazily for -webkit-user-modify, we need to disconnect the undo manager as needed here.
Also, each method of undo manager needs to check whether undo manager had been disconnected or not
upfront and throw & disconnect itself as needed.

> Source/WebCore/dom/Element.cpp:2241
> +        if (node->isContentEditable())
> +            element->disconnectUndoManager();

This is not right. It's possible to have an editable element within a non-editable element within an editable element.
e.g. <div undoscope><span contenteditable=false><span contenteditable=true undoscope>hello</span></span></div>
div.contentEditable=true should NOT disconnect the undoManager on the inner-most span because the inner most element
had always been editable. r- because of this.

[Sukolsak Sakshuwong]

Created attachment 154422 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 154422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154422&action=review

> LayoutTests/editing/undomanager/undoscope-attribute-expected.txt:3
> +PASS Element d has the undoscope attribute set to true. 

Please use more descriptive name than "d".

[Daniel Bates]

Comment on attachment 154422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154422&action=review

I know that this patch has already been reviewed. I noticed some minor notes and had some questions.

> Source/WebCore/WebCore.gypi:2713
> +            'editing/UndoManager.cpp',
> +            'editing/UndoManager.h',

I take it that this change is only applicable to Chromium?

> Source/WebCore/dom/Element.cpp:2205
> +        setAttribute(undoscopeAttr, "");

Would it be sufficient to construct a null AtomicString() (i.e. call the AtomicString() constructor with no arguments) instead of an AtomicString with an empty string?

> Source/WebCore/editing/UndoManager.h:57
> +    inline unsigned long length() const { return m_undoStack.size() + m_redoStack.size(); }

From my understanding, a member function defined within a class definition is implicitly an inline function. So, it's unnecessary to explicitly add the inline keyword to this function signature.

[Daniel Bates]

Comment on attachment 154422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154422&action=review

>> Source/WebCore/dom/Element.cpp:2205
>> +        setAttribute(undoscopeAttr, "");
> 
> Would it be sufficient to construct a null AtomicString() (i.e. call the AtomicString() constructor with no arguments) instead of an AtomicString with an empty string?

Disregard this comment. Passing a null AtomicString would remove the attribute by <http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp?rev=123636#L674>. Can we use emptyAtom?

[Ryosuke Niwa]

Comment on attachment 154422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154422&action=review

Sorry, I wasn't thinking straight when I reviewed this patch. Please add a test cases where you modify content attribute by calling setAttribute. As is, it won't do the right thing.

> Source/WebCore/dom/Element.cpp:2199
> +    return fastHasAttribute(HTMLNames::undoscopeAttr);

Ugh... we shouldn't be storing states in attribute like this. What we need to store it in Element (probably rare data instead).

> Source/WebCore/dom/Element.idl:147
> +        attribute [Conditional=UNDO_MANAGER] boolean undoScope;

We should have [Reflect] on this.

[Sukolsak Sakshuwong]

Created attachment 154527 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 154527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154527&action=review

> Source/WebCore/dom/Element.cpp:2215
> +    if (!undoScope() || (isContentEditable() && !isRootEditableElement())) {
> +        disconnectUndoManager();
> +        return 0;
> +    }

We need to do this on every method of undo manager since scripts can keep a reference to it.

> Source/WebCore/dom/Element.cpp:2236
> +void Element::disconnectEditableDescendentsUndoManagers()

I would prefer calling it disconnectUndoManagersInSubtree.

> Source/WebCore/editing/UndoManager.h:57
> +    unsigned long length() const { return m_undoStack.size() + m_redoStack.size(); }

This can just be "unsigned length()".

> LayoutTests/editing/undomanager/undoscope-attribute.html:44
> +    element.removeAttribute('undoscope');

We should explicitly set the attribute first.

> LayoutTests/editing/undomanager/undoscope-attribute.html:59
> +test(function() {
> +    element.setAttribute('undoscope', '');
> +
> +    assert_equals(element.undoManager.toString(), "[object UndoManager]");
> +}, "After element.setAttribute('undoscope', ''), element.undoManager returns an UndoManager object.");
> +
> +test(function() {
> +    element.setAttribute('undoscope', 'undoscope');
> +
> +    assert_equals(element.undoManager.toString(), "[object UndoManager]");
> +}, "After element.setAttribute('undoscope', 'undoscope'), element.undoManager returns an UndoManager object.");

I would like to see test cases where undoScope IDL attribute and undoscope content attributes are set in turn.

[Sukolsak Sakshuwong]

Created attachment 154562 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 154562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154562&action=review

> Source/WebCore/editing/UndoManager.cpp:111
> +    if (!m_undoScopeHost->isElementNode())
> +        return true;

host can't become a non-element so this check is superfluous.

> Source/WebCore/editing/UndoManager.cpp:113
> +    if (!element->undoScope() || (element->isContentEditable() && !element->isRootEditableElement())) {

undoscope attribute can't be changed by CSS so we should ASSERT(element->undoScope()) instead of disconnecting undo manager when undoScope evaluates to true.

[Sukolsak Sakshuwong]

(In reply to comment #28)
> > Source/WebCore/editing/UndoManager.cpp:111
> > +    if (!m_undoScopeHost->isElementNode())
> > +        return true;
> 
> host can't become a non-element so this check is superfluous.

Host can be a Document, which is a non-element.

[Sukolsak Sakshuwong]

Created attachment 154743 [details]
Patch

[Adam Barth]

Comment on attachment 154743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154743&action=review

> Source/WebCore/editing/UndoManager.cpp:111
> +        return true;

Should we add ASSERT(m_undoScopeHost->isDocument()) in this branch?

> Source/WebCore/editing/UndoManager.cpp:115
> +        element->disconnectUndoManager();

It's strange for a function like UndoManager::isConnected to have side-effects.  I might call this function something like disconnectIfNecessary()...

[WebKit Review Bot]

Comment on attachment 154743 [details]
Patch

Clearing flags on attachment: 154743

Committed r123827: <http://trac.webkit.org/changeset/123827>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.