Page MenuHomePhabricator

Ensure decline-events from ToneCheck and from the Improve Tone Structured Task can be distinguished
Closed, ResolvedPublic

Description

As part of the Improve Tone Structured Task, the newcomer will be shown the Tone Edit Check UI. That UI features a decline action:

image.png (318×228 px, 15 KB)

That action presents the user with a short survey when taken:

image.png (325×272 px, 15 KB)

It would be very valuable if we were able to distinguish which responses to that survey came from an EditCheck that was shown due to the Newcomer writing a paragraph with a tone issue, and which responses came from an EditCheck UI that was shown due to the Newcomer following a suggested edit from the Homepage.

NOTE: the Editing Team will complete work related to this ticket by EOD Wednesday, 24 Sep 2025

Story

As an engineer seeking to implement the Revise Tone UX, I need to know when someone who was shown the Tone Check decline survey after following a suggested edit from the Homepage completes said survey so that I can ensure they see the post-survey success message and in-context edit suggestion module. Pictured below and viewable in Figma.

image.png (3,504×1,626 px, 1 MB)

Requirements

Growth Team to populate

  • Growth needs a hook to listen to for this event (postEdit is probably not right, because no edit took place? But that's the call of the editing team.)
  • Ideally, that hook should include the plugin data (ve.init.target.saveFields.plugins)
  • It should include that the Tone Edit Check was declined
  • It should include with what reason the Tone Edit Check was declined (With "tone is appropriate" we will remove the suggestion; with "don't know how to revise" and "other" we probably won't, but might track it differently.)

(Open to discuss other approaches. Many roads lead to Rome.)

Event Timeline

Regarding the requirements:

Currently, we're adding plugin-data to the VE save-fields to identify what task we're dealing with:
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/GrowthExperiments/+/refs/heads/master/modules/ext.growthExperiments.SuggestedEditSession/index.js#731

And then we're listening to postEdit and postEditMobile hooks to show the dialog: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/GrowthExperiments/+/refs/heads/master/modules/ext.growthExperiments.SuggestedEditSession/index.js#565

That means:

  • We need a hook to listen to for this event (postEdit is probably not right, because no edit took place? But that's the call of the editing team.)
  • Ideally, that hook should include the plugin data (ve.init.target.saveFields.plugins)
  • It should include that the Tone Edit Check was declined
  • It should include with what reason the edit check was declined (With "tone is appropriate" we will remove the suggestion, With "don't know how to revise" and "other" we probably won't.)

Does that reasoning make sense? I'll add it to the AC, but happy to discuss more about it!

Simplest thing is probably for you to use ve.trackSubscribe('activity.editCheck-tone, ...) to hook into the existing logging that's tracking the responses for us. If you do your handler will be given a bunch of events related to the tone check, but specifically if you watched out for the ones with an action like this:

{
    "action": "edit-check-feedback-reason-uncertain",
    "feature": "editCheck-tone"
}

Simplest thing is probably for you to use ve.trackSubscribe('activity.editCheck-tone, ...) to hook into the existing logging that's tracking the responses for us. If you do your handler will be given a bunch of events related to the tone check, but specifically if you watched out for the ones with an action like this:

{
    "action": "edit-check-feedback-reason-uncertain",
    "feature": "editCheck-tone"
}

Thank you, that makes sense. I guess we can set the listeners for that when we initialize the Revise Tone VE session. And then when we get that event, we can listen to ve.deactivateComplete to know when VE has finished its teardown process and we can take over again. Though I assume Editing is still looking into making the actual tearing down of the editor happen after the Homepage-initiated edit-check was declined.

Some further points here following our team discussion:

  • We confirm that the ve.trackSubscribe('activity.editCheck-tone', ...) mechanism outlined above is our recommendation for the time being. In the long term, we intend that there be a more intentional mechanism for being notified of a declined action.
  • Our recommendation is that you can call ve.init.target.tryTeardown() from within your listener to tear down the VisualEditor session, and then proceed with further UX actions when the teardown process is complete. That method returns a promise that resolves when teardown is complete or rejects if teardown is cancelled — or you can listen to ve.deactivationComplete as you suggested, which will also fire if the user ends the edit session in other ways (e.g. cancelling).
  • Note that some inherent complexities exist in the workflow. A user could make some edits before declining the edit action — and those edits may or may not lie within the paragraph related to the tone suggestion. In either case, calling tryTeardown will prompt the user to save. We propose that, in either case, you would not want to tear down the surface and discard the user's work without prompting. But if you really wanted to suppress that prompt, you could pass a true argument to tryTeardown to force this.
  • Another option is you can detect whether the user has made, and then vary the UX depending on that. The boolean property ve.init.target.edited reflects whether edits have been made.

Some further points here following our team discussion:

  • We confirm that the ve.trackSubscribe('activity.editCheck-tone', ...) mechanism outlined above is our recommendation for the time being. In the long term, we intend that there be a more intentional mechanism for being notified of a declined action.
  • Our recommendation is that you can call ve.init.target.tryTeardown() from within your listener to tear down the VisualEditor session, and then proceed with further UX actions when the teardown process is complete. That method returns a promise that resolves when teardown is complete or rejects if teardown is cancelled — or you can listen to ve.deactivationComplete as you suggested, which will also fire if the user ends the edit session in other ways (e.g. cancelling).
  • Note that some inherent complexities exist in the workflow. A user could make some edits before declining the edit action — and those edits may or may not lie within the paragraph related to the tone suggestion. In either case, calling tryTeardown will prompt the user to save. We propose that, in either case, you would not want to tear down the surface and discard the user's work without prompting. But if you really wanted to suppress that prompt, you could pass a true argument to tryTeardown to force this.
  • Another option is you can detect whether the user has made, and then vary the UX depending on that. The boolean property ve.init.target.edited reflects whether edits have been made.

Thank you! I think this should allow us to do what is needed. I will look into it and create a proof of concept to see if any functionality is missing.

@Michael: to what extent does Growth have what they need from Editing?

...I ask this wondering it would be appropriate for the Editing Team to consider work on this task as Done?

@Michael: to what extent does Growth have what they need from Editing?

...I ask this wondering it would be appropriate for the Editing Team to consider work on this task as Done?

Almost.

Some further points here following our team discussion:

[...]

  • Another option is you can detect whether the user has made, and then vary the UX depending on that. The boolean property ve.init.target.edited reflects whether edits have been made.

That unfortunately does not work, because your change to allow showing the Tone EditCheck, Allow triggering checks on a specific ContentBranchNode, is actually fake-modifying the content internally. So, ve.init.target.edited is always true for us. Is there another way how we could figure out if the content was actually modified?

You could check the actual history-length, I suppose. In the patch as-currently-written, ve.init.target.surface.model.documentModel.getCompleteHistoryLength() will be 1 until an edit of some sort is made. (Which is also what it'd be without the patch, because there's always an initial transaction that says the entire document-content has been retained. The patch is just replacing that normal stack with its slightly-mangled history.)

You could check the actual history-length, I suppose. In the patch as-currently-written, ve.init.target.surface.model.documentModel.getCompleteHistoryLength() will be 1 until an edit of some sort is made. (Which is also what it'd be without the patch, because there's always an initial transaction that says the entire document-content has been retained. The patch is just replacing that normal stack with its slightly-mangled history.)

Thanks, I'll try that!

However, I discovered another problem: the tryTeardown method on mobile is not returning a Promise like the method on ArticleTarget that it overrides (LSP violation):

ve.init.mw.MobileArticleTarget.js
ve.init.mw.MobileArticleTarget.prototype.tryTeardown = function () {
	this.overlay.onExitClick( $.Event() );
};

Could this be fixed somehow?

You could check the actual history-length, I suppose. In the patch as-currently-written, ve.init.target.surface.model.documentModel.getCompleteHistoryLength() will be 1 until an edit of some sort is made. (Which is also what it'd be without the patch, because there's always an initial transaction that says the entire document-content has been retained. The patch is just replacing that normal stack with its slightly-mangled history.)

Thanks, I'll try that!

However, I discovered another problem: the tryTeardown method on mobile is not returning a Promise like the method on ArticleTarget that it overrides (LSP violation):

ve.init.mw.MobileArticleTarget.js
ve.init.mw.MobileArticleTarget.prototype.tryTeardown = function () {
	this.overlay.onExitClick( $.Event() );
};

Could this be fixed somehow?

I created a new task for this: T406738: Error when closing VE on mobile by declining a Revise Tone check.

@ppelberg: Given that the above issue has now its own task, I think we can resolve this one.