Inconsistent emit of `sl-change` events

[cyantree]

This is a follow up from Discord: https://discord.com/channels/739437527907303535/1020332605599662181/1020666965850869801

Describe the bug

Currently the behaviour of emitting sl-change events in Shoelace components isn't a) consistent and b) imho doesn't adhere to best practices regarding data flow and native form elements

My opinion regarding data flow and native form elements

sl-change events shouldn't be emitted when changing the value programmatically but only as a result of some kind of user interaction with the component. This can be observed with native form elements like <input> or <select> and is also comparable with unidirectional data flow in React with the difference that in React a controlled component wouldn't change its state on its own but only through its props and in html5 form elements it would control its own state by default but allows being used in a controlled way through props and events.

Components

I've looked through some (not all) relevant components and checked the current behaviour of sl-change and sl-input where applicable.

<sl-rating>

https://github.com/shoelace-style/shoelace/blob/next/src/components/rating/rating.ts#L165

Correct: Clicking the stars emits sl-change. Incorrect: Changing the value via .value emits sl-change.

<sl-checkbox>

https://github.com/shoelace-style/shoelace/blob/next/src/components/checkbox/checkbox.ts

Correct: Clicking the element emits sl-change. Correct: Changing the state via .checked doesn't emit sl-change.

<sl-input>

https://github.com/shoelace-style/shoelace/blob/next/src/components/input/input.ts

Correct: Entering some input and blurring the component emits sl-change. Correct: Changing the state via .value doesn't emit sl-change. Incorrect: Changing the state via .setTextRange() emits sl-change. Incorrect: Changing the state via .setTextRange() emits sl-input.

<sl-textarea>

https://github.com/shoelace-style/shoelace/blob/next/src/components/textarea/textarea.ts

Correct: Entering some input and blurring the component emits sl-change. Correct: Changing the state via .value doesn't emit sl-change. Hidden bug: Changing the state via .setTextRange() might also emit sl-change however because of twice the same if condition only the first one gets executed and the second one should always get skipped and therefore sl-change never emitted. Incorrect: Changing the state via .setTextRange() emits sl-input.

<sl-range>

https://github.com/shoelace-style/shoelace/blob/next/src/components/range/range.ts

Correct: Changing the value through drag and drop emits sl-change. Correct: Changing the state via .value doesn't emit sl-change.

Besides: One could argue that while doing drag and drop only sl-input should be emitted as it is an ongoing input process. And only after ending the drag drop sl-change should be emitted. That would be comparable to input elements where every key fires sl-input but only blurring the input results in sl-change being emitted.

<sl-radio-group>

https://github.com/shoelace-style/shoelace/blob/next/src/components/radio-group/radio-group.ts

Correct: Clicking an radio button emits sl-change. Incorrect: Changing the value via .value emits sl-change.

To Reproduce

Steps to reproduce the behavior:

1. Add an sl-change event listener to a component.
2. Change the value via ui and check whether the listener gets triggered.
3. Change the value programmatically (via dev tools) and check whether the listener gets triggered.

[cyantree]

Related to #264

[Cbrad24]

Experienced the same issue with <sl-select>, from my anonymous Laravel Blade component I used in a Livewire form:

views\components\shoelace\select.blade.php

@props(['help' => null, 'disabled' => false])

@php
    $model = $attributes->whereStartsWith('wire:model')->first();

    if (isset($model)) {
        $attributes = $attributes->merge(['x-data' => '', 'x-on:sl-change' => '$wire.'.$model.' = $el.value']);
    }

    if (! isset($model) and $attributes->has('name')) {
        $model = $attributes['name'];
    }

    $errored = (isset($model) and $errors->has($model));
@endphp

<sl-select {{ $attributes->class(['invalid' => $errored])->merge(['size' => 'medium']) }} @disabled($disabled)>
    {{ $slot }}
    @if($help !== null or $errored)
        <span slot="help-text">{{ $errored ? $errors->first($model) : $help }}</span>
    @endif
</sl-select>

First issue I had to solve was that wire:model was not binding two way. Livewire was setting the value, but not receiving updates to the component. No problem, thought I'd sprinkle some AlpineJS as a small little hack to get it updating: x-on:sl-change="$wire.{{ $model }} = $el.value"

What I noticed was the inconsistencies mentioned above. When I, the end user, changed the dropdown, everything worked fine. But I quickly noticed if Livewire had to set the value because it changed elsewhere, e.g. on the server, it would change it, sl-change would fire again, and Livewire would have to process the update again, resulting in two round trips to the server rather then just one.

Didn't really have a solution to this problem since I don't really have an easy way to differentiate if the update was caused by the user or by Livewire programmatically changing the value.

[claviska]

Thanks for the report. I will be auditing all components soon to establish a policy on when and how events are emitted and adding tests to ensure they are emitted consistently. Thanks for your patience!

[claviska]

Thanks again for reporting this. All components that emit sl-change have been audited and the behaviors have been corrected in c6df057. I added tests for each to prevent regressions, as well as a note on the contribution page indicating that change events should not be emitted when programmatic changes occur.

[cyantree]

Thanks for resolving this!