fix: Call onFinishEditing_ for fields on mobile.

[laurensvalk]

On the desktop, widgetDispose_ will call onFinishEditing_ on close.

This was missing in the mobile counterpart, so any cleanups in onFinishEditing_ would not be called.

Also update the message string comment while we are touching this code.

The basics

• I validated my changes

The details

Resolves

On mobile, any cleanups in onFinishEditing_ would not be called.

It is called in widgetDispose_ but not in showPromptEditor_.

Proposed Changes

Call onFinishEditing_ in showPromptEditor_.

Reason for Changes

Make sure onFinishEditing_ works consistently on mobile and desktop.

Test Coverage

Tested on a playground hosted on desktop, viewed on Android mobile device on local network and verified that onFinishEditing_ was not called before but is now.

Documentation

Updated the docstring for the prompt translation.

Additional Information

For consideration (though possibly not needed): Should showPromptEditor_ share more things with widgetDispose_ like forceRerender and firing events or can we assume that setValue does that?

[cpcallen]

Seems sensible to me, but I'm going to get a colleague who is more familiar with this part of the codebase to have a look before approving.

[cpcallen]

So, after discussing this colleagues, we'd like to suggest using the new intermediate edit events vs change events instead of onFinishEditing_, which was always a bit of a hacky solution and might be a candidate for deprecation—see issue #2496 for some discussion about this, and PR #7151 for the implementation of the new events.

Let us know if you think that approach might meet your needs, as we're not sure we want to add further support fro onFinishEditing_ at the current time.

[laurensvalk]

Thank you, I'll have a look.

It did seem quite natural to have some control over the final value set by the field (i.e. as argued in #2496 (comment))

If I understand correctly, the suggested approach is to handle events at the block level as opposed to at the field level. Does this mean adding an event handler via setOnChange to every block that uses the field?

This could probably work, but having it close to the field seemed logical thus far. On a related note, I was having some difficulty "merging" several onchange handlers, i.e. adding one to a block that already uses one as part of another extension. It's quite possible there's an obvious method that I missed though :)

[BeksOmega]

If I understand correctly, the suggested approach is to handle events at the block level as opposed to at the field level. Does this mean adding an event handler via setOnChange to every block that uses the field?

You could do that! Or you could add one handler at the workspace level that iterates over all of the relevant blocks.

I was having some difficulty "merging" several onchange handlers, i.e. adding one to a block that already uses one as part of another extension. It's quite possible there's an obvious method that I missed though :)

Yeah there's not a good way to do this at the moment :/ we have similar problems with other mixins.

[BeksOmega]

Hello @laurensvalk We discussed this at our team meeting yesterday and decided to accept this change! We still recommend switching over to use the events since assigning to onFinishEditing_ is hacky and not TypeScript compatible. But if this functionality exists we think it makes sense to make it consistent for browsers and mobile =)

Thank you for putting up this change!

[Edit: I'll get this merged after I have a chance to manually test]

[laurensvalk]

Thank you!