Implement undoscope attribute specified at http://dvcs.w3.org/hg/undomanager/raw-file/tip/undomanager.html.
Created attachment 146883 [details] Proposed patch Initial patch.
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.
Created attachment 147552 [details] Updated patch
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() ?
(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.
(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.
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.
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.
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.
(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?
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?
(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.
(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.
Created attachment 154214 [details] Patch
Created attachment 154217 [details] Patch
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
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
Created attachment 154370 [details] Patch
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.
Created attachment 154422 [details] Patch
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".
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.
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?
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.
Created attachment 154527 [details] Patch
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.
Created attachment 154562 [details] Patch
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.
(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.
Created attachment 154743 [details] Patch
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()...
Comment on attachment 154743 [details] Patch Clearing flags on attachment: 154743 Committed r123827: <http://trac.webkit.org/changeset/123827>
All reviewed patches have been landed. Closing bug.