Rich Text Editor | Toggle buttons should be able to handle their own interactions like click, keydown

[vivinkrishna-ni]

🧹 Tech Debt

Currently, the toggle buttons employed in the nimble-rich-text-editor toolbar use the click and keydown events for toggling text formats within the editor, as well as for managing the active state of the buttons. Instead, it is recommended to adopt the change event to toggle text formats. By doing so, the responsibility for these interactions will shift from the rich text editor to the toggle button itself.

Refer to this conversation for more details: https://github.com/ni/nimble/pull/1416#discussion_r1296023449

[saikrishnan-ni]

@jattasNI, @rajsite, @mollykreis

change event shows the below behavior

• change event gets triggered even:
  • When moving cursor to any place where styling is different, (this should enable or disable a style button based on cursor's position and this emits change event for a disabled style)
  • When we change the list type from one to other (eg, changing from bullet to ordered makes the bulleted list unhighlighted and also triggers change event)

With the above behaviors while using the @change event - we're not able to distinguish if the change event was emitted from user interaction or it gets fired from toggle button programmatically. This makes us to use @click and @keydown events again to capture user interactions. Looks like this is a special case of toggle buttons getting toggled automatically for formatting options as we traverse through the rich text editor.

Having said this, we're updating the toggle-button's checked property for updating the editor buttons state and this triggers the change event. Let us know if this is not the intended way and if there's another way to update the button's checked state without emitting change event.

It would also be helpful if there's a way to send any custom properties while the change event is being triggered from programmatically updating the checked property by this we will let us distinguish the event triggered from user interaction.

[jattasNI]

I would consider it a bug that the toggle button is firing a change event even when the checked state is changed programmatically. It looks like this is also an issue with the switch and checkbox. All of these come from the FAST library and they have also acknowledged that this is a bug but no one has had a chance to deliver a fix yet: https://github.com/microsoft/fast/issues/5750 (we've even participated in that issue in the past).

I'm not sure what a good next step is. Some options:

1. Contribute a fix to that FAST bug.
2. Fork our components from FAST's and fix the bug in our fork.
3. Look for workarounds in the rich text editor component. For example, it could keep track of whether it's in the process of updating button state programmatically and it could ignore change events that are fired while that is happening.
4. Mark this issue as blocked until the linked FAST issue is fixed.

We've done both 1 and 2 for previous bug fixes, but they are a fair amount of work (probably at least a week for someone who hasn't done it before). Option 3 could be quicker but might result in ugly state management code (but that code may be less ugly than the workarounds we already have); that assumes we can find a workaround that even succeeds. Option 4 is a sad outcome but maybe not terrible.

I'm curious for @rajsite and @mollykreis opinions or other ideas. I think my vote given your team's experience level would be to try 3 if we can but fall back to 4 if we can't find an answer we're happy with.

[saikrishnan-ni]

We could also look into passing some custom property on the change event while setting the checked value programmatically if that's possible @rajsite , @mollykreis .

With regards to 3rd option, we will have some more additional state management code which will be replaced eventually when toggle button fixes this issue. So will wait for any further inputs from Milan and Molly to check if there are other approaches as well to fine tune this.

Otherwise, unfortunately we have to settle down on 4th option until we have this fixed considering our other business delivery priorities😔.

[mollykreis]

I don't see a good way to pass a custom property on the change event while setting the checked value programmatically. The code that fires that event is pretty deep within the FAST code (i.e. in their form-associated base class), so I'm not sure how you'd go about identifying the cases where the event should include that custom property and then putting that in the event.

We had to use an approach similar to 3 to get row selection checkboxes to work correctly in the nimble-table because we faced similar issues.

[jattasNI]

Marking as blocked on https://github.com/microsoft/fast/issues/5750. If someone comes up with a solution that doesn't require that, or if that gets fixed by FAST or us, we can mark it unblocked.

[jattasNI]

Removed the blocked label because we could work on this by forking the template. We can prioritize doing that vs other tech debt.