adobe / spectrum-web-components

Spectrum Web Components
https://opensource.adobe.com/spectrum-web-components/
Apache License 2.0
1.29k stars 205 forks source link

[Bug]: [Slider] editable data-binding breaks when a single sp-slider-handle is slotted #1781

Open Westbrook opened 3 years ago

Westbrook commented 3 years ago

Code of conduct

Impacted component(s)

sp-slider, sp-number-field

Expected behavior

Number Field shows the normalized value when normalization API is leveraged. This value needs to be "denormalized" on its way back into the component.

Actual behavior

Internal value instead of visible value in currently being applied.

Screenshots

No response

What browsers are you seeing the problem in?

No response

How can we reproduce this issue?

No response

Sample code that illustrates the problem

No response

Logs taken while reproducing problem

No response

Westbrook commented 3 years ago

Copied from #1782: https://webcomponents.dev/edit/cyjAtZPLG4FTtBW21fhW/src/index.ts

hunterloftis commented 3 years ago

Update: this isn't a normalization issue. It happens whenever an sp-slider-handle is slotted into an editable sp-slider.

Slider itself extends SliderHandle and is used to store handle state whenever one isn't slotted. There are a few codepaths with special cases for handle === host. The Slider's NumberField connects directly to the Slider itself, assuming that it's the primary handle. Whenever that isn't the case, the state between the NumberField, Slider, and actual-primary SliderHandle gets out-of-sync.

I've implemented a preliminary change that fixes the issue, but I'm going to spend a little time looking for a simpler solution that doesn't add yet more state to a component where so many methods already have side-effects.

Longer-term, we may want to consider refactoring Slider into a declarative, hierarchical implementation with one-way binding, so we can minimize and isolate state changes. That would give us the opportunity to merge the "zero handles" and "one handle" special cases into a single codepath, and to get Slider to fulfill the single-responsibility & liskov substitution principles.

hunterloftis commented 3 years ago

/cc @spdev3000 for visibility on the original issue

hunterloftis commented 3 years ago

This is in fact a bug, but I think identifying that the bug is a Slider-as-handle vs SliderHandle-as-handle conflict can provide a straightforward workaround. I've developed two fixes, both of which increase the complexity of this already-complex component. @Westbrook I'd like to discuss whether they are worthwhile additions, or if it might make sense to fix this and several other issues at the same time with a refactor.

Meanwhile, @spdev3000 can you let me know if this workaround will unblock you? An editable slider with normalization can be achieved by treating the <sp-slider> as if it were the <sp-slider-handle>:

<sp-slider
  label="Slider Label"
  max=1
  min=0
  step=0.1
  value=1
  editable
  .normalization=${{
    toNormalized: (value: number) => { return Math.sqrt(value) },
    fromNormalized: (unit: number) => { return unit * unit},
  }}          
  ></sp-slider>
spdev3000 commented 3 years ago

@hunterloftis Yes this seems a great approach, thx for pointing me to this.

Westbrook commented 3 years ago

Woot!

najikahalsema commented 2 years ago

@hunterloftis Would it make sense to start a general slider refactor project?