Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(color-area,color-slider): color-area labeling, RTL support, vertical slider orientation #3313 #3315

Merged
merged 3 commits into from
Jul 11, 2023
Merged

fix(color-area,color-slider): color-area labeling, RTL support, vertical slider orientation #3313 #3315

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f1c3b5
fix(color-area,color-slider): color-area labeling, RTL support, verti…
majornista Jun 13, 2023
1f8f2b3
ci: update golden images cache
majornista Jul 10, 2023
950595e
Merge branch 'main' into Issue-3313-ColorArea-a11y
majornista Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ executors:
parameters:
current_golden_images_hash:
type: string
default: 2f06eaa56e1938ea37a5c753193074e1baead124
default: 5f1c3b5d2ecc0c44683517e3148819b6c93b1805
wireit_cache_name:
type: string
default: wireit
Expand Down
177 changes: 131 additions & 46 deletions packages/color-area/src/ColorArea.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ import {
ColorController,
ColorValue,
} from '@spectrum-web-components/reactive-controllers/src/Color.js';
import { LanguageResolutionController } from '@spectrum-web-components/reactive-controllers/src/LanguageResolution.js';
import {
isAndroid,
isIOS,
} from '@spectrum-web-components/shared/src/platform.js';

import styles from './color-area.css.js';

Expand All @@ -44,6 +49,9 @@ export class ColorArea extends SpectrumElement {
return [styles];
}

@property({ type: String, reflect: true })
public override dir!: 'ltr' | 'rtl';

@property({ type: Boolean, reflect: true })
public disabled = false;

Expand All @@ -62,15 +70,17 @@ export class ColorArea extends SpectrumElement {
@query('.handle')
private handle!: ColorHandle;

private languageResolver = new LanguageResolutionController(this);

private colorController = new ColorController(this, {
extractColorFromState: () => ({
h: this.hue,
s: this.x,
v: 1 - this.y,
v: this.y,
}),
applyColorToState: ({ s, v }) => {
this.x = s;
this.y = 1 - v;
this.y = v;
},
});

Expand Down Expand Up @@ -142,7 +152,7 @@ export class ColorArea extends SpectrumElement {
this.requestUpdate('y', oldValue);
}

private _y = 0;
private _y = 1;

@property({ type: Number })
public step = 0.01;
Expand All @@ -157,6 +167,8 @@ export class ColorArea extends SpectrumElement {

private activeKeys = new Set();

private _valueChanged = false;

public override focus(focusOptions: FocusOptions = {}): void {
super.focus(focusOptions);
this.forwardFocus();
Expand All @@ -171,15 +183,18 @@ export class ColorArea extends SpectrumElement {
}
}

private handleFocusin(): void {
private handleFocus(): void {
this.focused = true;
this._valueChanged = false;
}

private handleFocusout(): void {
private handleBlur(): void {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the name change of these functions!

if (this._pointerDown) {
return;
}
this.altered = 0;
this.focused = false;
this._valueChanged = false;
}

private handleKeydown(event: KeyboardEvent): void {
Expand All @@ -188,7 +203,11 @@ export class ColorArea extends SpectrumElement {
this.altered = [event.shiftKey, event.ctrlKey, event.altKey].filter(
(key) => !!key
).length;
const isArrowKey = code.search('Arrow') === 0;
const isArrowKey =
code.search('Arrow') === 0 ||
code.search('Page') === 0 ||
code.search('Home') === 0 ||
code.search('End') === 0;
if (isArrowKey) {
event.preventDefault();
this.activeKeys.add(code);
Expand All @@ -203,16 +222,28 @@ export class ColorArea extends SpectrumElement {
this.activeKeys.forEach((code) => {
switch (code) {
case 'ArrowUp':
deltaY = step * -1;
deltaY = step;
break;
case 'ArrowDown':
deltaY = step * 1;
deltaY = step * -1;
break;
case 'ArrowLeft':
deltaX = step * -1;
deltaX = this.step * (this.isLTR ? -1 : 1);
break;
case 'ArrowRight':
deltaX = step * 1;
deltaX = this.step * (this.isLTR ? 1 : -1);
break;
case 'PageUp':
deltaY = step * 10;
break;
case 'PageDown':
deltaY = step * -10;
break;
case 'Home':
deltaX = step * (this.isLTR ? -10 : 10);
break;
case 'End':
deltaX = step * (this.isLTR ? 10 : -10);
break;
/* c8 ignore next 2 */
default:
Expand All @@ -233,6 +264,7 @@ export class ColorArea extends SpectrumElement {
this.colorController.applyColorFromState();

if (deltaX != 0 || deltaY != 0) {
this._valueChanged = true;
this.dispatchEvent(
new Event('input', {
bubbles: true,
Expand Down Expand Up @@ -296,8 +328,10 @@ export class ColorArea extends SpectrumElement {
private handlePointermove(event: PointerEvent): void {
const [x, y] = this.calculateHandlePosition(event);

this._valueChanged = false;

this.x = x;
this.y = y;
this.y = 1 - y;
this.colorController.applyColorFromState();
this.dispatchEvent(
new Event('input', {
Expand Down Expand Up @@ -355,7 +389,7 @@ export class ColorArea extends SpectrumElement {
Math.min(1, (offsetY - minOffsetY) / height)
);

return [percentX, percentY];
return [this.isLTR ? percentX : 1 - percentX, percentY];
}

private handleAreaPointerdown(event: PointerEvent): void {
Expand All @@ -382,6 +416,32 @@ export class ColorArea extends SpectrumElement {
}
}

const isMobile = isAndroid() || isIOS();
const defaultAriaLabel = 'Color Picker';
const ariaLabel = this.label
? `${this.label} ${defaultAriaLabel}`
: defaultAriaLabel;
const ariaRoleDescription = ifDefined(
isMobile ? undefined : '2d slider'
);

const ariaLabelX = this.labelX;
const ariaLabelY = this.labelY;
const ariaValueX = new Intl.NumberFormat(
this.languageResolver.language,
{
style: 'percent',
unitDisplay: 'narrow',
}
).format(this.x);
const ariaValueY = new Intl.NumberFormat(
this.languageResolver.language,
{
style: 'percent',
unitDisplay: 'narrow',
}
).format(this.y);

return html`
<div
@pointerdown=${this.handleAreaPointerdown}
Expand All @@ -402,8 +462,9 @@ export class ColorArea extends SpectrumElement {
class="handle"
color=${this.colorController.getHslString()}
?disabled=${this.disabled}
style="transform: translate(${this.x * width}px, ${this.y *
height}px);"
style=${`transform: translate(${
(this.isLTR ? this.x : 1 - this.x) * width
}px, ${height - this.y * height}px);`}
${streamingListener({
start: ['pointerdown', this.handlePointerdown],
streamInside: ['pointermove', this.handlePointermove],
Expand All @@ -414,45 +475,69 @@ export class ColorArea extends SpectrumElement {
})}
></sp-color-handle>

<div>
<input
type="range"
class="slider"
name="x"
aria-label=${this.label ?? this.labelX}
min="0"
max="1"
step=${this.step}
tabindex="-1"
.value=${String(this.x)}
@input=${this.handleInput}
@change=${this.handleChange}
/>
</div>
<div>
<input
type="range"
class="slider"
name="y"
aria-label=${this.label ?? this.labelY}
min="0"
max="1"
step=${this.step}
tabindex="-1"
.value=${String(this.y)}
@input=${this.handleInput}
@change=${this.handleChange}
/>
</div>
<fieldset
class="fieldset"
aria-label=${ifDefined(isMobile ? ariaLabel : undefined)}
>
<div role="presentation">
<input
type="range"
class="slider"
name="x"
aria-label=${isMobile ? ariaLabelX : ariaLabel}
aria-roledescription=${ariaRoleDescription}
aria-orientation="horizontal"
aria-valuetext=${isMobile
? ariaValueX
: `${ariaValueX}, ${ariaLabelX}${
this._valueChanged
? ''
: `, ${ariaValueY}, ${ariaLabelY}`
}`}
min="0"
max="1"
step=${this.step}
tabindex="-1"
.value=${String(this.x)}
@input=${this.handleInput}
@change=${this.handleChange}
/>
</div>
<div role="presentation">
<input
type="range"
class="slider"
name="y"
aria-label=${isMobile ? ariaLabelY : ariaLabel}
aria-roledescription=${ariaRoleDescription}
aria-orientation="vertical"
aria-valuetext=${isMobile
? ariaValueY
: `${ariaValueY}, ${ariaLabelY}${
this._valueChanged
? ''
: `, ${ariaValueX}, ${ariaLabelX}`
}`}
orient="vertical"
min="0"
max="1"
step=${this.step}
tabindex="-1"
.value=${String(this.y)}
@input=${this.handleInput}
@change=${this.handleChange}
/>
</div>
</fieldset>
`;
}

protected override firstUpdated(changed: PropertyValues): void {
super.firstUpdated(changed);
this.boundingClientRect = this.getBoundingClientRect();

this.addEventListener('focusin', this.handleFocusin);
this.addEventListener('focusout', this.handleFocusout);
this.addEventListener('focus', this.handleFocus);
this.addEventListener('blur', this.handleBlur);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do worry about the API name change not being consistent with our other components, though. I think this is going in the right direction, but I feel like it would be good for me to confer with the rest of the team.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name for these private methods can remain handleFocusin/handleFocusout, but we should listen for the focus and blur events in order to set the focused state properly.

this.addEventListener('keyup', this.handleKeyup);
this.addEventListener('keydown', this.handleKeydown);
}
Expand Down
18 changes: 18 additions & 0 deletions packages/color-area/src/color-area.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,21 @@ governing permissions and limitations under the License.
width: 100%;
height: 100%;
}

:host([dir='rtl']) .gradient {
transform: scaleX(-1);
}

.slider[orient='vertical'] {
appearance: slider-vertical;
}

.slider:focus {
z-index: 1;
}

.fieldset {
border: 0;
margin: 0;
padding: 0;
}