fix(slider): double click on slider handle to reset slider position #3991
Conversation
Tachometer resultsChromeaction-bar permalink
action-menu permalink
combobox permalink
menu permalink
overlay permalink
picker permalink
popover permalink
slider permalink
split-button permalink
tooltip permalink
Firefoxaction-bar permalink
action-menu permalink
combobox permalink
menu permalink
overlay permalink
picker permalink
popover permalink
slider permalink
split-button permalink
tooltip permalink
|
|
Please add tests cases for the various input modalities covered in this change to the PR description. |
|
screen-capture.webm |
Rajdeepc
changed the title
sp-slider handle to S1 design
|
@Westbrook Slider Handle UI is updated according to S1 designs |
Rajdeepc
changed the title
sp-slider handle to S1 design
|
Did you try listen to |
TarunAdobe
force-pushed
the
feature/slider-default
branch
from
3525e25
to
47ca064
Compare
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Yes thats the way to go forward to include on all the input modalities. The code should now listen to a dblClick event instead of handling it with a timer function. Thanks for pointing it out. |
|
looks good to me so far - one question: |
|
Valid question @spdev3000. You are right if the user drags the slider that is contained inside of an overlaid content and then presses |
| */ | ||
| public handleDoubleClick(event: PointerEvent): void { | ||
| const { input } = this.extractDataFromEvent(event); | ||
| if (input.model?.handle.defaultValue) { |
There was a problem hiding this comment.
If the default value in 0 then this test will not work appropriately.
| min="0" | ||
| value=".5" | ||
| step="0.01" | ||
| default-value="0.2" |
There was a problem hiding this comment.
Might be useful to put default-value into the args, but not a blocker.
| @@ -1604,4 +1603,75 @@ describe('Slider', () => { | |||
| await elementUpdated(el); | |||
| expect(el.values).to.deep.equal({ a: 10, b: 20, c: 29 }); | |||
| }); | |||
| it('resets to default value on double click after moving pointer', async () => { | |||
There was a problem hiding this comment.
We should add a test where the Slider is in an Overlay to confirm it does not close the Overlay.
We should also add default-value to one or some of the Stories in Overlay to confirm this functionality there, as well.
There was a problem hiding this comment.
I have added the test (line 1679) as well as the default-value for stories in overlay.
packages/slider/test/slider.test.ts
Outdated
| const handle = el.shadowRoot.querySelector('.handle') as HTMLDivElement; | ||
| el.track.setPointerCapture = (id: number) => (pointerId = id); | ||
| el.track.releasePointerCapture = (id: number) => (pointerId = id); |
There was a problem hiding this comment.
If we use sendMouse() all the way through the test here, instead of synthetically dispatching events. Then we should be able to exclude this part of the test all together. Here we are mimicking the default pointerCapture functionality, which the actual pointer interactions in sendMouse() do not need.
packages/slider/src/Slider.ts
Outdated
|
|
||
| protected override firstUpdated(changes: PropertyValues<this>): void { | ||
| super.firstUpdated(changes); | ||
| } |
There was a problem hiding this comment.
If we're not actually doing work here than empty additions to the call stack are just costing us tiny amounts of performance.
| @@ -442,6 +443,7 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), { | |||
| ['pointerup', 'pointercancel', 'pointerleave'], | |||
| this.handlePointerup, | |||
| ], | |||
| streamOutside: ['dblclick', this.handleDoubleClick], | |||
There was a problem hiding this comment.
Half surprised this works, but it does make a nice even API for this.
| private renderHandle(): TemplateResult { | ||
| if (this.variant === 'tick') { |
| @change=${this.onInputChange} | ||
| @focus=${this.onInputFocus} | ||
| @blur=${this.onInputBlur} | ||
| @keydown=${this.onInputKeydown} | ||
| .model=${model} | ||
| /> | ||
| <p id="sliderDesc" class="visually-hidden"> |
There was a problem hiding this comment.
This should be OK as just display: none as it is describedby content. Otherwise you get multiple stops of the screen reader cursor. This should be confirmed manually after the change, preferably in assistivlabs across more than one screen reader, as also in a unit test using accessibility snapshotting.
Probably OK to make it a span over a p as it's not visible, too.
| if (event.key == 'Escape') { | ||
| const input = event.target as InputWithModel; | ||
| if ( | ||
| input.model.handle?.defaultValue && |
There was a problem hiding this comment.
This will also fail on 0.
| @change=${this.onInputChange} | ||
| @focus=${this.onInputFocus} | ||
| @blur=${this.onInputBlur} | ||
| @keydown=${this.onInputKeydown} | ||
| .model=${model} | ||
| /> | ||
| <p id="sliderDesc" class="visually-hidden"> | ||
| Press escape or double click to reset the slider to its |
There was a problem hiding this comment.
Please add an entry for this content to #1975
There was a problem hiding this comment.
Can we display: none in the CSS rather than inline?
|
As another requirement: the double click to |
Westbrook
changed the title
packages/slider/test/slider.test.ts
Outdated
| // open the overlay | ||
| const trigger = el.querySelector('#trigger') as HTMLButtonElement; | ||
| trigger.click(); |
There was a problem hiding this comment.
Overlay open action should follow the pattern:
const opened = oneEvent(el, 'sp-opened');
trigger.click(); // or similiar
await opened;
Otherwise there's no guarantee the content actually opened and was promoted to the top layer.
| // send escape key again | ||
| await sendKeys({ | ||
| press: 'Escape', | ||
| }); |
There was a problem hiding this comment.
Needs to reverse of the above, e.g. const closed = oneEvent(...);
|
|
||
| if (input.model?.handle.defaultValue !== undefined) { | ||
| input.model.handle.value = input.model.handle.defaultValue; | ||
| this.dispatchChangeEvent(input, input.model.handle); |
There was a problem hiding this comment.
Confirm whether this dispatches an input event with the same data. See the result of clicking a new value in an <input type="range"> as an example: https://codepen.io/Westbrook/pen/ExJjVRZ?editors=1111
There was a problem hiding this comment.
added dispatchInputEvent on both double-click and escape key interactions
There was a problem hiding this comment.
What's a good test to confirm that this happens before we close this up?
There was a problem hiding this comment.
would it make sense if I extend the dispatches input of the animation frame test (line 388) or should I make a separate one?
| input.model.handle.value = input.model.handle.defaultValue; | ||
| this.dispatchChangeEvent(input, input.model.handle); | ||
| this.requestUpdate(); | ||
| event.preventDefault(); |
There was a problem hiding this comment.
This needs to dispatch an input event with the same data. See the result of clicking a new value in an as an example: https://codepen.io/Westbrook/pen/ExJjVRZ?editors=1111
Description
Double click on slider handle to reset slider position
Related issue(s)
Motivation and context
How has this been tested?
Screenshots (if appropriate)
sliderdefault.mov
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.