-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(slider): double click on slider handle to reset slider position #3991
Changes from 24 commits
2827c3f
15ade1f
01eb233
6205062
15d0170
745e5c0
47ca064
3ce3482
26956b6
4823191
194bba5
d136580
8853675
08d021e
a9e2eac
792484f
ad9dbf0
4f8cb68
7f52af2
949cc01
2c97a76
a63f460
fa57fcc
d1bb967
975bd58
b7e5c09
27ccaa2
c655213
de6b775
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -341,6 +341,22 @@ export class HandleController { | |
|
||
private _activePointerEventData!: DataFromPointerEvent | undefined; | ||
|
||
/** | ||
* @description check for defaultvalue(value) property in sp-slider and reset on double click on sliderHandle | ||
* @param event | ||
*/ | ||
public handleDoubleClick(event: PointerEvent): void { | ||
const input = (event.target as Element).querySelector( | ||
'.input' | ||
) as InputWithModel; | ||
|
||
if (input.model?.handle.defaultValue !== undefined) { | ||
input.model.handle.value = input.model.handle.defaultValue; | ||
this.dispatchChangeEvent(input, input.model.handle); | ||
this.requestUpdate(); | ||
} | ||
} | ||
|
||
public handlePointerdown(event: PointerEvent): void { | ||
const { resolvedInput, model } = this.extractDataFromEvent(event); | ||
if (!model || this.host.disabled || event.button !== 0) { | ||
|
@@ -432,7 +448,21 @@ export class HandleController { | |
this.requestUpdate(); | ||
}; | ||
|
||
private onInputKeydown = (event: Event): void => { | ||
private onInputKeydown = (event: KeyboardEvent): void => { | ||
if (event.key == 'Escape') { | ||
const input = event.target as InputWithModel; | ||
if ( | ||
input.model.handle?.defaultValue !== undefined && | ||
input.model.handle.value !== input.model.handle.defaultValue | ||
) { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
event.stopPropagation(); | ||
} | ||
return; | ||
} | ||
const input = event.target as InputWithModel; | ||
input.model.handle.highlight = true; | ||
this.requestUpdate(); | ||
|
@@ -522,12 +552,17 @@ export class HandleController { | |
aria-label=${ifDefined(model.ariaLabel)} | ||
aria-labelledby=${ariaLabelledBy} | ||
aria-valuetext=${this.formattedValueForHandle(model)} | ||
aria-describedby="slider-description" | ||
@change=${this.onInputChange} | ||
@focus=${this.onInputFocus} | ||
@blur=${this.onInputBlur} | ||
@keydown=${this.onInputKeydown} | ||
.model=${model} | ||
/> | ||
<span id="slider-description"> | ||
Press escape or double click to reset the slider to its | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add an entry for this content to #1975 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done 👍 |
||
default value. | ||
</span> | ||
</div> | ||
`; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -362,7 +362,6 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), { | |
* @description calculates the fill width | ||
* @param fillStartValue | ||
* @param currentValue | ||
* @param cachedValue | ||
* @returns | ||
*/ | ||
private getOffsetWidth( | ||
|
@@ -409,10 +408,12 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), { | |
></div> | ||
`; | ||
} | ||
|
||
private renderHandle(): TemplateResult { | ||
if (this.variant === 'tick') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏼 |
||
return html``; | ||
} | ||
return html` | ||
${this.variant === 'tick' ? html`` : this.handleController.render()} | ||
${this.handleController.render()} | ||
`; | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Half surprised this works, but it does make a nice even API for this. |
||
})} | ||
> | ||
<div id="controls"> | ||
|
@@ -475,6 +477,10 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), { | |
`; | ||
} | ||
|
||
protected handleDoubleClick(event: PointerEvent): void { | ||
this.handleController.handleDoubleClick(event); | ||
} | ||
|
||
protected handlePointerdown(event: PointerEvent): void { | ||
this.handleController.handlePointerdown(event); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,6 +152,26 @@ export const Filled = (args: StoryArgs = {}): TemplateResult => { | |
`; | ||
}; | ||
|
||
export const HasADefaultValue = (args: StoryArgs = {}): TemplateResult => { | ||
return html` | ||
<div style="width: 500px; margin-inline: 20px;"> | ||
<sp-slider | ||
max="1" | ||
min="0" | ||
value=".5" | ||
step="0.01" | ||
default-value="0.2" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be useful to put |
||
@input=${handleEvent(args)} | ||
@change=${handleEvent(args)} | ||
.formatOptions=${{ style: 'percent' }} | ||
...=${spreadProps(args)} | ||
> | ||
double click or press escape key to reset | ||
</sp-slider> | ||
</div> | ||
`; | ||
}; | ||
|
||
export const FillStart = (args: StoryArgs = {}): TemplateResult => { | ||
return html` | ||
<div style="width: 500px; margin-inline: 20px;"> | ||
|
@@ -397,6 +417,37 @@ export const editable = (args: StoryArgs = {}): TemplateResult => { | |
|
||
editable.decorators = [editableDecorator]; | ||
|
||
export const editableWithDefaultValue = ( | ||
args: StoryArgs = {} | ||
): TemplateResult => { | ||
return html` | ||
<div style="width: 500px; margin: 12px 20px;"> | ||
<sp-slider | ||
editable | ||
max="360" | ||
min="0" | ||
value="90" | ||
step="1" | ||
default-value="180" | ||
@input=${handleEvent(args)} | ||
@change=${handleEvent(args)} | ||
.formatOptions=${{ | ||
style: 'unit', | ||
unit: 'degree', | ||
unitDisplay: 'narrow', | ||
}} | ||
...=${spreadProps(args)} | ||
> | ||
Angle | ||
</sp-slider> | ||
</div> | ||
`; | ||
}; | ||
|
||
editableWithDefaultValue.swc_vrt = { | ||
skip: true, | ||
}; | ||
|
||
export const editableDisabled = (args: StoryArgs = {}): TemplateResult => { | ||
return html` | ||
<div style="width: 500px; margin: 12px 20px;"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,10 @@ governing permissions and limitations under the License. | |
|
||
import '@spectrum-web-components/slider/sp-slider.js'; | ||
import '@spectrum-web-components/slider/sp-slider-handle.js'; | ||
import '@spectrum-web-components/button/sp-button.js'; | ||
import '@spectrum-web-components/overlay/sp-overlay.js'; | ||
import '@spectrum-web-components/popover/sp-popover.js'; | ||
import { Overlay } from '@spectrum-web-components/overlay'; | ||
import { Slider, SliderHandle } from '@spectrum-web-components/slider'; | ||
import { tick } from '../stories/slider.stories.js'; | ||
import { | ||
|
@@ -169,7 +173,6 @@ describe('Slider', () => { | |
}) | ||
); | ||
await elementUpdated(el); | ||
|
||
expect(el.dragging, 'it is dragging 1').to.be.true; | ||
expect(pointerId, '2').to.equal(1); | ||
|
||
|
@@ -1604,4 +1607,128 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add a test where the Slider is in an Overlay to confirm it does not close the Overlay. We should also add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added the test (line 1679) as well as the |
||
const el = await fixture<Slider>( | ||
html` | ||
<sp-slider | ||
style="width: 100px" | ||
value="50" | ||
default-value="50" | ||
></sp-slider> | ||
` | ||
); | ||
await elementUpdated(el); | ||
expect(el.value, 'initial').to.equal(50); | ||
|
||
const handle = el.shadowRoot.querySelector('.handle') as HTMLDivElement; | ||
const handleBoundingRect = handle.getBoundingClientRect(); | ||
const position: [number, number] = [ | ||
handleBoundingRect.x + handleBoundingRect.width / 2, | ||
handleBoundingRect.y + handleBoundingRect.height / 2, | ||
]; | ||
await sendMouse({ | ||
steps: [ | ||
{ | ||
type: 'move', | ||
position, | ||
}, | ||
{ | ||
type: 'down', | ||
}, | ||
], | ||
}); | ||
|
||
await elementUpdated(el); | ||
await sendMouse({ | ||
steps: [ | ||
{ | ||
type: 'move', | ||
position: [ | ||
150, | ||
handleBoundingRect.y + handleBoundingRect.height + 100, | ||
], | ||
}, | ||
{ | ||
type: 'up', | ||
}, | ||
], | ||
}); | ||
|
||
await elementUpdated(el); | ||
|
||
// since we've moved the pointer, the new value should be 100 | ||
expect(el.value).to.equal(100); | ||
|
||
handle.dispatchEvent( | ||
new PointerEvent('dblclick', { | ||
clientX: 0, | ||
cancelable: true, | ||
button: 0, | ||
composed: true, | ||
bubbles: true, | ||
}) | ||
); | ||
|
||
await elementUpdated(el); | ||
|
||
expect( | ||
el.value, | ||
'reset to default value on double click after moving pointer' | ||
).to.equal(50); | ||
}); | ||
it('manages escape key interactions correctly in an overlaid context', async () => { | ||
const el = await fixture<HTMLDivElement>( | ||
html` | ||
<div> | ||
<sp-button id="trigger">Overlay Trigger</sp-button> | ||
<sp-overlay trigger="trigger@click" placement="bottom"> | ||
<sp-popover> | ||
<sp-slider | ||
style="width: 100px" | ||
value="70" | ||
default-value="50" | ||
></sp-slider> | ||
</sp-popover> | ||
</sp-overlay> | ||
</div> | ||
` | ||
); | ||
|
||
await elementUpdated(el); | ||
|
||
// open the overlay | ||
const trigger = el.querySelector('#trigger') as HTMLButtonElement; | ||
trigger.click(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overlay open action should follow the pattern:
Otherwise there's no guarantee the content actually opened and was promoted to the top layer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it! |
||
|
||
await elementUpdated(el); | ||
|
||
// current slider value should be 70 | ||
const slider = el.querySelector('sp-slider') as Slider; | ||
expect(slider.value).to.equal(70); | ||
|
||
slider.focus(); | ||
// send escape key | ||
await sendKeys({ | ||
press: 'Escape', | ||
}); | ||
|
||
await elementUpdated(el); | ||
|
||
// now the slider value should be 50 | ||
expect(slider.value).to.equal(50); | ||
|
||
// and the overlay should be in open state | ||
const overlay = el.querySelector('sp-overlay') as Overlay; | ||
expect(overlay.open).to.be.true; | ||
|
||
// send escape key again | ||
await sendKeys({ | ||
press: 'Escape', | ||
}); | ||
Comment on lines
+1758
to
+1761
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs to reverse of the above, e.g. |
||
|
||
await elementUpdated(el); | ||
|
||
// now the overlay should be closed | ||
expect(overlay.open).to.be.false; | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=1111There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
dispatchInputEvent
on both double-click and escape key interactionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a good test to confirm that this happens before we close this up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense if I extend the
dispatches input of the animation frame
test (line 388) or should I make a separate one?