Page MenuHomePhabricator

VisualEditor: [Regression wmf5] Have to undo twice to reverse comment creation
Closed, ResolvedPublic

Description

Environment- beta.

1> In VE, insert a comment. Click done.
2> click to undo while the comment is still highlighted. Notice the label on the Comment first changes to read “Comment” from the actual comment text, before removing it. See screencast.


Version: unspecified
Severity: normal

Details

Reference
bz72315

Event Timeline

Confirmed in Beta(test2 and production do not have that issue).

This seems to be because the first undo you make only clears the comment -text- change. The second undo actually removes the comment node itself. This makes sense since you make the comment node and it goes in the document first, then later insert some text into it.
ContextItemWidget#getDescription falls back to the tool's title if the text was falsey (so, an empty string), which is why you see 'Comment' rather than an empty box. This also seems reasonable.

We should probably be removing an empty comment node from the document if it's left there due to undo... Currently this only appears to be done if you have an open comment inspector with no text and then click outside it (see CommentInspector#getTeardownProcess).

(In reply to Alex Monk from comment #3)

This seems to be because the first undo you make only clears the comment
-text- change. The second undo actually removes the comment node itself.
This makes sense since you make the comment node and it goes in the document
first, then later insert some text into it.
ContextItemWidget#getDescription falls back to the tool's title if the text
was falsey (so, an empty string), which is why you see 'Comment' rather than
an empty box. This also seems reasonable.

We should probably be removing an empty comment node from the document if
it's left there due to undo... Currently this only appears to be done if you
have an open comment inspector with no text and then click outside it (see
CommentInspector#getTeardownProcess).

This sounds like it was caused by Ed's "staging empty comment node" change. That code should ensure that the creation of the comment node and setting the value are one entry in the undo stack.

gerritadmin wrote:

Change 173808 had a related patch set uploaded by Esanders:
Make changes to comment node before apply staging stack

https://gerrit.wikimedia.org/r/173808

gerritadmin wrote:

Change 173808 merged by jenkins-bot:
Make changes to comment node before apply staging stack

https://gerrit.wikimedia.org/r/173808