| Summary: | Removing text node does not remove its associated markers | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||||||
| Component: | DOM | Assignee: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | cdumez, commit-queue, darin, enrica, esprehn+autocc, kangil.han, rniwa, webkit-bug-importer | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Created attachment 245544 [details]
Patch
Comment on attachment 245544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245544&action=review > Source/WebCore/dom/ContainerNodeAlgorithms.h:242 > + if (is<Text>(node)) > + node.document().markers().removeMarkers(&node); This is in a really hot code path; I don’t think we want to do this in this way. Lets find a way that is more efficient. Comment on attachment 245544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245544&action=review >> Source/WebCore/dom/ContainerNodeAlgorithms.h:242 >> + node.document().markers().removeMarkers(&node); > > This is in a really hot code path; I don’t think we want to do this in this way. Lets find a way that is more efficient. Right. I'll try to propose a new approach. Thanks! I was playing with this bug a bit and it turned out that it's reproducible for contenteditable as well, for example:
<script>
div.focus();
document.execCommand("InsertText", false, "welllcome ");
div.innerHTML = ""; // after that we still store markers for "welllcome" in DocumentMakerController
</script>
Since the root cause is the same (we don't remove markers while children are being removed) I'll suggest removing them somewhere inside ContatinerNode::removeChildren().
Created attachment 245704 [details]
test_case_1, input.value = "" does not clear markers
Created attachment 245705 [details]
Patch
Comment on attachment 245705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245705&action=review > Source/WebCore/dom/ContainerNode.cpp:664 > + if (is<Text>(removedChild)) Special-casing Text here seems weird. Also, I am a bit worried about the performance impact of this change. Wouldn't it be possible to override "virtual void removedFrom(ContainerNode& insertionPoint);" in Text class and call removeMarkers() there instead? It would be much nicer if Text handling was in Text class instead of ContainerNode. Comment on attachment 245705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245705&action=review >> Source/WebCore/dom/ContainerNode.cpp:664 >> + if (is<Text>(removedChild)) > > Special-casing Text here seems weird. Also, I am a bit worried about the performance impact of this change. Wouldn't it be possible to override "virtual void removedFrom(ContainerNode& insertionPoint);" in Text class and call removeMarkers() there instead? It would be much nicer if Text handling was in Text class instead of ContainerNode. Moving this to Text::removedFrom(ContainerNode) seems like a good idea. Thanks! Additionally, I'll try to add some async bits to make it non-blocking. Created attachment 245871 [details]
apply Chris' comments + add async bits
Comment on attachment 245871 [details] apply Chris' comments + add async bits View in context: https://bugs.webkit.org/attachment.cgi?id=245871&action=review > Source/WebCore/dom/DocumentMarkerController.h:81 > + void willRemoveAllMarkersFrom(Node*); Why doesn't this take a reference? > Source/WebCore/dom/DocumentMarkerController.h:113 > + void removeMarkersFromRemovedNodeTimerFired(); We usually put private methods before the private data members, not after. > Source/WebCore/dom/Text.cpp:200 > + document().markers().willRemoveAllMarkersFrom(this); I am not familiar with these markers and I cannot cannot if doing things asynchronously makes sense here so I'll let someone else review. However, I'll say that willRemoveAllMarkersFrom() naming is confusing as it doesn't tell me that it actually removes the markers asynchronously. We probably want something like removeMarkersAsync(). > Source/WebCore/dom/Text.cpp:201 > + Node::removedFrom(removalPoint); CharacterData:: as this is the parent class. Even if CharacterData doesn't override this function now, it could later. > Source/WebCore/dom/Text.h:53 > + virtual void removedFrom(ContainerNode& removalPoint) override final; Should probably be protected? (In reply to comment #10) > Comment on attachment 245871 [details] > apply Chris' comments + add async bits > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245871&action=review > > > Source/WebCore/dom/DocumentMarkerController.h:81 > > + void willRemoveAllMarkersFrom(Node*); > > Why doesn't this take a reference? > > > Source/WebCore/dom/DocumentMarkerController.h:113 > > + void removeMarkersFromRemovedNodeTimerFired(); > > We usually put private methods before the private data members, not after. > > > Source/WebCore/dom/Text.cpp:200 > > + document().markers().willRemoveAllMarkersFrom(this); > > I am not familiar with these markers and I cannot cannot if doing things > asynchronously makes sense here so I'll let someone else review. However, > I'll say that willRemoveAllMarkersFrom() naming is confusing as it doesn't > tell me that it actually removes the markers asynchronously. We probably > want something like removeMarkersAsync(). > > > Source/WebCore/dom/Text.cpp:201 > > + Node::removedFrom(removalPoint); > > CharacterData:: as this is the parent class. Even if CharacterData doesn't > override this function now, it could later. > > > Source/WebCore/dom/Text.h:53 > > + virtual void removedFrom(ContainerNode& removalPoint) override final; > > Should probably be protected? Thanks for comprehensive review! I'll take all the comments into consideration. Created attachment 245933 [details]
Apply Chris' comments (round 2) and let removeMarkersAync remove specific DocumentMarkers
I'll let Darin or Ryosuke review as I am not familiar with these document markers. ping reviewers this is a friendly request for review. Thanks Comment on attachment 245933 [details] Apply Chris' comments (round 2) and let removeMarkersAync remove specific DocumentMarkers View in context: https://bugs.webkit.org/attachment.cgi?id=245933&action=review > Source/WebCore/dom/Text.cpp:203 > +void Text::removedFrom(ContainerNode& removalPoint) > +{ > + document().markers().removeMarkersAsync(*this); > + CharacterData::removedFrom(removalPoint); > +} > + I don't think it makes sense to add Text::removedFrom just to remove markers. We should be doing this in either one of Document::nodeWillBeRemoved, Document::nodeChildrenWillBeRemoved, or Document::updateRangesAfterChildrenChanged instead. And I don't think making only this one code path async make sense. If anything, we should check hasMarkers() before calling removeMarkers or make all of Document::textInserted, Document::textRemoved, etc… async as well. Comment on attachment 245933 [details] Apply Chris' comments (round 2) and let removeMarkersAync remove specific DocumentMarkers View in context: https://bugs.webkit.org/attachment.cgi?id=245933&action=review > Source/WebCore/dom/DocumentMarkerController.cpp:474 > + m_removeMarkersTimerData.first = &node; > + m_removeMarkersTimerData.second = markerTypes; I don't think this would work if two different nodes were removed in a single task. Created attachment 246515 [details]
test_case_2, removing text node does not clear markers
I was playing with this bug a bit and found out that we don't remove markers on div.removeChild(div.firstChild);
Comment on attachment 245933 [details]
Apply Chris' comments (round 2) and let removeMarkersAync remove specific DocumentMarkers
Clear review flag due to Ryosuke's review.
Created attachment 246526 [details]
Apply Ryosuke's comments
Comment on attachment 246526 [details] Apply Ryosuke's comments View in context: https://bugs.webkit.org/attachment.cgi?id=246526&action=review > Source/WebCore/dom/Document.cpp:3713 > + if (is<Text>(n)) We should check m_markers->hasMarkers() here as well. (In reply to comment #20) > Comment on attachment 246526 [details] > Apply Ryosuke's comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246526&action=review > > > Source/WebCore/dom/Document.cpp:3713 > > + if (is<Text>(n)) > > We should check m_markers->hasMarkers() here as well. Happy to see r+, thanks. I think we can skip it here as inside removeMarkers() we already check them, https://trac.webkit.org/browser/trunk/Source/WebCore/dom/DocumentMarkerController.cpp#L454 Committed r180139: <http://trac.webkit.org/changeset/180139> |
Clearing text on HTMLTextAreaElement and HTMLInputElement does not remove its previous markers. For example, running the following script two times will cause that Document stores two markers (one for current node and one for previous which was remved!). <script> textArea.value = ""; textArea.focus(); document.execCommand("InsertText", false, "welllcome "); </script> document().markers().showMarkers() 2 nodes have markers: 0x7f31b5122dc0 1:[0:9](0) 0x7f31b5122d70 1:[0:9](0) This happens when text to insert is empty and setInnerTextValue("") calls removeChildren() on the previous node. It does not affect user as the absolete node has been removed and its markers not drawn. However does it make sense to store it until document's detach?