From dfcf104dc28f4564b1c1e8bf0acbef573405ec8b Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 15 Dec 2020 20:26:08 +0200 Subject: [PATCH 01/20] slider commit --- packages/base/src/Keys.js | 19 +++ packages/main/src/Slider.hbs | 2 +- packages/main/src/Slider.js | 69 +++++++- packages/main/src/SliderBase.hbs | 4 +- packages/main/src/SliderBase.js | 152 +++++++++++++++++- packages/main/src/themes/SliderBase.css | 7 +- .../src/themes/base/SliderBase-parameters.css | 2 + packages/main/test/specs/Slider.spec.js | 5 + 8 files changed, 249 insertions(+), 11 deletions(-) diff --git a/packages/base/src/Keys.js b/packages/base/src/Keys.js index 09d3652ebce2..f1addf9f7e28 100644 --- a/packages/base/src/Keys.js +++ b/packages/base/src/Keys.js @@ -115,6 +115,14 @@ const isUp = event => (event.key ? (event.key === "ArrowUp" || event.key === "Up const isDown = event => (event.key ? (event.key === "ArrowDown" || event.key === "Down") : event.keyCode === KeyCodes.ARROW_DOWN) && !hasModifierKeys(event); +const isLeftCtrl = event => (event.key ? (event.key === "ArrowLeft" || event.key === "Left") : event.keyCode === KeyCodes.ARROW_LEFT) && checkModifierKeys(event, true, false, false); + +const isRightCtrl = event => (event.key ? (event.key === "ArrowRight" || event.key === "Right") : event.keyCode === KeyCodes.ARROW_RIGHT) && checkModifierKeys(event, true, false, false); + +const isUpCtrl = event => (event.key ? (event.key === "ArrowUp" || event.key === "Up") : event.keyCode === KeyCodes.ARROW_UP) && checkModifierKeys(event, true, false, false); + +const isDownCtrl = event => (event.key ? (event.key === "ArrowDown" || event.key === "Down") : event.keyCode === KeyCodes.ARROW_DOWN) && checkModifierKeys(event, true, false, false); + const isHome = event => (event.key ? event.key === "Home" : event.keyCode === KeyCodes.HOME) && !hasModifierKeys(event); const isEnd = event => (event.key ? event.key === "End" : event.keyCode === KeyCodes.END) && !hasModifierKeys(event); @@ -145,6 +153,10 @@ const isPageUpShiftCtrl = event => (event.key ? event.key === "PageUp" : event.k const isPageDownShiftCtrl = event => (event.key ? event.key === "PageDown" : event.keyCode === KeyCodes.PAGE_DOWN) && checkModifierKeys(event, true, false, true); +const isPlus = event => ((event.key ? event.key === "=" : event.keyCode === KeyCodes.PLUS) && checkModifierKeys(event, true, false, false)) || (event.keyCode === KeyCodes.NUMPAD_PLUS); + +const isMinus = event => ((event.key ? event.key === "-" : event.keyCode === KeyCodes.MINUS) && !hasModifierKeys(event)) || (event.keyCode === KeyCodes.NUMPAD_MINUS); + const isShow = event => { if (event.key) { return isF4(event) || isShowByArrows(event); @@ -176,8 +188,14 @@ export { isRight, isUp, isDown, + isLeftCtrl, + isRightCtrl, + isUpCtrl, + isDownCtrl, isHome, isEnd, + isPlus, + isMinus, isHomeCtrl, isEndCtrl, isEscape, @@ -194,4 +212,5 @@ export { isPageDownShift, isPageUpShiftCtrl, isPageDownShiftCtrl, + getCtrlKey, }; diff --git a/packages/main/src/Slider.hbs b/packages/main/src/Slider.hbs index eea8490a5ea0..4b6116e266a5 100644 --- a/packages/main/src/Slider.hbs +++ b/packages/main/src/Slider.hbs @@ -1,7 +1,7 @@ {{>include "./SliderBase.hbs"}} {{#*inline "handles"}} -
+
{{#if showTooltip}}
{{tooltipValue}} diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index 9d722e1c7c63..b00049cbfce3 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -4,6 +4,7 @@ import SliderBase from "./SliderBase.js"; // Template import SliderTemplate from "./generated/templates/SliderTemplate.lit.js"; +import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; /** * @public @@ -81,10 +82,15 @@ class Slider extends SliderBase { constructor() { super(); - this._stateStorage.value = null; + this._stateStorage.value = null; + this._setInitialValue("value", null) this.i18nBundle = getI18nBundle("@ui5/webcomponents"); } + onEnterDOM() { + this._sliderHandle = this.shadowRoot.querySelector(".ui5-slider-handle"); + } + /** * * Check if the previously saved state is outdated. That would mean @@ -106,6 +112,10 @@ class Slider extends SliderBase { this._updateHandleAndProgress(this.value); } + _onkeydown(event) { + this._onKeyDownBase(event, "value"); + } + /** * Called when the user starts interacting with the slider * @@ -119,7 +129,13 @@ class Slider extends SliderBase { } const newValue = this.handleDownBase(event); - this._valueOnInteractionStart = this.value; + this._valueOnInteractionStart = this.value; + + // Set initial value if one is not set previously on focus in. + // It will be restored if ESC key is pressed. + if (this._getInitialValue("value") === null) { + this._setInitialValue("value", this.value); + } // Do not yet update the Slider if press is over a handle. It will be updated if the user drags the mouse. if (!this._isHandlePressed(this.constructor.getPageXValueFromEvent(event))) { @@ -128,6 +144,35 @@ class Slider extends SliderBase { } } + _focusInnerElement() { + this._sliderHandle.focus(); + } + + _onfocusin(event) { + // Set initial value if one is not set previously on focus in. + // It will be restored if ESC key is pressed. + if (this._getInitialValue("value") === null) { + this._setInitialValue("value", this.value); + } + + this.focused = true; + } + + _onfocusout(event) { + // Prevent focusout when the focus is getting initially set within the slider before the + // slider customElement itself is finished focusing. + if (this._isFocusing()) { + this._preventFocusOut(); + return; + } + + // Reset focus state and the stored Slider's initial + // value that was saved when it was first focused in + this.focused = false; + this._setInitialValue("value", null) + } + + /** * Called when the user moves the slider * @@ -166,9 +211,7 @@ class Slider extends SliderBase { * @private */ _isHandlePressed(clientX) { - const sliderHandle = this.shadowRoot.querySelector(".ui5-slider-handle"); - const sliderHandleDomRect = sliderHandle.getBoundingClientRect(); - + const sliderHandleDomRect = this._sliderHandle.getBoundingClientRect(); return clientX >= sliderHandleDomRect.left && clientX <= sliderHandleDomRect.right; } @@ -187,6 +230,18 @@ class Slider extends SliderBase { this._handlePositionFromStart = this._progressPercentage * 100; } + _handleActionKeyPress(event) { + const min = this._effectiveMin; + const max = this._effectiveMax; + const currentValue = this.value; + const newValue = isEscape(event) ? this._getInitialValue("value") : this.constructor.clipValue(SliderBase.prototype._handleActionKeyPress.call(this, event, "value") + currentValue, min, max); + + if (newValue !== currentValue) { + this._updateHandleAndProgress(newValue); + this.updateValue("value", newValue); + } + } + get styles() { return { progress: { @@ -221,6 +276,10 @@ class Slider extends SliderBase { return this.value.toFixed(stepPrecision); } + get tabIndexProgress() { + return "-1"; + } + static async onDefine() { await fetchI18nBundle("@ui5/webcomponents"); } diff --git a/packages/main/src/SliderBase.hbs b/packages/main/src/SliderBase.hbs index ed6dd2eff017..0e9e25b15938 100644 --- a/packages/main/src/SliderBase.hbs +++ b/packages/main/src/SliderBase.hbs @@ -4,6 +4,8 @@ @touchstart="{{_ontouchstart}}" @mouseover="{{_onmouseover}}" @mouseout="{{_onmouseout}}" + @keydown="{{_onkeydown}}" + @keyup="{{_onkeyup}}" dir="{{effectiveDir}}" >
@@ -21,7 +23,7 @@ {{/if}}
-
+
{{> handles}}
diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index e74e64bdfca8..677868729120 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -4,7 +4,7 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import Integer from "@ui5/webcomponents-base/dist/types/Integer.js"; import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import { isPhone } from "@ui5/webcomponents-base/dist/Device.js"; - +import { isEscape, isHome, isEnd, isUp, isDown, isRight, isLeft, isUpCtrl, isDownCtrl, isRightCtrl, isLeftCtrl, isPlus, isMinus, isPageUp, isPageDown, getCtrlKey } from "@ui5/webcomponents-base/dist/Keys.js"; import { getTheme } from "@ui5/webcomponents-base/dist/config/Theme.js"; // Styles @@ -97,6 +97,15 @@ const metadata = { disabled: { type: Boolean, }, + + /** + * Indicates if the elements is on focus + * @private + */ + focused: { + type: Boolean, + }, + /** * @private */ @@ -110,6 +119,11 @@ const metadata = { _hiddenTickmarks: { type: Boolean, }, + _tabIndex: { + type: String, + defaultValue: "0", + noAttribute: true, + }, }, slots: /** @lends sap.ui.webcomponents.main.SliderBase.prototype */ { /** @@ -205,6 +219,26 @@ class SliderBase extends UI5Element { }; } + static get ACTION_KEYS() { + return [ + isLeft, + isRight, + isUp, + isDown, + isLeftCtrl, + isRightCtrl, + isUpCtrl, + isDownCtrl, + isPlus, + isMinus, + isHome, + isEnd, + isPageUp, + isPageDown, + isEscape, + ]; + } + static get MIN_SPACE_BETWEEN_TICKMARKS() { return 8; } @@ -258,6 +292,73 @@ class SliderBase extends UI5Element { } } + _setInitialValue(valueType, value) { + this[`_${valueType}Initial`] = value; + } + + _getInitialValue(valueType) { + return this[`_${valueType}Initial`]; + } + + _onKeyDownBase(event) { + if (this.disabled) { + return; + } + + if (SliderBase._isActionKey(event)) { + event.preventDefault(); + this._isUserInteraction = true; + this._handleActionKeyPress(event); + } + } + + _onkeyup(event) { + if (this.disabled) { + return; + } + + this._isUserInteraction = false; + } + + static _isActionKey(event) { + return this.ACTION_KEYS.some(actionKey => actionKey(event)); + } + + _isFocusing() { + return this._isInProcessOfFocusing; + } + + _setIsFocusing(isInProcessOfFocusing) { + this._isInProcessOfFocusing = isInProcessOfFocusing; + } + + + /* Flag the component that it is currently being in process of focusing in. When the slider is getting focused + we need that focus to be delegated to the Slider's handle and to not stay on the slider's shadow root div, as it + is by default. In theory this can be achieved either if the 'delegatesFocus' attribute of the .attachShadow() + customElement method is set to true or if we forward it manually as part of the component logic. + + As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and + we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be + removed as it will become redundant. + + When we manually set the focus on mouseDown to the first focusable element inside the shadowDom - the slider's handle, + that inside focus and subsquently the shadowRoot.activeElement are set a moment before the global document.activeElement + is set to the customElement (ui5-slider) causing a 'race condition'. + + In order for a element within the shadowRoot to be focused, the global document.activeElement MUST be the parent + customElement of the shadow root, in our case the ui5-slider component. Because of that after our focusin of the handle, + a focusout event fired by the browser immidiatly after, resetting the focus. + + Note: If we set the focus to the handle a bit later in time, for example on a mouseup or click event it will + work fine and we will avoid the described race condition as our customElement will be already finished focusing. + However, that does not work for us as we need the focus to be set to the handle exactly on mousedown, + because of the nature of the component and its available drag interactions.*/ + _preventFocusOut() { + this._focusInnerElement(); + } + + /** * Handle the responsiveness of the Slider's UI elements when resizing * @@ -322,6 +423,8 @@ class SliderBase extends UI5Element { SliderBase.UP_EVENTS.forEach(upEventType => window.addEventListener(upEventType, this._upHandler)); window.addEventListener(this._moveEventType, this._moveHandler); + this._setIsFocusing(true); + this._focusInnerElement(); return newValue; } @@ -341,6 +444,7 @@ class SliderBase extends UI5Element { this._moveEventType = null; this._isUserInteraction = false; + this._setIsFocusing(false); } /** @@ -609,8 +713,42 @@ class SliderBase extends UI5Element { } } - get _labels() { - return this._labelValues || []; + _handleActionKeyPress(event, affectedValue) { + const isDownAction = SliderBase._isDecreaseValueAction(event); + const isUpAction = SliderBase._isIncreaseValueAction(event); + const isBigStep = SliderBase._isBigStepAction(event); + + const currentValue = this[affectedValue]; + const min = this._effectiveMin; + const max = this._effectiveMax; + + // If the action key corresponds to a long step and the slider has more than 10 normal steps, + // make a jump of 1/10th of the Slider's length, otherwise just use the normal step property. + let step = this._effectiveStep; + step = isBigStep && ((max - min) / step > 10) ? (max - min) / 10 : step; + + + if (isEnd(event)) { + return max - currentValue; + } + + if (isHome(event)) { + return (currentValue - min) * - 1; + } + + return isUpAction ? step : step * -1; + } + + static _isDecreaseValueAction(event) { + return isDown(event) || isDownCtrl(event) || isLeft(event) || isLeftCtrl(event) || isMinus(event) || isPageDown(event); + } + + static _isIncreaseValueAction(event) { + return isUp(event) || isUpCtrl(event) || isRight(event) || isRightCtrl(event) || isPlus(event) || isPageUp(event); + } + + static _isBigStepAction(event) { + return isDownCtrl(event) || isUpCtrl(event) || isLeftCtrl(event) || isRightCtrl(event) || isPageUp(event) || isPageDown(event); } /** @@ -644,6 +782,10 @@ class SliderBase extends UI5Element { } } + get _labels() { + return this._labelValues || []; + } + /** * Normalizes a new step property value. * If tickmarks are enabled recreates them according to it. @@ -671,6 +813,10 @@ class SliderBase extends UI5Element { get _effectiveMax() { return Math.max(this.min, this.max); } + + get tabIndex() { + return this.disabled ? "-1" : this._tabIndex; + } } export default SliderBase; diff --git a/packages/main/src/themes/SliderBase.css b/packages/main/src/themes/SliderBase.css index 57cbaec686a0..091fd15382aa 100644 --- a/packages/main/src/themes/SliderBase.css +++ b/packages/main/src/themes/SliderBase.css @@ -20,6 +20,7 @@ box-sizing: border-box; height: 3.3125rem; padding: 1rem 0; + outline: none; touch-action: none; -ms-touch-action: pan-y; } @@ -68,11 +69,15 @@ width: var(--_ui5_slider_handle_width); } +.ui5-slider-handle:focus { + outline: var(--_ui5_slider_handle_outline); + outline-offset: var(--_ui5_slider_handle_outline_offset); +} + .ui5-slider-handle--start, .ui5-slider-handle--end { background: var(--_ui5_range_slider_handle_background); } - [dir="rtl"] .ui5-slider-handle { margin-right: var(--_ui5_slider_handle_margin_left); } diff --git a/packages/main/src/themes/base/SliderBase-parameters.css b/packages/main/src/themes/base/SliderBase-parameters.css index 112d0e7973bc..4f23504852ee 100644 --- a/packages/main/src/themes/base/SliderBase-parameters.css +++ b/packages/main/src/themes/base/SliderBase-parameters.css @@ -14,6 +14,8 @@ --_ui5_slider_handle_margin_left: -0.9725rem; --_ui5_slider_handle_hover_background: var(--sapButton_Hover_Background); --_ui5_slider_handle_hover_border: var(--sapButton_Hover_BorderColor); + --_ui5_slider_handle_outline: 1px dotted var(--sapContent_FocusColor); + --_ui5_slider_handle_outline_offset: 0.075rem; --_ui5_range_slider_handle_hover_background: rgba(var(--sapButton_Background), 0.25); --_ui5_slider_tickmark_color: #89919a; --_ui5_slider_tickmark_top: -0.375rem; diff --git a/packages/main/test/specs/Slider.spec.js b/packages/main/test/specs/Slider.spec.js index 6f38c857badd..a85aecf89719 100644 --- a/packages/main/test/specs/Slider.spec.js +++ b/packages/main/test/specs/Slider.spec.js @@ -185,6 +185,11 @@ describe("Testing events", () => { }); }); +describe("Accessibility: Testing keyboard handling", () => { + it("Tab should focus the slider and move the visible focus outline to the slider's handle", () => { + }); +}); + describe("Testing resize handling and RTL support", () => { it("Testing RTL support", () => { const slider = browser.$("#basic-slider"); From 4bbdbb27e74490eb556800471badad07d51ccb07 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 22 Dec 2020 00:48:21 +0200 Subject: [PATCH 02/20] Refactoring and comments --- packages/main/src/Slider.js | 16 ++--- packages/main/src/SliderBase.js | 114 +++++++++++++++++++++----------- 2 files changed, 83 insertions(+), 47 deletions(-) diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index b00049cbfce3..594296143ac0 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -1,10 +1,10 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import { fetchI18nBundle, getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js"; +import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; import SliderBase from "./SliderBase.js"; // Template import SliderTemplate from "./generated/templates/SliderTemplate.lit.js"; -import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; /** * @public @@ -82,8 +82,8 @@ class Slider extends SliderBase { constructor() { super(); - this._stateStorage.value = null; - this._setInitialValue("value", null) + this._stateStorage.value = null; + this._setInitialValue("value", null); this.i18nBundle = getI18nBundle("@ui5/webcomponents"); } @@ -113,7 +113,7 @@ class Slider extends SliderBase { } _onkeydown(event) { - this._onKeyDownBase(event, "value"); + this._handleKeyDown(event, "value"); } /** @@ -129,7 +129,7 @@ class Slider extends SliderBase { } const newValue = this.handleDownBase(event); - this._valueOnInteractionStart = this.value; + this._valueOnInteractionStart = this.value; // Set initial value if one is not set previously on focus in. // It will be restored if ESC key is pressed. @@ -159,8 +159,8 @@ class Slider extends SliderBase { } _onfocusout(event) { - // Prevent focusout when the focus is getting initially set within the slider before the - // slider customElement itself is finished focusing. + // Prevent focusout when the focus is getting set within the slider internal + // element (on the handle), before the Slider' customElement itself is finished focusing if (this._isFocusing()) { this._preventFocusOut(); return; @@ -169,7 +169,7 @@ class Slider extends SliderBase { // Reset focus state and the stored Slider's initial // value that was saved when it was first focused in this.focused = false; - this._setInitialValue("value", null) + this._setInitialValue("value", null); } diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 677868729120..7a15b9c86f4e 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -4,7 +4,9 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import Integer from "@ui5/webcomponents-base/dist/types/Integer.js"; import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import { isPhone } from "@ui5/webcomponents-base/dist/Device.js"; -import { isEscape, isHome, isEnd, isUp, isDown, isRight, isLeft, isUpCtrl, isDownCtrl, isRightCtrl, isLeftCtrl, isPlus, isMinus, isPageUp, isPageDown, getCtrlKey } from "@ui5/webcomponents-base/dist/Keys.js"; +import { + isEscape, isHome, isEnd, isUp, isDown, isRight, isLeft, isUpCtrl, isDownCtrl, isRightCtrl, isLeftCtrl, isPlus, isMinus, isPageUp, isPageDown, +} from "@ui5/webcomponents-base/dist/Keys.js"; import { getTheme } from "@ui5/webcomponents-base/dist/config/Theme.js"; // Styles @@ -292,21 +294,27 @@ class SliderBase extends UI5Element { } } + /** + * Sets initial value when the component is focused in, can be restored with ESC key + * + * @private + */ _setInitialValue(valueType, value) { this[`_${valueType}Initial`] = value; - } + } _getInitialValue(valueType) { return this[`_${valueType}Initial`]; } - _onKeyDownBase(event) { + _handleKeyDown(event) { if (this.disabled) { return; } if (SliderBase._isActionKey(event)) { event.preventDefault(); + this._isUserInteraction = true; this._handleActionKeyPress(event); } @@ -320,45 +328,53 @@ class SliderBase extends UI5Element { this._isUserInteraction = false; } - static _isActionKey(event) { - return this.ACTION_KEYS.some(actionKey => actionKey(event)); + /** + * Flags if an inner element is currently being focused + * + * @private + */ + _preserveFocus(isFocusing) { + this._isInnerElementFocusing = isFocusing; } + /** + * Return if an inside element within the component is currently being focused + * + * @private + */ _isFocusing() { - return this._isInProcessOfFocusing; + return this._isInnerElementFocusing; } - _setIsFocusing(isInProcessOfFocusing) { - this._isInProcessOfFocusing = isInProcessOfFocusing; - } - - - /* Flag the component that it is currently being in process of focusing in. When the slider is getting focused - we need that focus to be delegated to the Slider's handle and to not stay on the slider's shadow root div, as it - is by default. In theory this can be achieved either if the 'delegatesFocus' attribute of the .attachShadow() - customElement method is set to true or if we forward it manually as part of the component logic. - - As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and - we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be - removed as it will become redundant. - - When we manually set the focus on mouseDown to the first focusable element inside the shadowDom - the slider's handle, - that inside focus and subsquently the shadowRoot.activeElement are set a moment before the global document.activeElement - is set to the customElement (ui5-slider) causing a 'race condition'. - - In order for a element within the shadowRoot to be focused, the global document.activeElement MUST be the parent - customElement of the shadow root, in our case the ui5-slider component. Because of that after our focusin of the handle, - a focusout event fired by the browser immidiatly after, resetting the focus. - - Note: If we set the focus to the handle a bit later in time, for example on a mouseup or click event it will - work fine and we will avoid the described race condition as our customElement will be already finished focusing. - However, that does not work for us as we need the focus to be set to the handle exactly on mousedown, - because of the nature of the component and its available drag interactions.*/ + /** + * Prevent focus out when inner element within the component is currently being in process of focusing in. + * In theory this can be achieved either if the shadow root is focusable and 'delegatesFocus' attribute of + * the .attachShadow() customElement method is set to true, or if we forward it manually. + + * As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and + * we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be + * removed as it will become redundant. + * + * When we manually set the focus on mouseDown to the first focusable element inside the shadowDom, + * that inner focus (shadowRoot.activeElement) is set a moment before the global document.activeElement + * is set to the customElement (ui5-slider) causing a 'race condition'. + * + * In order for a element within the shadowRoot to be focused, the global document.activeElement MUST be the parent + * customElement of the shadow root, in our case the ui5-slider component. Because of that after our focusin of the handle, + * a focusout event fired by the browser immidiatly after, resetting the focus. Focus out must be manually prevented + * in both initial focusing and switching the focus between inner elements of the component cases. + + * Note: If we set the focus to the handle with a timeout or a bit later in time, on a mouseup or click event it will + * work fine and we will avoid the described race condition as our host customElement will be already finished focusing. + * However, that does not work for us as we need the focus to be set to the handle exactly on mousedown, + * because of the nature of the component and its available drag interactions. + * + * @private + */ _preventFocusOut() { this._focusInnerElement(); } - /** * Handle the responsiveness of the Slider's UI elements when resizing * @@ -389,7 +405,6 @@ class SliderBase extends UI5Element { return; } - // Check if there are any overlapping labels. // If so - only the first and the last one should be visible const labelItems = this.shadowRoot.querySelectorAll(".ui5-slider-labels li"); @@ -423,11 +438,24 @@ class SliderBase extends UI5Element { SliderBase.UP_EVENTS.forEach(upEventType => window.addEventListener(upEventType, this._upHandler)); window.addEventListener(this._moveEventType, this._moveHandler); - this._setIsFocusing(true); - this._focusInnerElement(); + this._handleFocusOnMouseDown(event); return newValue; } + /** + * Forward the focus to an inner inner part within the component on press + * + * @private + */ + _handleFocusOnMouseDown(event) { + const focusedElement = this.shadowRoot.activeElement; + + if (!focusedElement || focusedElement !== event.target) { + this._preserveFocus(true); + this._focusInnerElement(); + } + } + /** * Called when the user finish interacting with the slider * Fires an change event indicating a final value change, after user interaction is finished. @@ -444,7 +472,7 @@ class SliderBase extends UI5Element { this._moveEventType = null; this._isUserInteraction = false; - this._setIsFocusing(false); + this._preserveFocus(false); } /** @@ -461,6 +489,15 @@ class SliderBase extends UI5Element { } } + /** + * Goes through the key shortcuts available for the component and returns 'true' if the event is triggered by one. + * + * @private + */ + static _isActionKey(event) { + return this.ACTION_KEYS.some(actionKey => actionKey(event)); + } + /** * Locks the given value between min and max boundaries based on slider properties * @@ -714,7 +751,6 @@ class SliderBase extends UI5Element { } _handleActionKeyPress(event, affectedValue) { - const isDownAction = SliderBase._isDecreaseValueAction(event); const isUpAction = SliderBase._isIncreaseValueAction(event); const isBigStep = SliderBase._isBigStepAction(event); @@ -733,7 +769,7 @@ class SliderBase extends UI5Element { } if (isHome(event)) { - return (currentValue - min) * - 1; + return (currentValue - min) * -1; } return isUpAction ? step : step * -1; From 2ec805c421935f2e6b2fb7daac6c07c4b68171dd Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 22 Dec 2020 07:43:52 +0200 Subject: [PATCH 03/20] feat(ui5-slider)focus and keyboard handling implementation The Slider is now focusable, the element that gets the focus when the component is active is the slider's handle. A new private property 'focused' is added. The full keyboard handling specifications are implemented. --- packages/base/src/Keys.js | 1 - packages/main/src/SliderBase.js | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/base/src/Keys.js b/packages/base/src/Keys.js index f1addf9f7e28..f1a979cb88ea 100644 --- a/packages/base/src/Keys.js +++ b/packages/base/src/Keys.js @@ -212,5 +212,4 @@ export { isPageDownShift, isPageUpShiftCtrl, isPageDownShiftCtrl, - getCtrlKey, }; diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 7a15b9c86f4e..45dbd8d4f134 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -336,7 +336,7 @@ class SliderBase extends UI5Element { _preserveFocus(isFocusing) { this._isInnerElementFocusing = isFocusing; } - + /** * Return if an inside element within the component is currently being focused * @@ -354,7 +354,7 @@ class SliderBase extends UI5Element { * As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and * we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be * removed as it will become redundant. - * + * * When we manually set the focus on mouseDown to the first focusable element inside the shadowDom, * that inner focus (shadowRoot.activeElement) is set a moment before the global document.activeElement * is set to the customElement (ui5-slider) causing a 'race condition'. From a07f2f6ed9400e1e96629c2cc2576a461ab67f8f Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 29 Dec 2020 07:42:06 +0200 Subject: [PATCH 04/20] Add more tests, refactor value and visual UI updating --- packages/main/config/wdio.conf.js | 4 +- packages/main/debug.log | 22 ++++++++ packages/main/src/Slider.js | 6 +-- packages/main/src/SliderBase.js | 4 +- packages/main/test/specs/Slider.spec.js | 69 ++++++++++++++++++++++++- 5 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 packages/main/debug.log diff --git a/packages/main/config/wdio.conf.js b/packages/main/config/wdio.conf.js index ec46d09522f7..b03b44b47dcf 100644 --- a/packages/main/config/wdio.conf.js +++ b/packages/main/config/wdio.conf.js @@ -1 +1,3 @@ -module.exports = require("@ui5/webcomponents-tools/components-package/wdio.js"); +const result = require("@ui5/webcomponents-tools/components-package/wdio.js"); +result.config.capabilities[0]["goog:chromeOptions"].args = ['--disable-gpu']; // From: ['--disable-gpu', '--headless'] +module.exports = result; \ No newline at end of file diff --git a/packages/main/debug.log b/packages/main/debug.log new file mode 100644 index 000000000000..34c6bb8be962 --- /dev/null +++ b/packages/main/debug.log @@ -0,0 +1,22 @@ +[1228/073142.313:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/073142.320:ERROR:exception_snapshot_win.cc(99)] thread ID 18976 not found in process +[1228/073142.333:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/073142.333:ERROR:exception_snapshot_win.cc(99)] thread ID 21920 not found in process +[1228/075036.472:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/075036.474:ERROR:exception_snapshot_win.cc(99)] thread ID 14112 not found in process +[1228/075036.491:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/075036.491:ERROR:exception_snapshot_win.cc(99)] thread ID 10232 not found in process +[1228/080549.353:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/080549.354:ERROR:exception_snapshot_win.cc(99)] thread ID 34800 not found in process +[1228/081841.860:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/081841.862:ERROR:exception_snapshot_win.cc(99)] thread ID 40880 not found in process +[1228/081841.886:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/081841.886:ERROR:exception_snapshot_win.cc(99)] thread ID 27960 not found in process +[1228/085901.519:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/085901.521:ERROR:exception_snapshot_win.cc(99)] thread ID 28088 not found in process +[1228/085901.540:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/085901.540:ERROR:exception_snapshot_win.cc(99)] thread ID 8784 not found in process +[1228/093713.544:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/093713.546:ERROR:exception_snapshot_win.cc(99)] thread ID 18964 not found in process +[1228/093713.567:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) +[1228/093713.568:ERROR:exception_snapshot_win.cc(99)] thread ID 25052 not found in process diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index 594296143ac0..edcde5fec577 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -112,10 +112,6 @@ class Slider extends SliderBase { this._updateHandleAndProgress(this.value); } - _onkeydown(event) { - this._handleKeyDown(event, "value"); - } - /** * Called when the user starts interacting with the slider * @@ -234,7 +230,7 @@ class Slider extends SliderBase { const min = this._effectiveMin; const max = this._effectiveMax; const currentValue = this.value; - const newValue = isEscape(event) ? this._getInitialValue("value") : this.constructor.clipValue(SliderBase.prototype._handleActionKeyPress.call(this, event, "value") + currentValue, min, max); + const newValue = isEscape(event) ? this._getInitialValue("value") : this.constructor.clipValue(this._handleActionKeyPressBase(event, "value") + currentValue, min, max); if (newValue !== currentValue) { this._updateHandleAndProgress(newValue); diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 45dbd8d4f134..6542f63bea2f 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -307,7 +307,7 @@ class SliderBase extends UI5Element { return this[`_${valueType}Initial`]; } - _handleKeyDown(event) { + _onkeydown(event) { if (this.disabled) { return; } @@ -750,7 +750,7 @@ class SliderBase extends UI5Element { } } - _handleActionKeyPress(event, affectedValue) { + _handleActionKeyPressBase(event, affectedValue) { const isUpAction = SliderBase._isIncreaseValueAction(event); const isBigStep = SliderBase._isBigStepAction(event); diff --git a/packages/main/test/specs/Slider.spec.js b/packages/main/test/specs/Slider.spec.js index a85aecf89719..7fb09c0c7e8b 100644 --- a/packages/main/test/specs/Slider.spec.js +++ b/packages/main/test/specs/Slider.spec.js @@ -185,8 +185,75 @@ describe("Testing events", () => { }); }); +describe("Accessibility: Testing focus", () => { + it("Click anywhere in the Slider should focus the Slider's handle and set the 'focused' property to true", () => { + browser.url("http://localhost:8080/test-resources/pages/Slider.html"); + + const slider = browser.$("#basic-slider"); + const sliderHandle = slider.shadow$(".ui5-slider-handle"); + + slider.click(); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-slider").shadowRoot.activeElement; + }); + + assert.strictEqual(slider.isFocused(), true, "Slider component is focused"); + assert.strictEqual(slider.getProperty("focused"), true, "Slider state is focused"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), sliderHandle.getAttribute("class"), "Slider handle has the shadowDom focus"); + }); + + it("Tab should focus the Slider and move the visible focus outline to the slider's handle", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + const sliderHandle = slider.shadow$(".ui5-slider-handle"); + + browser.keys("Tab"); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-slider-with-tooltip").shadowRoot.activeElement; + }); + + assert.strictEqual(slider.isFocused(), true, "Slider component is focused"); + assert.strictEqual(slider.getProperty("focused"), true, "Slider is focused"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), sliderHandle.getAttribute("class"), "Slider handle has the shadowDom focus"); + }); + + it("Shift+Tab should focus the previous Slider and move the visible focus outline to the previous slider's handle", () => { + const slider = browser.$("#basic-slider"); + const sliderHandle = slider.shadow$(".ui5-slider-handle"); + + browser.keys(["Shift", "Tab"]); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-slider").shadowRoot.activeElement; + }); + + assert.strictEqual(slider.isFocused(), true, "Slider component is focused"); + assert.strictEqual(slider.getProperty("focused"), true, "Slider is focused"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), sliderHandle.getAttribute("class"), "Slider handle has the shadowDom focus"); + }); +}); + + describe("Accessibility: Testing keyboard handling", () => { - it("Tab should focus the slider and move the visible focus outline to the slider's handle", () => { + it("Right arrow should increase the value of the slider with a small increment step", () => { + const slider = browser.$("#basic-slider"); + const sliderHandle = slider.shadow$(".ui5-slider-handle"); + + slider.setProperty("value", 0); + browser.keys("ArrowRight"); + + assert.strictEqual(slider.getProperty("value"), 1, "Value is increased"); + }); + + it("Left arrow should decrease the value of the slider with a small increment step", () => { + const slider = browser.$("#basic-slider"); + const sliderHandle = slider.shadow$(".ui5-slider-handle"); + + slider.setProperty("value", 0); + browser.keys("ArrowLeft"); + + assert.strictEqual(slider.getProperty("value"), 0, "Value is increased"); }); }); From a6ced77b234cee933a425c9d32fc1fdeb1855e21 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 29 Dec 2020 09:52:18 +0200 Subject: [PATCH 05/20] Add more keyboard handling tests --- packages/main/config/wdio.conf.js | 4 +- packages/main/src/Slider.js | 2 + packages/main/src/SliderBase.js | 5 -- packages/main/test/pages/Slider.html | 3 + packages/main/test/specs/Slider.spec.js | 85 +++++++++++++++++++++---- 5 files changed, 79 insertions(+), 20 deletions(-) diff --git a/packages/main/config/wdio.conf.js b/packages/main/config/wdio.conf.js index b03b44b47dcf..ec46d09522f7 100644 --- a/packages/main/config/wdio.conf.js +++ b/packages/main/config/wdio.conf.js @@ -1,3 +1 @@ -const result = require("@ui5/webcomponents-tools/components-package/wdio.js"); -result.config.capabilities[0]["goog:chromeOptions"].args = ['--disable-gpu']; // From: ['--disable-gpu', '--headless'] -module.exports = result; \ No newline at end of file +module.exports = require("@ui5/webcomponents-tools/components-package/wdio.js"); diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index edcde5fec577..1abec00d8407 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -1,6 +1,7 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import { fetchI18nBundle, getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js"; import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; +import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import SliderBase from "./SliderBase.js"; // Template @@ -89,6 +90,7 @@ class Slider extends SliderBase { onEnterDOM() { this._sliderHandle = this.shadowRoot.querySelector(".ui5-slider-handle"); + ResizeHandler.register(this, this._resizeHandler); } /** diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 6542f63bea2f..5f5de92834f3 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -2,7 +2,6 @@ import UI5Element from "@ui5/webcomponents-base/dist/UI5Element.js"; import litRender from "@ui5/webcomponents-base/dist/renderer/LitRenderer.js"; import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import Integer from "@ui5/webcomponents-base/dist/types/Integer.js"; -import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import { isPhone } from "@ui5/webcomponents-base/dist/Device.js"; import { isEscape, isHome, isEnd, isUp, isDown, isRight, isLeft, isUpCtrl, isDownCtrl, isRightCtrl, isLeftCtrl, isPlus, isMinus, isPageUp, isPageDown, @@ -253,10 +252,6 @@ class SliderBase extends UI5Element { }; } - onEnterDOM() { - ResizeHandler.register(this, this._resizeHandler); - } - onExitDOM() { ResizeHandler.deregister(this, this._handleResize); } diff --git a/packages/main/test/pages/Slider.html b/packages/main/test/pages/Slider.html index 2c4cf6f33933..d196f489fa92 100644 --- a/packages/main/test/pages/Slider.html +++ b/packages/main/test/pages/Slider.html @@ -65,6 +65,9 @@

Slider with steps, tooltips, tickmarks and label

Slider with float number step (1.25), tooltips, tickmarks and labels between 3 steps (3.75 value)

+ +

Basic RTL Slider

+
diff --git a/packages/main/test/specs/Slider.spec.js b/packages/main/test/specs/Slider.spec.js index 7fb09c0c7e8b..26ecac5a4285 100644 --- a/packages/main/test/specs/Slider.spec.js +++ b/packages/main/test/specs/Slider.spec.js @@ -238,7 +238,6 @@ describe("Accessibility: Testing focus", () => { describe("Accessibility: Testing keyboard handling", () => { it("Right arrow should increase the value of the slider with a small increment step", () => { const slider = browser.$("#basic-slider"); - const sliderHandle = slider.shadow$(".ui5-slider-handle"); slider.setProperty("value", 0); browser.keys("ArrowRight"); @@ -248,30 +247,92 @@ describe("Accessibility: Testing keyboard handling", () => { it("Left arrow should decrease the value of the slider with a small increment step", () => { const slider = browser.$("#basic-slider"); - const sliderHandle = slider.shadow$(".ui5-slider-handle"); - slider.setProperty("value", 0); browser.keys("ArrowLeft"); + assert.strictEqual(slider.getProperty("value"), 0, "Value is decreased"); + }); + + it("Up arrow should increase the value of the slider with a small increment step", () => { + const slider = browser.$("#basic-slider"); + + browser.keys("ArrowUp"); + assert.strictEqual(slider.getProperty("value"), 1, "Value is increased"); + }); + + it("Down arrow should increase the value of the slider with a small increment step", () => { + const slider = browser.$("#basic-slider"); + + browser.keys("ArrowDown"); + assert.strictEqual(slider.getProperty("value"), 0, "Value is decreased"); + }); - assert.strictEqual(slider.getProperty("value"), 0, "Value is increased"); + it("Ctrl + Right arrow should increase the value of the slider with a big increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys("Tab"); + browser.keys(["Control", "ArrowRight"]); + + assert.strictEqual(slider.getProperty("value"), 2, "Value is increased"); + }); + + it("Ctrl + Left arrow should decrease the value of the slider with a big increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys(["Control", "ArrowLeft"]); + assert.strictEqual(slider.getProperty("value"), 0, "Value is decreased"); + }); + + it("Ctrl + Up arrow should increase the value of the slider with a big increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys(["Control", "ArrowUp"]); + assert.strictEqual(slider.getProperty("value"), 2, "Value is increased"); + }); + + it("Ctrl + Down arrow should increase the value of the slider with a big increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys(["Control", "ArrowDown"]); + assert.strictEqual(slider.getProperty("value"), 0, "Value is decreased"); + }); + + it("PageUp should increase the value of the slider with a big increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys("PageUp"); + assert.strictEqual(slider.getProperty("value"), 2, "Value is increased"); + }); + + it("PageDown should increase the value of the slider with a big increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys("PageDown"); + assert.strictEqual(slider.getProperty("value"), 0, "Value is decreased"); + }); + + it("A '+' key press should increase the value of the slider with a small increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys("+"); + assert.strictEqual(slider.getProperty("value"), 1, "Value is increased"); + }); + + it("A '-' key press should increase the value of the slider with a small increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys("-"); + assert.strictEqual(slider.getProperty("value"), 0, "Value is decreased"); }); }); describe("Testing resize handling and RTL support", () => { it("Testing RTL support", () => { - const slider = browser.$("#basic-slider"); + const slider = browser.$("#basic-slider-rtl"); const sliderHandle = slider.shadow$(".ui5-slider-handle"); - slider.setAttribute("dir", "rtl"); - slider.setProperty("min", 0); - slider.setProperty("max", 10); - slider.setProperty("step", 1); - slider.setProperty("value", 0); - assert.strictEqual(sliderHandle.getAttribute("style"), "right: 0%;", "Initially if no value is set, the Slider handle is at the right of the Slider"); slider.setProperty("value", 3); - assert.strictEqual(sliderHandle.getAttribute("style"), "right: 30%;", "Slider handle should be 30% from the right"); slider.click(); From e08aefd0a49d9bd1fcbca56386603b1bd3e12ed0 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 29 Dec 2020 10:50:22 +0200 Subject: [PATCH 06/20] Remove log file, update Keys.js --- packages/base/src/Keys.js | 2 +- packages/main/debug.log | 22 ---------------------- 2 files changed, 1 insertion(+), 23 deletions(-) delete mode 100644 packages/main/debug.log diff --git a/packages/base/src/Keys.js b/packages/base/src/Keys.js index f1a979cb88ea..dfb5ffe51a0f 100644 --- a/packages/base/src/Keys.js +++ b/packages/base/src/Keys.js @@ -153,7 +153,7 @@ const isPageUpShiftCtrl = event => (event.key ? event.key === "PageUp" : event.k const isPageDownShiftCtrl = event => (event.key ? event.key === "PageDown" : event.keyCode === KeyCodes.PAGE_DOWN) && checkModifierKeys(event, true, false, true); -const isPlus = event => ((event.key ? event.key === "=" : event.keyCode === KeyCodes.PLUS) && checkModifierKeys(event, true, false, false)) || (event.keyCode === KeyCodes.NUMPAD_PLUS); +const isPlus = event => (event.keyCode ? event.keyCode === KeyCodes.PLUS : KeyCodes.NUMPAD_PLUS) || (event.key === "=" && checkModifierKeys(event, false, false, true)); const isMinus = event => ((event.key ? event.key === "-" : event.keyCode === KeyCodes.MINUS) && !hasModifierKeys(event)) || (event.keyCode === KeyCodes.NUMPAD_MINUS); diff --git a/packages/main/debug.log b/packages/main/debug.log deleted file mode 100644 index 34c6bb8be962..000000000000 --- a/packages/main/debug.log +++ /dev/null @@ -1,22 +0,0 @@ -[1228/073142.313:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/073142.320:ERROR:exception_snapshot_win.cc(99)] thread ID 18976 not found in process -[1228/073142.333:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/073142.333:ERROR:exception_snapshot_win.cc(99)] thread ID 21920 not found in process -[1228/075036.472:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/075036.474:ERROR:exception_snapshot_win.cc(99)] thread ID 14112 not found in process -[1228/075036.491:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/075036.491:ERROR:exception_snapshot_win.cc(99)] thread ID 10232 not found in process -[1228/080549.353:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/080549.354:ERROR:exception_snapshot_win.cc(99)] thread ID 34800 not found in process -[1228/081841.860:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/081841.862:ERROR:exception_snapshot_win.cc(99)] thread ID 40880 not found in process -[1228/081841.886:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/081841.886:ERROR:exception_snapshot_win.cc(99)] thread ID 27960 not found in process -[1228/085901.519:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/085901.521:ERROR:exception_snapshot_win.cc(99)] thread ID 28088 not found in process -[1228/085901.540:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/085901.540:ERROR:exception_snapshot_win.cc(99)] thread ID 8784 not found in process -[1228/093713.544:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/093713.546:ERROR:exception_snapshot_win.cc(99)] thread ID 18964 not found in process -[1228/093713.567:ERROR:process_reader_win.cc(123)] NtOpenThread: {Access Denied} A process has requested access to an object, but has not been granted those access rights. (0xc0000022) -[1228/093713.568:ERROR:exception_snapshot_win.cc(99)] thread ID 25052 not found in process From dd5cfac5aee1686a794dd55844eba48cfd1d423b Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 29 Dec 2020 16:24:23 +0200 Subject: [PATCH 07/20] Complete the test cases covered --- packages/base/src/Keys.js | 4 +-- packages/main/test/specs/Slider.spec.js | 43 +++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/packages/base/src/Keys.js b/packages/base/src/Keys.js index dfb5ffe51a0f..d574d8213ace 100644 --- a/packages/base/src/Keys.js +++ b/packages/base/src/Keys.js @@ -153,9 +153,9 @@ const isPageUpShiftCtrl = event => (event.key ? event.key === "PageUp" : event.k const isPageDownShiftCtrl = event => (event.key ? event.key === "PageDown" : event.keyCode === KeyCodes.PAGE_DOWN) && checkModifierKeys(event, true, false, true); -const isPlus = event => (event.keyCode ? event.keyCode === KeyCodes.PLUS : KeyCodes.NUMPAD_PLUS) || (event.key === "=" && checkModifierKeys(event, false, false, true)); +const isPlus = event => (event.key ? event.key === "+" : event.keyCode === KeyCodes.PLUS) || (event.keyCode === KeyCodes.NUMPAD_PLUS && !hasModifierKeys(event)); -const isMinus = event => ((event.key ? event.key === "-" : event.keyCode === KeyCodes.MINUS) && !hasModifierKeys(event)) || (event.keyCode === KeyCodes.NUMPAD_MINUS); +const isMinus = event => (event.key ? event.key === "-" : event.keyCode === KeyCodes.MINUS) || (event.keyCode === KeyCodes.NUMPAD_MINUS && !hasModifierKeys(event)); const isShow = event => { if (event.key) { diff --git a/packages/main/test/specs/Slider.spec.js b/packages/main/test/specs/Slider.spec.js index 26ecac5a4285..699a8c24d314 100644 --- a/packages/main/test/specs/Slider.spec.js +++ b/packages/main/test/specs/Slider.spec.js @@ -303,7 +303,7 @@ describe("Accessibility: Testing keyboard handling", () => { assert.strictEqual(slider.getProperty("value"), 2, "Value is increased"); }); - it("PageDown should increase the value of the slider with a big increment step", () => { + it("PageDown should decrease the value of the slider with a big increment step", () => { const slider = browser.$("#basic-slider-with-tooltip"); browser.keys("PageDown"); @@ -317,12 +317,51 @@ describe("Accessibility: Testing keyboard handling", () => { assert.strictEqual(slider.getProperty("value"), 1, "Value is increased"); }); - it("A '-' key press should increase the value of the slider with a small increment step", () => { + it("A '-' key press should decrease the value of the slider with a small increment step", () => { const slider = browser.$("#basic-slider-with-tooltip"); browser.keys("-"); assert.strictEqual(slider.getProperty("value"), 0, "Value is decreased"); }); + + it("A numpad '+' key press should increase the value of the slider with a small increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + const numpadAdd = "\uE025"; + + browser.keys(numpadAdd); + assert.strictEqual(slider.getProperty("value"), 1, "Value is increased"); + }); + + it("A numpad '-' key press should decrease the value of the slider with a small increment step", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + const numpadSubtract = "\uE027"; + + browser.keys(numpadSubtract); + assert.strictEqual(slider.getProperty("value"), 0, "Value is decreased"); + }); + + it("An 'End' key press should increase the value of the slider to its max", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys("End"); + assert.strictEqual(slider.getProperty("value"), 20, "Value is decreased"); + }); + + it("A 'Home' key press should set the value of the slider to its minimum", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + browser.keys("Home"); + assert.strictEqual(slider.getProperty("value"), 0, "Value is increased"); + }); + + it("A 'Esc' key press should return the value of the slider at its initial point at the time of its focusing", () => { + const slider = browser.$("#basic-slider-with-tooltip"); + + slider.setProperty("value", 12); + + browser.keys("Escape"); + assert.strictEqual(slider.getProperty("value"), 0, "Value is increased"); + }); }); describe("Testing resize handling and RTL support", () => { From ac497215128bad985a880e3709ea4ab57b70ae9f Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 29 Dec 2020 16:29:11 +0200 Subject: [PATCH 08/20] Restore ResizeHandler Import in SliderBase --- packages/main/src/SliderBase.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 5f5de92834f3..c1dab1e3dbfc 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -2,6 +2,7 @@ import UI5Element from "@ui5/webcomponents-base/dist/UI5Element.js"; import litRender from "@ui5/webcomponents-base/dist/renderer/LitRenderer.js"; import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import Integer from "@ui5/webcomponents-base/dist/types/Integer.js"; +import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import { isPhone } from "@ui5/webcomponents-base/dist/Device.js"; import { isEscape, isHome, isEnd, isUp, isDown, isRight, isLeft, isUpCtrl, isDownCtrl, isRightCtrl, isLeftCtrl, isPlus, isMinus, isPageUp, isPageDown, From 374142f63fe10e86dd9eb372cc23e64a05ab5f07 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Mon, 4 Jan 2021 07:13:47 +0200 Subject: [PATCH 09/20] Minor template fixes --- packages/main/src/Slider.hbs | 2 +- packages/main/src/SliderBase.hbs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/main/src/Slider.hbs b/packages/main/src/Slider.hbs index 4b6116e266a5..819750256d07 100644 --- a/packages/main/src/Slider.hbs +++ b/packages/main/src/Slider.hbs @@ -1,7 +1,7 @@ {{>include "./SliderBase.hbs"}} {{#*inline "handles"}} -
+
{{#if showTooltip}}
{{tooltipValue}} diff --git a/packages/main/src/SliderBase.hbs b/packages/main/src/SliderBase.hbs index 0e9e25b15938..fa6f00e89de5 100644 --- a/packages/main/src/SliderBase.hbs +++ b/packages/main/src/SliderBase.hbs @@ -23,7 +23,7 @@ {{/if}}
-
+
{{> handles}}
From ac972423b59317b3e8045d3fd41a5b3ad70f514d Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 15 Dec 2020 20:26:08 +0200 Subject: [PATCH 10/20] slider commit --- packages/base/src/Keys.js | 1 + packages/main/src/RangeSlider.hbs | 6 +- packages/main/src/RangeSlider.js | 141 +++++++++++++++++- packages/main/src/Slider.js | 5 + packages/main/src/SliderBase.js | 41 +++++ packages/main/src/themes/SliderBase.css | 14 +- .../src/themes/base/SliderBase-parameters.css | 4 +- 7 files changed, 198 insertions(+), 14 deletions(-) diff --git a/packages/base/src/Keys.js b/packages/base/src/Keys.js index d574d8213ace..433337e78b4b 100644 --- a/packages/base/src/Keys.js +++ b/packages/base/src/Keys.js @@ -212,4 +212,5 @@ export { isPageDownShift, isPageUpShiftCtrl, isPageDownShiftCtrl, + getCtrlKey, }; diff --git a/packages/main/src/RangeSlider.hbs b/packages/main/src/RangeSlider.hbs index 0c01161c6fee..1ed4b1ba6147 100644 --- a/packages/main/src/RangeSlider.hbs +++ b/packages/main/src/RangeSlider.hbs @@ -1,14 +1,16 @@ {{>include "./SliderBase.hbs"}} {{#*inline "handles"}} -
+
{{#if showTooltip}}
{{tooltipStartValue}}
{{/if}}
-
+
{{#if showTooltip}}
{{tooltipEndValue}} diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index c3815f472dc9..b73ea0db699a 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -1,6 +1,7 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import { fetchI18nBundle, getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js"; import SliderBase from "./SliderBase.js"; +import {isEscape, isTabPrevious, isTabNext, isHome, isEnd} from "@ui5/webcomponents-base/dist/Keys.js"; // Template import RangeSliderTemplate from "./generated/templates/RangeSliderTemplate.lit.js"; @@ -99,6 +100,12 @@ class RangeSlider extends SliderBase { this.i18nBundle = getI18nBundle("@ui5/webcomponents"); } + onEnterDOM() { + this._sliderStartHandle = this.shadowRoot.querySelector(".ui5-slider-handle--start"); + this._sliderEndHandle = this.shadowRoot.querySelector(".ui5-slider-handle--end"); + this._sliderProgress = this.shadowRoot.querySelector(".ui5-slider-progress"); + } + get tooltipStartValue() { const stepPrecision = this.constructor._getDecimalPrecisionOfNumber(this._effectiveStep); return this.startValue.toFixed(stepPrecision); @@ -129,6 +136,103 @@ class RangeSlider extends SliderBase { this._updateHandlesAndRange(null); } + _onfocusin(event) { + this.focused = true; + + this._setInitialValue("startValue", this._prevStartValue || this.startValue); + this._setInitialValue("endValue", this._prevEndValue || this.endValue); + } + + _onfocusout(event) { + if (this._isFocusing()) { + // Prevent focusout when the focus is getting initially set within the slider before the + // slider customElement itself is finished focusing. + this._preventFocusOut(); + } else { + this.focused = false; + this._valueAffected = null; + + // Reset the stored Slider's initial value saved when it was first focused + this._setInitialValue("value", null); + } + } + + _preventFocusOut() { + this._focusInnerElement(); + } + + _onkeydown(event) { + this._onKeyDownBase(event); + } + + _handleActionKeyPress(event) { + if (isEscape(event)) { + this.startValue = this._prevStartValue; + this.endValue = this._prevEndValue; + } + + + if (this.shadowRoot.activeElement === this._sliderStartHandle) { + this._valueAffected = "startValue" + } + + if (this.shadowRoot.activeElement === this._sliderEndHandle) { + this._valueAffected = "endValue" + } + + const min = this._effectiveMin; + const max = this._effectiveMax; + const valueType = this._valueAffected; + + if ((isEnd(event) || isHome(event)) && !valueType) { + const eventType = isHome(event) ? "home" : "end" + this._handleHomeEndKeys(event, eventType, min, max) + + return; + } + + if (valueType) { + const newValueOffset = SliderBase.prototype._handleActionKeyPress.call(this, event, valueType); + + if (!newValueOffset) { + return; + } + + const newValue = isEscape(event) ? this._getInitialValue(valueType) : this.constructor.clipValue(newValueOffset + currentValue, min, max); + const currentValue = this[valueType]; + + this._updateHandlesAndRange(newValue); + this.updateValue(valueType, newValue); + this.storePropertyState(valueType); + } else { + const newValueOffset = SliderBase.prototype._handleActionKeyPress.call(this, event, null); + + if (!newValueOffset) { + return; + } + + const newStartValue = isEscape(event) ? this._getInitialValue("startValue") : this.constructor.clipValue(newValueOffset + this.startValue, min, max); + const newEndValue = isEscape(event) ? this._getInitialValue("endValue") : this.constructor.clipValue(newValueOffset + this.endValue, min, max); + + this.updateValue("startValue", newStartValue); + this.updateValue("endValue", newEndValue); + this._updateHandlesAndRange(null); + this.storePropertyState("startValue", "endValue"); + } + } + + _handleHomeEndKeys(event, eventType, min, max) { + const affectedValue = eventType === "home" ? "startValue" : "endValue"; + const newValueOffset = SliderBase.prototype._handleActionKeyPress.call(this, event, affectedValue) + const newStartValue = this.constructor.clipValue(newValueOffset + this.startValue, min, max); + const newEndValue = this.constructor.clipValue(newValueOffset + this.endValue, min, max); + + this.updateValue("startValue", newStartValue); + this.updateValue("endValue", newEndValue); + this._updateHandlesAndRange(null); + this.storePropertyState("startValue", "endValue"); + } + /** * Called when the user starts interacting with the slider * @@ -254,9 +358,9 @@ class RangeSlider extends SliderBase { this._swapValues(); this.handleUpBase(); - this._valueAffected = null; this._prevStartValue = null; this._prevEndValue = null; + this._inCurrentRange = null; } /** @@ -300,6 +404,37 @@ class RangeSlider extends SliderBase { } } + /* Flag the component that it is currently being in process of focusing in. When the slider is getting focused + we need that focus to be delegated to the Slider's handle and to not stay on the slider's shadow root div, as it + is by default. In theory this can be achieved either if the 'delegatesFocus' attribute of the .attachShadow() + customElement method is set to true or if we forward it manually as part of the component logic. + + As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and + we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be + removed as it will become redundant. + + When we manually set the focus on mouseDown to the first focusable element inside the shadowDom - the slider's handle, + that inside focus and subsquently the shadowRoot.activeElement are set a moment before the global document.activeElement + is set to the customElement (ui5-slider) causing a 'race condition'. + + In order for a element within the shadowRoot to be focused, the global document.activeElement MUST be the parent + customElement of the shadow root, in our case the ui5-slider component. Because of that after our focusin of the handle, + a focusout event fired by the browser immidiatly after, resetting the focus. + + Note: If we set the focus to the handle a bit later in time, for example on a mouseup or click event it will + work fine and we will avoid the described race condition as our customElement will be already finished focusing. + However, that does not work for us as we need the focus to be set to the handle exactly on mousedown, + because of the nature of the component and its available drag interactions.*/ + _focusInnerElement() { + if (this._inCurrentRange) { + this._sliderProgress.focus(); + } else if (this._valueAffected === "startValue") { + this._sliderStartHandle.focus(); + } else { + this._sliderEndHandle.focus(); + } + } + /** * Calculates startValue/endValue properties when the whole range is moved. * @@ -419,6 +554,10 @@ class RangeSlider extends SliderBase { } } + get tabIndexProgress() { + return this.tabIndex; + } + get styles() { return { progress: { diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index 1abec00d8407..609c2a0e02aa 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -6,6 +6,7 @@ import SliderBase from "./SliderBase.js"; // Template import SliderTemplate from "./generated/templates/SliderTemplate.lit.js"; +import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; /** * @public @@ -114,6 +115,10 @@ class Slider extends SliderBase { this._updateHandleAndProgress(this.value); } + _onkeydown(event) { + this._onKeyDownBase(event, "value"); + } + /** * Called when the user starts interacting with the slider * diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 476688a9681b..8936bbb9f0b0 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -277,6 +277,47 @@ class SliderBase extends UI5Element { } } + _setInitialValue(valueType, value) { + this[`_${valueType}Initial`] = value; + } + + _getInitialValue(valueType) { + return this[`_${valueType}Initial`]; + } + + _onKeyDownBase(event) { + if (this.disabled) { + return; + } + + if (SliderBase._isActionKey(event)) { + event.preventDefault(); + this._isUserInteraction = true; + this._handleActionKeyPress(event); + } + } + + _onkeyup(event) { + if (this.disabled) { + return; + } + + this._isUserInteraction = false; + } + + static _isActionKey(event) { + return this.ACTION_KEYS.some(actionKey => actionKey(event)); + } + + _isFocusing() { + return this._isInProcessOfFocusing; + } + + _setIsFocusing(isInProcessOfFocusing) { + this._isInProcessOfFocusing = isInProcessOfFocusing; + } + + /** * Sets initial value when the component is focused in, can be restored with ESC key * diff --git a/packages/main/src/themes/SliderBase.css b/packages/main/src/themes/SliderBase.css index 091fd15382aa..05f1d6ea501c 100644 --- a/packages/main/src/themes/SliderBase.css +++ b/packages/main/src/themes/SliderBase.css @@ -49,6 +49,11 @@ position: relative; } +.ui5-slider-progress:focus { + outline: var(--_ui5_slider_progress_outline); + outline-offset: var(--_ui5_slider_progress_outline_offset); +} + .ui5-slider-tickmarks { color: var(--_ui5_slider_tickmark_color); position: absolute; @@ -74,10 +79,6 @@ outline-offset: var(--_ui5_slider_handle_outline_offset); } -.ui5-slider-handle--start, .ui5-slider-handle--end { - background: var(--_ui5_range_slider_handle_background); -} - [dir="rtl"] .ui5-slider-handle { margin-right: var(--_ui5_slider_handle_margin_left); } @@ -88,11 +89,6 @@ border-color: var(--_ui5_slider_handle_hover_border); } -.ui5-slider-root:hover .ui5-slider-handle--start, .ui5-slider-root:hover .ui5-slider-handle--end, -.ui5-slider-root:active .ui5-slider-handle--start, .ui5-slider-root:active .ui5-slider-handle--end, -.ui5-slider-handle--start:active, .ui5-slider-handle--end:active { - background: var(--_ui5_range_slider_handle_hover_background); -} .ui5-slider-tooltip { text-align: center; diff --git a/packages/main/src/themes/base/SliderBase-parameters.css b/packages/main/src/themes/base/SliderBase-parameters.css index 4f23504852ee..46a2d131e2f0 100644 --- a/packages/main/src/themes/base/SliderBase-parameters.css +++ b/packages/main/src/themes/base/SliderBase-parameters.css @@ -9,14 +9,14 @@ --_ui5_slider_handle_width: 1.625rem; --_ui5_slider_handle_border: solid 0.125rem var(--sapField_BorderColor); --_ui5_slider_handle_background: var(--sapButton_Background); - --_ui5_range_slider_handle_background: rgba(var(--sapButton_Background), 0.25); --_ui5_slider_handle_top: -0.825rem; --_ui5_slider_handle_margin_left: -0.9725rem; --_ui5_slider_handle_hover_background: var(--sapButton_Hover_Background); --_ui5_slider_handle_hover_border: var(--sapButton_Hover_BorderColor); --_ui5_slider_handle_outline: 1px dotted var(--sapContent_FocusColor); --_ui5_slider_handle_outline_offset: 0.075rem; - --_ui5_range_slider_handle_hover_background: rgba(var(--sapButton_Background), 0.25); + --_ui5_slider_progress_outline: 0.0625rem dotted var(--sapContent_FocusColor); + --_ui5_slider_progress_outline_offset: 0.525rem; --_ui5_slider_tickmark_color: #89919a; --_ui5_slider_tickmark_top: -0.375rem; --_ui5_slider_disabled_opacity: 0.4; From 12233d3fcc94b492059e4ae0354d39ee96facc04 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Wed, 23 Dec 2020 08:21:01 +0200 Subject: [PATCH 11/20] Focus and keyboard handling documentation The Range Slider is now focusable, the elements that gets the focus when the component is active are the slider's handles and progress tracker. The full keyboard handling specifications are implemented. --- packages/main/src/RangeSlider.hbs | 6 +- packages/main/src/RangeSlider.js | 357 +++++++++++++++++++----------- packages/main/src/Slider.js | 10 +- packages/main/src/SliderBase.js | 58 ++++- 4 files changed, 284 insertions(+), 147 deletions(-) diff --git a/packages/main/src/RangeSlider.hbs b/packages/main/src/RangeSlider.hbs index 1ed4b1ba6147..6ebefdbec6c1 100644 --- a/packages/main/src/RangeSlider.hbs +++ b/packages/main/src/RangeSlider.hbs @@ -1,16 +1,14 @@ {{>include "./SliderBase.hbs"}} {{#*inline "handles"}} -
+
{{#if showTooltip}}
{{tooltipStartValue}}
{{/if}}
-
+
{{#if showTooltip}}
{{tooltipEndValue}} diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index b73ea0db699a..832ce916eafb 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -1,9 +1,12 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import { fetchI18nBundle, getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js"; -import SliderBase from "./SliderBase.js"; -import {isEscape, isTabPrevious, isTabNext, isHome, isEnd} from "@ui5/webcomponents-base/dist/Keys.js"; +import { + isEscape, + isHome, + isEnd, +} from "@ui5/webcomponents-base/dist/Keys.js"; -// Template +import SliderBase from "./SliderBase.js"; import RangeSliderTemplate from "./generated/templates/RangeSliderTemplate.lit.js"; /** @@ -136,101 +139,145 @@ class RangeSlider extends SliderBase { this._updateHandlesAndRange(null); } - _onfocusin(event) { + _onfocusin(event) { this.focused = true; - this._setInitialValue("startValue", this._prevStartValue || this.startValue); - this._setInitialValue("endValue", this._prevEndValue || this.endValue); + // If this is the initial focusin of the component save its initial + // value properties so they could be restored on ESC key press + if (!this._getInitialValue("endValue")) { + this._setInitialValue("startValue", this.startValue); + this._setInitialValue("endValue", this.endValue); + } } + /** + * Handles focus out event of the focusable components inner elements. + * Prevent focusout when the focus is getting initially set within the slider before the + * slider customElement itself is finished focusing. + * + * Prevents the focus from leaving the Range Slider when the focus is managed between + * its inner elements in result of user interactions. + * + * Resets the stored Range Slider's initial values saved when it was first focused + * + * @private + */ _onfocusout(event) { - if (this._isFocusing()) { - // Prevent focusout when the focus is getting initially set within the slider before the - // slider customElement itself is finished focusing. + if (this._isFocusing()) { this._preventFocusOut(); } else { this.focused = false; - this._valueAffected = null; - - // Reset the stored Slider's initial value saved when it was first focused - this._setInitialValue("value", null); + this._setAffectedValue(null); + this._setInitialValue("startValue", null); + this._setInitialValue("endValue", null); } } - _preventFocusOut() { - this._focusInnerElement(); - } + /** + * Handles keyup logic. If one of the handles came across the other + * swap the start and end values. Reset the affected value by the finished + * user interaction. + * + * @private + */ + _onkeyup(event) { + SliderBase.prototype._onkeyup.call(this, event); - _onkeydown(event) { - this._onKeyDownBase(event); + this._swapValues(); + this._setAffectedValue(null); } _handleActionKeyPress(event) { if (isEscape(event)) { - this.startValue = this._prevStartValue; - this.endValue = this._prevEndValue; + this.update(null, this._getInitialValue("startValue"), this._getInitialValue("endValue")); + return; } + // Set the target of the interaction based on the focused inner element + this._setAffectedValueByFocusedElement(); - if (this.shadowRoot.activeElement === this._sliderStartHandle) { - this._valueAffected = "startValue" - } - - if (this.shadowRoot.activeElement === this._sliderEndHandle) { - this._valueAffected = "endValue" - } - const min = this._effectiveMin; const max = this._effectiveMax; - const valueType = this._valueAffected; - - if ((isEnd(event) || isHome(event)) && !valueType) { - const eventType = isHome(event) ? "home" : "end" - this._handleHomeEndKeys(event, eventType, min, max) - + const affectedValue = this._getAffectedValue(); + + // If home/end key is pressed and no single handle is focused the active element + // is the range selection - update both start and end values. Otherwise, if 'home' + // is pressed the 'startValue'will be used for the start-handle offset calculation, + // if 'End' is pressed - the 'endValue' will be used for the end-handle update. + if ((isEnd(event) || isHome(event)) && !affectedValue) { + this._homeEndForSelectedRange(event, isHome(event) ? "startValue" : "endValue", min, max); return; } - if (valueType) { - const newValueOffset = SliderBase.prototype._handleActionKeyPress.call(this, event, valueType); - - if (!newValueOffset) { - return; - } + // Calculate how much the value should be increased/decreased based on the action key + const newValueOffset = this._handleActionKeyPressBase(event, affectedValue); - const newValue = isEscape(event) ? this._getInitialValue(valueType) : this.constructor.clipValue(newValueOffset + currentValue, min, max); - const currentValue = this[valueType]; + if (!newValueOffset) { + return; + } - this._updateHandlesAndRange(newValue); - this.updateValue(valueType, newValue); - this.storePropertyState(valueType); - } else { - const newValueOffset = SliderBase.prototype._handleActionKeyPress.call(this, event, null); + // Update a single value if one of the handles is focused or the range if not already at min or max + if (affectedValue && !this._inCurrentRange) { + const newValue = this.constructor.clipValue(newValueOffset + this[affectedValue], min, max); + this.update(affectedValue, newValue, null); + } else if ((newValueOffset < 0 && this.startValue > min) || (newValueOffset > 0 && this.endValue < max)) { + const newStartValue = this.constructor.clipValue(newValueOffset + this.startValue, min, max); + const newEndValue = this.constructor.clipValue(newValueOffset + this.endValue, min, max); + this.update(affectedValue, newStartValue, newEndValue); + } + } - if (!newValueOffset) { - return; - } + /** + * Determines affected value (start/end) depending on the currently + * active inner element within the Range Slider - used in the keyboard handling. + * + * @private + */ + _setAffectedValueByFocusedElement() { + if (this.shadowRoot.activeElement === this._sliderStartHandle) { + this._setAffectedValue("startValue"); + } - const newStartValue = isEscape(event) ? this._getInitialValue("startValue") : this.constructor.clipValue(newValueOffset + this.startValue, min, max); - const newEndValue = isEscape(event) ? this._getInitialValue("endValue") : this.constructor.clipValue(newValueOffset + this.endValue, min, max); + if (this.shadowRoot.activeElement === this._sliderEndHandle) { + this._setAffectedValue("endValue"); + } - this.updateValue("startValue", newStartValue); - this.updateValue("endValue", newEndValue); - this._updateHandlesAndRange(null); - this.storePropertyState("startValue", "endValue"); + if (this.shadowRoot.activeElement === this._sliderProgress) { + this._inCurrentRange = true; } } - _handleHomeEndKeys(event, eventType, min, max) { - const affectedValue = eventType === "home" ? "startValue" : "endValue"; - const newValueOffset = SliderBase.prototype._handleActionKeyPress.call(this, event, affectedValue) - const newStartValue = this.constructor.clipValue(newValueOffset + this.startValue, min, max); - const newEndValue = this.constructor.clipValue(newValueOffset + this.endValue, min, max); + /** + * Calculates the start and end values when the 'Home" or 'End' keys + * are pressed on the selected range bar. + * + * @private + */ + _homeEndForSelectedRange(event, affectedValue, min, max) { + const newValueOffset = this._handleActionKeyPressBase(event, affectedValue); + const newStartValue = this.constructor.clipValue(newValueOffset + this.startValue, min, max); + const newEndValue = this.constructor.clipValue(newValueOffset + this.endValue, min, max); - this.updateValue("startValue", newStartValue); - this.updateValue("endValue", newEndValue); + this.update(null, newStartValue, newEndValue); + } + + /** + * Update values, stored inner state and the visual UI representation of the component. + * If no specific type of value property is passed - the range is selected - update both handles, + * otherwise update the handle corresponding to the affected by the user interacton value prop. + * + * @private + */ + update(affectedValue, startValue, endValue) { + if (!affectedValue) { + this.updateValue("startValue", startValue); + this.updateValue("endValue", endValue); this._updateHandlesAndRange(null); - this.storePropertyState("startValue", "endValue"); + } else { + const newValue = startValue; + this._updateHandlesAndRange(newValue); + this.updateValue(affectedValue, newValue); + } } /** @@ -258,9 +305,7 @@ class RangeSlider extends SliderBase { } // Update Slider UI and internal state - this._updateHandlesAndRange(newValue); - this.updateValue(this._valueAffected, newValue); - this.storePropertyState(this._valueAffected); + this.update(this._getAffectedValue(), newValue, null); } @@ -278,18 +323,16 @@ class RangeSlider extends SliderBase { const progressBarDom = this.shadowRoot.querySelector(".ui5-slider-progress").getBoundingClientRect(); // Save the state of the value properties on the start of the interaction - this._prevStartValue = this.startValue; - this._prevEndValue = this.endValue; + this._startValueAtBeginningOfAction = this.startValue; + this._endValueAtBeginningOfAction = this.endValue; // Check if the new value is in the current select range of values - this._inCurrentRange = newValue > this._prevStartValue && newValue < this._prevEndValue; + this._inCurrentRange = newValue > this._startValueAtBeginningOfAction && newValue < this._endValueAtBeginningOfAction; // Save the initial press point coordinates (position) this._initialPageXPosition = this.constructor.getPageXValueFromEvent(event); // Which element of the Range Slider is pressed and which value property to be modified on further interaction this._pressTargetAndAffectedValue(this._initialPageXPosition, newValue); - // Use the progress bar to save the initial coordinates of the start-handle when the interaction begins. - // We will use it as a reference to calculate a moving offset if the whole range selection is dragged. this._initialStartHandlePageX = this.directionStart === "left" ? progressBarDom.left : progressBarDom.right; } @@ -324,10 +367,7 @@ class RangeSlider extends SliderBase { */ _updateValueOnHandleDrag(event) { const newValue = this.constructor.getValueFromInteraction(event, this._effectiveStep, this._effectiveMin, this._effectiveMax, this.getBoundingClientRect(), this.directionStart); - - this._updateHandlesAndRange(newValue); - this.updateValue(this._valueAffected, newValue); - this.storePropertyState(this._valueAffected); + this.update(this._getAffectedValue(), newValue, null); } /** @@ -340,27 +380,27 @@ class RangeSlider extends SliderBase { const currentPageXPos = this.constructor.getPageXValueFromEvent(event); const newValues = this._calculateRangeOffset(currentPageXPos, this._initialStartHandlePageX); - // No matter the which value is set as the one to be modified (this._valueAffected) we want to modify both of them - this._valueAffected = null; + // No matter the which value is set as the one to be modified (by prev. user action) we want to modify both of them + this._setAffectedValue(null); // Update the UI and the state acccording to the calculated new values - this.updateValue("startValue", newValues[0]); - this.updateValue("endValue", newValues[1]); - this._updateHandlesAndRange(null); - this.storePropertyState("startValue", "endValue"); + this.update(null, newValues[0], newValues[1]); } _handleUp() { - if (this.startValue !== this._prevStartValue || this.endValue !== this._prevEndValue) { + if (this.startValue !== this._startValueAtBeginningOfAction || this.endValue !== this._endValueAtBeginningOfAction) { this.fireEvent("change"); } this._swapValues(); - this.handleUpBase(); + this._setAffectedValueByFocusedElement(); + this._setAffectedValue(null); - this._prevStartValue = null; - this._prevEndValue = null; + this._startValueAtBeginningOfAction = null; + this._endValueAtBeginningOfAction = null; this._inCurrentRange = null; + + this.handleUpBase(); } /** @@ -395,45 +435,75 @@ class RangeSlider extends SliderBase { // Return that handle that is closer to the press point if (inHandleEndDom || value > this.endValue) { - this._valueAffected = "endValue"; + this._setAffectedValue("endValue"); } // If one of the handle is pressed return that one if (inHandleStartDom || value < this.startValue) { - this._valueAffected = "startValue"; + this._setAffectedValue("startValue"); } } - /* Flag the component that it is currently being in process of focusing in. When the slider is getting focused - we need that focus to be delegated to the Slider's handle and to not stay on the slider's shadow root div, as it - is by default. In theory this can be achieved either if the 'delegatesFocus' attribute of the .attachShadow() - customElement method is set to true or if we forward it manually as part of the component logic. - - As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and - we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be - removed as it will become redundant. - - When we manually set the focus on mouseDown to the first focusable element inside the shadowDom - the slider's handle, - that inside focus and subsquently the shadowRoot.activeElement are set a moment before the global document.activeElement - is set to the customElement (ui5-slider) causing a 'race condition'. - - In order for a element within the shadowRoot to be focused, the global document.activeElement MUST be the parent - customElement of the shadow root, in our case the ui5-slider component. Because of that after our focusin of the handle, - a focusout event fired by the browser immidiatly after, resetting the focus. - - Note: If we set the focus to the handle a bit later in time, for example on a mouseup or click event it will - work fine and we will avoid the described race condition as our customElement will be already finished focusing. - However, that does not work for us as we need the focus to be set to the handle exactly on mousedown, - because of the nature of the component and its available drag interactions.*/ + /** + * Sets the value property (start/end) that will get updated + * by a user action depending on that user action's characteristics + * - mouse press position - cursor coordinates relative to the start/end handles + * - selected inner element via a keyboard navigation + * + * @param {String} valuePropAffectedByInteraction The value that will get modified by the interaction + * @private + */ + _setAffectedValue(valuePropAffectedByInteraction) { + this._valueAffected = valuePropAffectedByInteraction; + } + + _getAffectedValue() { + return this._valueAffected; + } + + /** + * Manage the focus between the focusable inner elements within the component. + * + * On initial focusin or if the whole range is affected by the user interaction + * set the focus on the progress selection, otherwise on one of the Range Slider + * handles based on the determined affected value by the user action. + * + * If one of the handles came across the other one in result of a user action + * switch the focus between them to keep it visually consistent. + * + * Note: + * In some cases this function is going to get called twice on one user action. + * + * 1. When the focus is initially set to an inner element it is done in the very beginning, + * of an interaction - on 'mousedown' and 'keydown' events. The focus of the host custom element + * is still not being received, causining an immediate focusout that we prevent by + * calling this function once again. + * + * 2. When the focused is manually switched from one inner element to another. + * The focusout handler is one and the same for all focusable parts within the + * Range Slider and when is called it checks if it should keep the focus within + * the component and which part of it should get focused if that is the case. + * + * @private + */ _focusInnerElement() { - if (this._inCurrentRange) { + const isReversed = this._getAreValuesReversed(); + const affectedValue = this._getAffectedValue(); + + if (this._inCurrentRange || !affectedValue) { this._sliderProgress.focus(); - } else if (this._valueAffected === "startValue") { + } + + if ((affectedValue === "startValue" && !isReversed) || (affectedValue === "endValue" && isReversed)) { this._sliderStartHandle.focus(); - } else { + this._switchReversedValues(); + } + + if ((affectedValue === "endValue" && !isReversed) || (affectedValue === "startValue" && isReversed)) { this._sliderEndHandle.focus(); + this._switchReversedValues(); } - } + } /** * Calculates startValue/endValue properties when the whole range is moved. @@ -507,21 +577,26 @@ class RangeSlider extends SliderBase { return startValue; } + /** + * Updates the visual representation of the component by calculating + * the styles of the handles and the range selection based on the new state. + * + * @private + */ _updateHandlesAndRange(newValue) { const max = this._effectiveMax; const min = this._effectiveMin; const prevStartValue = this.getStoredPropertyState("startValue"); const prevEndValue = this.getStoredPropertyState("endValue"); + const affectedValue = this._getAffectedValue(); // The value according to which we update the UI can be either the startValue // or the endValue property. It is determined in _getClosestHandle() // depending on to which handle is closer the user interaction. - if (this._valueAffected === "startValue") { - // When the value changing is the start value: + if (affectedValue === "startValue") { this._selectedRange = (prevEndValue - newValue) / (max - min); this._firstHandlePositionFromStart = ((newValue - min) / (max - min)) * 100; - } else if (this._valueAffected === "endValue") { - // Wen the value changing is the end value: + } else if (affectedValue === "endValue") { this._selectedRange = ((newValue - prevStartValue)) / (max - min); this._secondHandlePositionFromStart = (newValue - min) / (max - min) * 100; } else { @@ -533,27 +608,55 @@ class RangeSlider extends SliderBase { } /** - * Swaps start and end values and handles (thumbs), if one came accros the other + * Swaps the start and end values of the handles if one came accros the other: + * - If the start value is greater than the endValue swap them and their handles + * - If the endValue become less than the start value swap them and their handles + * + * Switches the focus to the opposite of the currently focused handle. + * + * Note: Only the property values are reversed, the DOM elements of the handles + * corresponding to them are never switched. * * @private */ _swapValues() { - // If the start value is greater than the endValue swap them and their handles - if (this._valueAffected === "startValue" && this.startValue > this.endValue) { - const oldEndValue = this.endValue; + const affectedValue = this._getAffectedValue(); + + if (affectedValue === "startValue" && this.startValue > this.endValue) { + const prevEndValue = this.endValue; this.endValue = this.startValue; - this.startValue = oldEndValue; - return; + this.startValue = prevEndValue; + + this._switchReversedValues(); + this._focusInnerElement(); } - // If the endValue become less than the start value swap them and their handles - if (this._valueAffected === "endValue" && this.endValue < this.startValue) { - const oldStartValue = this.startValue; + if (affectedValue === "endValue" && this.endValue < this.startValue) { + const prevStartValue = this.startValue; this.startValue = this.endValue; - this.endValue = oldStartValue; + this.endValue = prevStartValue; + + this._switchReversedValues(); + this._focusInnerElement(); } } + /** + * Flag that we have swapped the values of the 'start' and 'end' properties, + * to correctly switch the focus within the component from one handle to another + * when the swapping is finished. As we only swap property values and not + * the handle elements themselves, we must also swap their focus. + * + * @private + */ + _switchReversedValues() { + this._reversedValues = !this._reversedValues; + } + + _getAreValuesReversed(areValuesReversed) { + return this._reversedValues; + } + get tabIndexProgress() { return this.tabIndex; } diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index 609c2a0e02aa..f3001d189653 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -1,12 +1,12 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import { fetchI18nBundle, getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js"; import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; +<<<<<<< HEAD import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; +======= +>>>>>>> 0b3a6aaf (Focus and keyboard handling documentation) import SliderBase from "./SliderBase.js"; - -// Template import SliderTemplate from "./generated/templates/SliderTemplate.lit.js"; -import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; /** * @public @@ -115,10 +115,6 @@ class Slider extends SliderBase { this._updateHandleAndProgress(this.value); } - _onkeydown(event) { - this._onKeyDownBase(event, "value"); - } - /** * Called when the user starts interacting with the slider * diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 8936bbb9f0b0..e67a9a09f094 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -277,21 +277,27 @@ class SliderBase extends UI5Element { } } + /** + * Sets initial value when the component is focused in, can be restored with ESC key + * + * @private + */ _setInitialValue(valueType, value) { this[`_${valueType}Initial`] = value; - } + } _getInitialValue(valueType) { return this[`_${valueType}Initial`]; } - _onKeyDownBase(event) { + _onkeydown(event) { if (this.disabled) { return; } if (SliderBase._isActionKey(event)) { event.preventDefault(); + this._isUserInteraction = true; this._handleActionKeyPress(event); } @@ -305,18 +311,52 @@ class SliderBase extends UI5Element { this._isUserInteraction = false; } - static _isActionKey(event) { - return this.ACTION_KEYS.some(actionKey => actionKey(event)); + /** + * Flags if an inner element is currently being focused + * + * @private + */ + _preserveFocus(isFocusing) { + this._isInnerElementFocusing = isFocusing; } - + + /** + * Return if an inside element within the component is currently being focused + * + * @private + */ _isFocusing() { - return this._isInProcessOfFocusing; + return this._isInnerElementFocusing; } - _setIsFocusing(isInProcessOfFocusing) { - this._isInProcessOfFocusing = isInProcessOfFocusing; - } + /** + * Prevent focus out when inner element within the component is currently being in process of focusing in. + * In theory this can be achieved either if the shadow root is focusable and 'delegatesFocus' attribute of + * the .attachShadow() customElement method is set to true, or if we forward it manually. + + * As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and + * we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be + * removed as it will become redundant. + * + * When we manually set the focus on mouseDown to the first focusable element inside the shadowDom, + * that inner focus (shadowRoot.activeElement) is set a moment before the global document.activeElement + * is set to the customElement (ui5-slider) causing a 'race condition'. + * + * In order for a element within the shadowRoot to be focused, the global document.activeElement MUST be the parent + * customElement of the shadow root, in our case the ui5-slider component. Because of that after our focusin of the handle, + * a focusout event fired by the browser immidiatly after, resetting the focus. Focus out must be manually prevented + * in both initial focusing and switching the focus between inner elements of the component cases. + * Note: If we set the focus to the handle with a timeout or a bit later in time, on a mouseup or click event it will + * work fine and we will avoid the described race condition as our host customElement will be already finished focusing. + * However, that does not work for us as we need the focus to be set to the handle exactly on mousedown, + * because of the nature of the component and its available drag interactions. + * + * @private + */ + _preventFocusOut() { + this._focusInnerElement(); + } /** * Sets initial value when the component is focused in, can be restored with ESC key From e7130cca2eed8d8509c284aed7b1166019e926e7 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Wed, 30 Dec 2020 09:52:44 +0200 Subject: [PATCH 12/20] Add tests for focus and keyboard handling --- packages/base/src/Keys.js | 1 - packages/main/src/RangeSlider.js | 21 +- packages/main/test/specs/RangeSlider.spec.js | 561 +++++++++++++++++++ 3 files changed, 574 insertions(+), 9 deletions(-) diff --git a/packages/base/src/Keys.js b/packages/base/src/Keys.js index 433337e78b4b..d574d8213ace 100644 --- a/packages/base/src/Keys.js +++ b/packages/base/src/Keys.js @@ -212,5 +212,4 @@ export { isPageDownShift, isPageUpShiftCtrl, isPageDownShiftCtrl, - getCtrlKey, }; diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index 832ce916eafb..8ff28435e652 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -5,7 +5,7 @@ import { isHome, isEnd, } from "@ui5/webcomponents-base/dist/Keys.js"; - +import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import SliderBase from "./SliderBase.js"; import RangeSliderTemplate from "./generated/templates/RangeSliderTemplate.lit.js"; @@ -107,6 +107,8 @@ class RangeSlider extends SliderBase { this._sliderStartHandle = this.shadowRoot.querySelector(".ui5-slider-handle--start"); this._sliderEndHandle = this.shadowRoot.querySelector(".ui5-slider-handle--end"); this._sliderProgress = this.shadowRoot.querySelector(".ui5-slider-progress"); + + ResizeHandler.register(this, this._resizeHandler); } get tooltipStartValue() { @@ -455,6 +457,11 @@ class RangeSlider extends SliderBase { */ _setAffectedValue(valuePropAffectedByInteraction) { this._valueAffected = valuePropAffectedByInteraction; + + // If the values have been swapped reset the reversed flag + if (this._areValuesReversed()) { + this._setValuesAreReversed(); + } } _getAffectedValue() { @@ -487,7 +494,7 @@ class RangeSlider extends SliderBase { * @private */ _focusInnerElement() { - const isReversed = this._getAreValuesReversed(); + const isReversed = this._areValuesReversed(); const affectedValue = this._getAffectedValue(); if (this._inCurrentRange || !affectedValue) { @@ -496,12 +503,10 @@ class RangeSlider extends SliderBase { if ((affectedValue === "startValue" && !isReversed) || (affectedValue === "endValue" && isReversed)) { this._sliderStartHandle.focus(); - this._switchReversedValues(); } if ((affectedValue === "endValue" && !isReversed) || (affectedValue === "startValue" && isReversed)) { this._sliderEndHandle.focus(); - this._switchReversedValues(); } } @@ -627,7 +632,7 @@ class RangeSlider extends SliderBase { this.endValue = this.startValue; this.startValue = prevEndValue; - this._switchReversedValues(); + this._setValuesAreReversed(); this._focusInnerElement(); } @@ -636,7 +641,7 @@ class RangeSlider extends SliderBase { this.startValue = this.endValue; this.endValue = prevStartValue; - this._switchReversedValues(); + this._setValuesAreReversed(); this._focusInnerElement(); } } @@ -649,11 +654,11 @@ class RangeSlider extends SliderBase { * * @private */ - _switchReversedValues() { + _setValuesAreReversed() { this._reversedValues = !this._reversedValues; } - _getAreValuesReversed(areValuesReversed) { + _areValuesReversed() { return this._reversedValues; } diff --git a/packages/main/test/specs/RangeSlider.spec.js b/packages/main/test/specs/RangeSlider.spec.js index 619c45294a4f..9d1231c97655 100644 --- a/packages/main/test/specs/RangeSlider.spec.js +++ b/packages/main/test/specs/RangeSlider.spec.js @@ -262,6 +262,567 @@ describe("Testing events", () => { }); }); + +describe("Accessibility: Testing focus", () => { + it("Click anywhere in the Range Slider should focus the closest handle and set the 'focused' property to true", () => { + browser.url("http://localhost:8080/test-resources/pages/RangeSlider.html"); + + const rangeSlider = browser.$("#basic-range-slider"); + const rangeSliderStartHandle = rangeSlider.shadow$(".ui5-slider-handle--start"); + const rangeSliderEndHandle = rangeSlider.shadow$(".ui5-slider-handle--end"); + + rangeSlider.click(); + + let innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual(rangeSlider.isFocused(), true, "RangeSlider component is focused"); + assert.strictEqual(rangeSlider.getProperty("focused"), true, "RangeSlider state is focused"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderEndHandle.getAttribute("class"), "RangeSlider second handle has the shadowDom focus"); + + rangeSlider.setProperty("startValue", 30); + rangeSliderStartHandle.click({ x: -200}); + + innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderStartHandle.getAttribute("class"), "RangeSlider second handle has the shadowDom focus"); + }); + + it("Click currently selected range should focus it and set the 'focused' property to true", () => { + browser.url("http://localhost:8080/test-resources/pages/RangeSlider.html"); + + const rangeSlider = browser.$("#basic-range-slider"); + const rangeSliderSelection = rangeSlider.shadow$(".ui5-slider-progress"); + + rangeSlider.setProperty("endValue", 60); + rangeSlider.click(); + + let innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual(rangeSlider.isFocused(), true, "RangeSlider component is focused"); + assert.strictEqual(rangeSlider.getProperty("focused"), true, "RangeSlider state is focused"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderSelection.getAttribute("class"), "RangeSlider progress bar has the shadowDom focus"); + }); + + + it("When not yet focused, 'Tab' should focus the Range Slider and move the focus to the progress bar", () => { + browser.url("http://localhost:8080/test-resources/pages/RangeSlider.html"); + + const rangeSlider = browser.$("#basic-range-slider"); + const rangeSliderSelection = rangeSlider.shadow$(".ui5-slider-progress"); + + browser.keys("Tab"); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual(rangeSlider.isFocused(), true, "Range Slider component is focused"); + assert.strictEqual(rangeSlider.getProperty("focused"), true, "Range Slider is focused"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderSelection.getAttribute("class"), "Range Slider progress tracker has the shadowDom focus"); + }); + + it("When progress bar has the focus, 'Tab' should move the focus to the first handle", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const rangeSliderStartHandle = rangeSlider.shadow$(".ui5-slider-handle--start"); + + browser.keys("Tab"); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderStartHandle.getAttribute("class"), "Range Slider first handle has the hadowDom focus"); + }); + + it("When the first handle has the focus, 'Tab' should focus the second handle", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const rangeSliderEndHandle = rangeSlider.shadow$(".ui5-slider-handle--end"); + + browser.keys("Tab"); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderEndHandle.getAttribute("class"), "Range Slider second handle has the shadowDom focus"); + }); + + it("When the second handle has the focus, 'Tab' should move the focus away from the Range Slider", () => { + const currentRangeSlider = browser.$("#basic-range-slider"); + const nextRangeSlider = browser.$("#basic-range-slider-with-tooltip"); + const rangeSliderSelection = nextRangeSlider.shadow$(".ui5-slider-progress"); + + browser.keys("Tab"); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider-with-tooltip").shadowRoot.activeElement; + }); + + assert.strictEqual(currentRangeSlider.isFocused(), false, "First RangeSlider component is now not focused"); + assert.strictEqual(currentRangeSlider.getProperty("focused"), false, "First RangeSlider state is not focused"); + + assert.strictEqual(nextRangeSlider.isFocused(), true, "Next RangeSlider is focused"); + assert.strictEqual(nextRangeSlider.getProperty("focused"), true, "Next RangeSlider's state is focused"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderSelection.getAttribute("class"), "Next Range Slider second handle has the shadowDom focus"); + }); + + it("Shift+Tab should focus the previous Range Slider and move the focus to its second handle", () => { + const currentRangeSlider = browser.$("#basic-range-slider-with-tooltip"); + const previousRangeSlider = browser.$("#basic-range-slider"); + const previousRangeSliderEndHandle = previousRangeSlider.shadow$(".ui5-slider-handle--end"); + + browser.keys(["Shift", "Tab"]); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual(currentRangeSlider.isFocused(), false, "First RangeSlider component is now not focused"); + assert.strictEqual(currentRangeSlider.getProperty("focused"), false, "First RangeSlider state is not focused"); + + assert.strictEqual(previousRangeSlider.isFocused(), true, "Slider component is focused"); + assert.strictEqual(previousRangeSlider.getProperty("focused"), true, "Slider is focused"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), previousRangeSliderEndHandle.getAttribute("class"), "Previous Range Slider second handle now has the shadowDom focus"); + }); + + it("When the second handle has the focus, 'Shift' + 'Tab' should move the focus to the first handle", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const rangeSliderStartHandle = rangeSlider.shadow$(".ui5-slider-handle--start"); + + browser.keys(["Shift", "Tab"]); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderStartHandle.getAttribute("class"), "Range Slider first handle has the shadowDom focus"); + }); + + it("When the first handle has the focus, 'Shift' + 'Tab' should move the focus to the progress bar", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const rangeSliderSelection = rangeSlider.shadow$(".ui5-slider-progress"); + + browser.keys(["Shift", "Tab"]); + + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderSelection.getAttribute("class"), "Range Slider first handle has the shadowDom focus"); + }); + + it("When the progress bar has the focus, 'Shift' + 'Tab' should move the focus away from the Range Slider", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + + assert.strictEqual(rangeSlider.isFocused(), false, "First RangeSlider component is now not focused"); + assert.strictEqual(rangeSlider.getProperty("focused"), false, "First RangeSlider state is not focused"); + }); + + it("When one handle come across the other and the values are swapped the focus must be switched between the handles", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const startHandle = rangeSlider.shadow$(".ui5-slider-handle--start"); + const endHandle = rangeSlider.shadow$(".ui5-slider-handle--end"); + + startHandle.dragAndDrop({ x: 400, y: 1 }); + const innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual($(innerFocusedElement).getAttribute("class"), endHandle.getAttribute("class"), "Range Slider second handle now has the shadowDom focus"); + }); +}); + + +describe("Accessibility: Testing keyboard handling", () => { + it("When progress bar is focused 'Right Arrow' key should increase both values of the Range Slider with a small increment step", () => { + browser.url("http://localhost:8080/test-resources/pages/RangeSlider.html"); + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("Tab"); + browser.keys("ArrowRight"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 1, "start-value is increased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 21, "end-value is increased"); + }); + + it("When progress bar is focused 'Left Arrow' key should decrease both values of the Range Slider with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("ArrowLeft"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When progress bar is focused 'Up Arrow' key should increase both values of the Range Slider with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("ArrowUp"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 1, "start-value is increased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 21, "end-value is increased"); + }); + + it("When progress bar is focused 'Down' key should decrease both values of the Range Slider with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("ArrowDown"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When progress bar is focused 'Control' + 'Right Arrow' key should increase both values of the Range Slider with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Control", "ArrowRight"]); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 10, "start-value is increased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 30, "end-value is increased"); + }); + + it("When progress bar is focused 'Control' + 'Left Arrow' key should decrease both values of the Range Slider with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Control", "ArrowLeft"]); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When progress bar is focused 'Control' + 'Up Arrow' key should increase both values of the Range Slider with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Control", "ArrowUp"]); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 10, "start-value is increased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 30, "end-value is increased"); + }); + + it("When progress bar is focused 'Control' + 'Down' key should decrease both values of the Range Slider with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Control", "ArrowDown"]); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When progress bar is focused 'Page Up' key should increase both values of the Range Slider with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("PageUp"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 10, "start-value is increased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 30, "end-value is increased"); + }); + + it("When progress bar is focused 'Page Down' key should decrease both values of the Range Slider with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("PageDown"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When progress bar is focused the '+' key should increase both values of the Range Slider with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const numpadAdd = "\uE025"; + + browser.keys("+"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 1, "start-value is increased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 21, "end-value is increased"); + + browser.keys(numpadAdd); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 2, "start-value is increased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 22, "end-value is increased"); + }); + + it("When progress bar is focused the '-' key should decrease both values of the Range Slider with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const numpadSubtract = "\uE027"; + + browser.keys("-"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 1, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 21, "end-value is decreased"); + + browser.keys(numpadSubtract); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When progress bar is focused an 'End' key press should offset the selected range to the end of the Range Slider", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("End"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 80, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 100, "end-value is decreased"); + }); + + it("When progress bar is focused a 'Home' key press should offset the selected range to the start of the Range Slider", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("Home"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("A 'Esc' key press should return the values of the Range Slider at their initial point at the time of its focusing", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + rangeSlider.setProperty("startValue", 24); + rangeSlider.setProperty("endValue", 42); + + browser.keys("Escape"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When a handle is focused 'Right Arrow' key should increase its value with a small increment step", () => { + browser.url("http://localhost:8080/test-resources/pages/RangeSlider.html"); + + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("Tab"); + browser.keys("Tab"); + browser.keys("ArrowRight"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 1, "start-value is increased"); + + browser.keys("Tab"); + browser.keys("ArrowRight"); + assert.strictEqual(rangeSlider.getProperty("endValue"), 21, "end-value is increased"); + }); + + it("When a handle is focused 'Left Arrow' key should decrease its value with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys("ArrowLeft"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is increased"); + + browser.keys("Tab"); + browser.keys("ArrowLeft"); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When a handle is focused 'Up Arrow' key should increase its value with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys("ArrowUp"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 1, "start-value is increased"); + + browser.keys("Tab"); + browser.keys("ArrowUp"); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 21, "end-value is decreased"); + }); + + it("When a handle is focused 'Down' key should decrease its value with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys("ArrowDown"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is increased"); + + browser.keys("Tab"); + browser.keys("ArrowDown"); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When a handle is focused 'Control' + 'Right Arrow' key should increase its value with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys(["Control", "ArrowRight"]); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 10, "start-value is increased"); + + browser.keys("Tab"); + browser.keys(["Control", "ArrowRight"]); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 30, "end-value is increased"); + }); + + it("When a handle is focused 'Control' + 'Left Arrow' key should decrease its vale with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys(["Control", "ArrowLeft"]); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + + browser.keys("Tab"); + browser.keys(["Control", "ArrowLeft"]); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When a handle is focused 'Control' + 'Up Arrow' key should increase its value with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys(["Control", "ArrowUp"]); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 10, "start-value is increased"); + + browser.keys("Tab"); + browser.keys(["Control", "ArrowUp"]); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 30, "end-value is increased"); + }); + + it("When handle is focused 'Control' + 'Down' key should decrease its value with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys(["Control", "ArrowDown"]); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + + browser.keys("Tab"); + browser.keys(["Control", "ArrowDown"]); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When a handle is focused 'Page Up' key should increase its value with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + browser.keys(["Shift", "Tab"]); + browser.keys("PageUp"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 10, "start-value is increased"); + + browser.keys("Tab"); + browser.keys("PageUp"); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 30, "end-value is increased"); + }); + + it("When a handle is focused 'Page Down' key should decrease its value with a big increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys("PageDown"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + + browser.keys("Tab"); + browser.keys("PageDown"); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + }); + + it("When a handle focused the '+' key should increase its value with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const numpadAdd = "\uE025"; + + browser.keys(["Shift", "Tab"]); + browser.keys("+"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 1, "start-value is increased"); + + browser.keys(numpadAdd); + assert.strictEqual(rangeSlider.getProperty("startValue"), 2, "start-value is increased"); + + browser.keys("Tab"); + browser.keys("+"); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 21, "end-value is increased"); + + browser.keys(numpadAdd); + assert.strictEqual(rangeSlider.getProperty("endValue"), 22, "end-value is increased"); + + }); + + it("When a handle focused the '-' key should decrease its value with a small increment step", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const numpadSubtract = "\uE027"; + + browser.keys(["Shift", "Tab"]); + browser.keys("-"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 1, "start-value is decreased"); + + browser.keys(numpadSubtract); + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + + browser.keys("Tab"); + browser.keys("-"); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 21, "end-value is decreased"); + + browser.keys(numpadSubtract); + assert.strictEqual(rangeSlider.getProperty("endValue"), 20, "end-value is decreased"); + + }); + + it("When a handle is focused an 'End' key press should set its value to the maximum allowed", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys("End"); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 100, "end-value is decreased"); + }); + + it("When a handle is focused a 'Home' key press should set its value to the start of the Range Slider", () => { + const rangeSlider = browser.$("#basic-range-slider"); + + browser.keys(["Shift", "Tab"]); + browser.keys("Home"); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "start-value is decreased"); + }); + + it("When one handle come across the other and the values are swapped the focus must be switched between the handles", () => { + const rangeSlider = browser.$("#basic-range-slider"); + const startHandle = rangeSlider.shadow$(".ui5-slider-handle--start"); + const endHandle = rangeSlider.shadow$(".ui5-slider-handle--end"); + + rangeSlider.setProperty("endValue", 20); + startHandle.click(); + browser.keys("End"); + + let innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual(rangeSlider.getProperty("endValue"), 100, "The original end-value is set to min and switched as a start-value"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), endHandle.getAttribute("class"), "Range Slider second handle now has the shadowDom focus"); + + browser.keys("Home"); + + innerFocusedElement = browser.execute(() => { + return document.getElementById("basic-range-slider").shadowRoot.activeElement; + }); + + assert.strictEqual(rangeSlider.getProperty("startValue"), 0, "The original end-value is set to min and switched as a start-value"); + assert.strictEqual($(innerFocusedElement).getAttribute("class"), startHandle.getAttribute("class"), "Range Slider second handle now has the shadowDom focus"); + }); +}); + describe("Testing resize handling and RTL support", () => { it("Testing RTL support", () => { const rangeSlider = browser.$("#range-slider-tickmarks-labels"); From a6d29c226bddb375e6e5327511ac8af61170977a Mon Sep 17 00:00:00 2001 From: ndeshev Date: Mon, 4 Jan 2021 18:47:35 +0200 Subject: [PATCH 13/20] Fix rebasing errors --- packages/main/src/RangeSlider.js | 8 +-- packages/main/src/Slider.js | 5 +- packages/main/src/SliderBase.js | 85 ++------------------------------ 3 files changed, 10 insertions(+), 88 deletions(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index 8ff28435e652..ee45748b31a8 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -491,9 +491,9 @@ class RangeSlider extends SliderBase { * Range Slider and when is called it checks if it should keep the focus within * the component and which part of it should get focused if that is the case. * - * @private + * @protected */ - _focusInnerElement() { + focusInnerElement() { const isReversed = this._areValuesReversed(); const affectedValue = this._getAffectedValue(); @@ -633,7 +633,7 @@ class RangeSlider extends SliderBase { this.startValue = prevEndValue; this._setValuesAreReversed(); - this._focusInnerElement(); + this.focusInnerElement(); } if (affectedValue === "endValue" && this.endValue < this.startValue) { @@ -642,7 +642,7 @@ class RangeSlider extends SliderBase { this.endValue = prevStartValue; this._setValuesAreReversed(); - this._focusInnerElement(); + this.focusInnerElement(); } } diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index f3001d189653..3cddb5de8ff0 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -1,10 +1,7 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import { fetchI18nBundle, getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js"; import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; -<<<<<<< HEAD import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; -======= ->>>>>>> 0b3a6aaf (Focus and keyboard handling documentation) import SliderBase from "./SliderBase.js"; import SliderTemplate from "./generated/templates/SliderTemplate.lit.js"; @@ -143,7 +140,7 @@ class Slider extends SliderBase { } } - _focusInnerElement() { + focusInnerElement() { this._sliderHandle.focus(); } diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index e67a9a09f094..1f9b2c19737f 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -355,89 +355,14 @@ class SliderBase extends UI5Element { * @private */ _preventFocusOut() { - this._focusInnerElement(); + this.focusInnerElement(); } /** - * Sets initial value when the component is focused in, can be restored with ESC key - * - * @private - */ - _setInitialValue(valueType, value) { - this[`_${valueType}Initial`] = value; - } - - _getInitialValue(valueType) { - return this[`_${valueType}Initial`]; - } - - _onkeydown(event) { - if (this.disabled) { - return; - } - - if (SliderBase._isActionKey(event)) { - event.preventDefault(); - - this._isUserInteraction = true; - this._handleActionKeyPress(event); - } - } - - _onkeyup(event) { - if (this.disabled) { - return; - } - - this._isUserInteraction = false; - } - - /** - * Flags if an inner element is currently being focused - * - * @private - */ - _preserveFocus(isFocusing) { - this._isInnerElementFocusing = isFocusing; - } - - /** - * Return if an inside element within the component is currently being focused - * - * @private - */ - _isFocusing() { - return this._isInnerElementFocusing; - } - - /** - * Prevent focus out when inner element within the component is currently being in process of focusing in. - * In theory this can be achieved either if the shadow root is focusable and 'delegatesFocus' attribute of - * the .attachShadow() customElement method is set to true, or if we forward it manually. - - * As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and - * we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be - * removed as it will become redundant. - * - * When we manually set the focus on mouseDown to the first focusable element inside the shadowDom, - * that inner focus (shadowRoot.activeElement) is set a moment before the global document.activeElement - * is set to the customElement (ui5-slider) causing a 'race condition'. - * - * In order for a element within the shadowRoot to be focused, the global document.activeElement MUST be the parent - * customElement of the shadow root, in our case the ui5-slider component. Because of that after our focusin of the handle, - * a focusout event fired by the browser immidiatly after, resetting the focus. Focus out must be manually prevented - * in both initial focusing and switching the focus between inner elements of the component cases. - - * Note: If we set the focus to the handle with a timeout or a bit later in time, on a mouseup or click event it will - * work fine and we will avoid the described race condition as our host customElement will be already finished focusing. - * However, that does not work for us as we need the focus to be set to the handle exactly on mousedown, - * because of the nature of the component and its available drag interactions. - * - * @private + * This method is reserved for derived classes for managing the focus between the component's inner elements + * @protected */ - _preventFocusOut() { - this._focusInnerElement(); - } + focusInnerElement() {} /** * Handle the responsiveness of the Slider's UI elements when resizing @@ -516,7 +441,7 @@ class SliderBase extends UI5Element { if (!focusedElement || focusedElement !== event.target) { this._preserveFocus(true); - this._focusInnerElement(); + this.focusInnerElement(); } } From e37920a201b1cdb840b13cf72b75f02141703e64 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Sun, 10 Jan 2021 01:57:44 +0200 Subject: [PATCH 14/20] Improve code based on the code reviews, fix keyboard handling bugs -- onEnterDom() moved from Slider.js to SliderBase.js -- A couple of keyboard handling rare cases are fixed -- The private 'focus' property is removed as it is no longer used -- Keyboard interactions with inactive Slider (with step 0) are prevented -- Refactoring of the logic about actions onto the selected range --- packages/main/src/RangeSlider.js | 71 ++++++++++++-------- packages/main/src/Slider.hbs | 8 ++- packages/main/src/Slider.js | 19 ++---- packages/main/src/SliderBase.js | 19 +++--- packages/main/test/specs/RangeSlider.spec.js | 14 +--- packages/main/test/specs/Slider.spec.js | 5 +- 6 files changed, 69 insertions(+), 67 deletions(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index ee45748b31a8..8b9d1787de74 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -5,7 +5,6 @@ import { isHome, isEnd, } from "@ui5/webcomponents-base/dist/Keys.js"; -import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import SliderBase from "./SliderBase.js"; import RangeSliderTemplate from "./generated/templates/RangeSliderTemplate.lit.js"; @@ -103,14 +102,6 @@ class RangeSlider extends SliderBase { this.i18nBundle = getI18nBundle("@ui5/webcomponents"); } - onEnterDOM() { - this._sliderStartHandle = this.shadowRoot.querySelector(".ui5-slider-handle--start"); - this._sliderEndHandle = this.shadowRoot.querySelector(".ui5-slider-handle--end"); - this._sliderProgress = this.shadowRoot.querySelector(".ui5-slider-progress"); - - ResizeHandler.register(this, this._resizeHandler); - } - get tooltipStartValue() { const stepPrecision = this.constructor._getDecimalPrecisionOfNumber(this._effectiveStep); return this.startValue.toFixed(stepPrecision); @@ -142,8 +133,6 @@ class RangeSlider extends SliderBase { } _onfocusin(event) { - this.focused = true; - // If this is the initial focusin of the component save its initial // value properties so they could be restored on ESC key press if (!this._getInitialValue("endValue")) { @@ -168,7 +157,6 @@ class RangeSlider extends SliderBase { if (this._isFocusing()) { this._preventFocusOut(); } else { - this.focused = false; this._setAffectedValue(null); this._setInitialValue("startValue", null); this._setInitialValue("endValue", null); @@ -219,7 +207,7 @@ class RangeSlider extends SliderBase { } // Update a single value if one of the handles is focused or the range if not already at min or max - if (affectedValue && !this._inCurrentRange) { + if (affectedValue && !this._isPressInCurrentRange) { const newValue = this.constructor.clipValue(newValueOffset + this[affectedValue], min, max); this.update(affectedValue, newValue, null); } else if ((newValueOffset < 0 && this.startValue > min) || (newValueOffset > 0 && this.endValue < max)) { @@ -236,17 +224,19 @@ class RangeSlider extends SliderBase { * @private */ _setAffectedValueByFocusedElement() { - if (this.shadowRoot.activeElement === this._sliderStartHandle) { + if (this.shadowRoot.activeElement === this._startHandle) { this._setAffectedValue("startValue"); } - if (this.shadowRoot.activeElement === this._sliderEndHandle) { + if (this.shadowRoot.activeElement === this._endHandle) { this._setAffectedValue("endValue"); } - if (this.shadowRoot.activeElement === this._sliderProgress) { - this._inCurrentRange = true; + if (this.shadowRoot.activeElement === this._progressBar) { + this._setAffectedValue(null); } + + this._setIsPressInCurrentRange(!this._getAffectedValue); } /** @@ -301,7 +291,7 @@ class RangeSlider extends SliderBase { this._saveInteractionStartData(event, newValue); // Do not yet update the RangeSlider if press is in range or over a handle. - if (this._inCurrentRange || this._handeIsPressed) { + if (this._isPressInCurrentRange || this._handeIsPressed) { this._handeIsPressed = false; return; } @@ -328,8 +318,6 @@ class RangeSlider extends SliderBase { this._startValueAtBeginningOfAction = this.startValue; this._endValueAtBeginningOfAction = this.endValue; - // Check if the new value is in the current select range of values - this._inCurrentRange = newValue > this._startValueAtBeginningOfAction && newValue < this._endValueAtBeginningOfAction; // Save the initial press point coordinates (position) this._initialPageXPosition = this.constructor.getPageXValueFromEvent(event); // Which element of the Range Slider is pressed and which value property to be modified on further interaction @@ -353,7 +341,7 @@ class RangeSlider extends SliderBase { } // Update UI and state when dragging a single Range Slider handle - if (!this._inCurrentRange) { + if (!this._isPressInCurrentRange) { this._updateValueOnHandleDrag(event); return; } @@ -400,7 +388,7 @@ class RangeSlider extends SliderBase { this._startValueAtBeginningOfAction = null; this._endValueAtBeginningOfAction = null; - this._inCurrentRange = null; + this._setIsPressInCurrentRange(false); this.handleUpBase(); } @@ -431,7 +419,6 @@ class RangeSlider extends SliderBase { // Remove the flag for value in current range if the press action is over one of the handles if (inHandleEndDom || inHandleStartDom) { - this._inCurrentRange = false; this._handeIsPressed = true; } @@ -444,6 +431,10 @@ class RangeSlider extends SliderBase { if (inHandleStartDom || value < this.startValue) { this._setAffectedValue("startValue"); } + + // Flag if press is in the current select range + const isNewValueInCurrentRange = value > this._startValueAtBeginningOfAction && value < this._endValueAtBeginningOfAction; + this._setIsPressInCurrentRange(!(this._getAffectedValue() || this._handeIsPressed) ? isNewValueInCurrentRange : false); } /** @@ -468,6 +459,20 @@ class RangeSlider extends SliderBase { return this._valueAffected; } + /** + * Flag if press action is made on the currently selected range of values + * + * @param {Boolean} isPressInCurrentRange Did the current press action occur in the current range (between the two handles) + * @private + */ + _setIsPressInCurrentRange(isPressInCurrentRange) { + this._isPressInCurrentRange = isPressInCurrentRange; + } + + _isPressInCurrentRange() { + return this._isPressInCurrentRange; + } + /** * Manage the focus between the focusable inner elements within the component. * @@ -497,16 +502,16 @@ class RangeSlider extends SliderBase { const isReversed = this._areValuesReversed(); const affectedValue = this._getAffectedValue(); - if (this._inCurrentRange || !affectedValue) { - this._sliderProgress.focus(); + if (this._isPressInCurrentRange || !affectedValue) { + this._progressBar.focus(); } if ((affectedValue === "startValue" && !isReversed) || (affectedValue === "endValue" && isReversed)) { - this._sliderStartHandle.focus(); + this._startHandle.focus(); } if ((affectedValue === "endValue" && !isReversed) || (affectedValue === "startValue" && isReversed)) { - this._sliderEndHandle.focus(); + this._endHandle.focus(); } } @@ -662,6 +667,18 @@ class RangeSlider extends SliderBase { return this._reversedValues; } + get _startHandle() { + return this.shadowRoot.querySelector(".ui5-slider-handle--start"); + } + + get _endHandle() { + return this.shadowRoot.querySelector(".ui5-slider-handle--end"); + } + + get _progressBar() { + return this.shadowRoot.querySelector(".ui5-slider-progress"); + } + get tabIndexProgress() { return this.tabIndex; } diff --git a/packages/main/src/Slider.hbs b/packages/main/src/Slider.hbs index 819750256d07..e2d9e62937ad 100644 --- a/packages/main/src/Slider.hbs +++ b/packages/main/src/Slider.hbs @@ -1,7 +1,13 @@ {{>include "./SliderBase.hbs"}} {{#*inline "handles"}} -
+
{{#if showTooltip}}
{{tooltipValue}} diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index 3cddb5de8ff0..57b725cd4806 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -1,8 +1,9 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import { fetchI18nBundle, getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js"; import { isEscape } from "@ui5/webcomponents-base/dist/Keys.js"; -import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import SliderBase from "./SliderBase.js"; + +// Template import SliderTemplate from "./generated/templates/SliderTemplate.lit.js"; /** @@ -86,11 +87,6 @@ class Slider extends SliderBase { this.i18nBundle = getI18nBundle("@ui5/webcomponents"); } - onEnterDOM() { - this._sliderHandle = this.shadowRoot.querySelector(".ui5-slider-handle"); - ResizeHandler.register(this, this._resizeHandler); - } - /** * * Check if the previously saved state is outdated. That would mean @@ -140,18 +136,12 @@ class Slider extends SliderBase { } } - focusInnerElement() { - this._sliderHandle.focus(); - } - _onfocusin(event) { // Set initial value if one is not set previously on focus in. // It will be restored if ESC key is pressed. if (this._getInitialValue("value") === null) { this._setInitialValue("value", this.value); } - - this.focused = true; } _onfocusout(event) { @@ -164,7 +154,6 @@ class Slider extends SliderBase { // Reset focus state and the stored Slider's initial // value that was saved when it was first focused in - this.focused = false; this._setInitialValue("value", null); } @@ -263,6 +252,10 @@ class Slider extends SliderBase { }; } + get _sliderHandle() { + return this.shadowRoot.querySelector(".ui5-slider-handle"); + } + get labelItems() { return this._labelItems; } diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 1f9b2c19737f..7b7a4869c26e 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -100,13 +100,6 @@ const metadata = { type: Boolean, }, - /** - * Indicates if the elements is on focus - * @private - */ - focused: { - type: Boolean, - }, /** * @private @@ -240,6 +233,10 @@ class SliderBase extends UI5Element { }; } + onEnterDOM() { + ResizeHandler.register(this, this._resizeHandler); + } + onExitDOM() { ResizeHandler.deregister(this, this._handleResize); } @@ -291,7 +288,7 @@ class SliderBase extends UI5Element { } _onkeydown(event) { - if (this.disabled) { + if (this.disabled || this._effectiveStep === 0) { return; } @@ -359,10 +356,12 @@ class SliderBase extends UI5Element { } /** - * This method is reserved for derived classes for managing the focus between the component's inner elements + * Manages the focus between the component's inner elements * @protected */ - focusInnerElement() {} + focusInnerElement() { + this.focus(); + } /** * Handle the responsiveness of the Slider's UI elements when resizing diff --git a/packages/main/test/specs/RangeSlider.spec.js b/packages/main/test/specs/RangeSlider.spec.js index 9d1231c97655..2361a7d4624c 100644 --- a/packages/main/test/specs/RangeSlider.spec.js +++ b/packages/main/test/specs/RangeSlider.spec.js @@ -264,7 +264,7 @@ describe("Testing events", () => { describe("Accessibility: Testing focus", () => { - it("Click anywhere in the Range Slider should focus the closest handle and set the 'focused' property to true", () => { + it("Click anywhere in the Range Slider should focus the closest handle", () => { browser.url("http://localhost:8080/test-resources/pages/RangeSlider.html"); const rangeSlider = browser.$("#basic-range-slider"); @@ -277,8 +277,6 @@ describe("Accessibility: Testing focus", () => { return document.getElementById("basic-range-slider").shadowRoot.activeElement; }); - assert.strictEqual(rangeSlider.isFocused(), true, "RangeSlider component is focused"); - assert.strictEqual(rangeSlider.getProperty("focused"), true, "RangeSlider state is focused"); assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderEndHandle.getAttribute("class"), "RangeSlider second handle has the shadowDom focus"); rangeSlider.setProperty("startValue", 30); @@ -291,7 +289,7 @@ describe("Accessibility: Testing focus", () => { assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderStartHandle.getAttribute("class"), "RangeSlider second handle has the shadowDom focus"); }); - it("Click currently selected range should focus it and set the 'focused' property to true", () => { + it("Click currently selected range should focus it", () => { browser.url("http://localhost:8080/test-resources/pages/RangeSlider.html"); const rangeSlider = browser.$("#basic-range-slider"); @@ -304,8 +302,6 @@ describe("Accessibility: Testing focus", () => { return document.getElementById("basic-range-slider").shadowRoot.activeElement; }); - assert.strictEqual(rangeSlider.isFocused(), true, "RangeSlider component is focused"); - assert.strictEqual(rangeSlider.getProperty("focused"), true, "RangeSlider state is focused"); assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderSelection.getAttribute("class"), "RangeSlider progress bar has the shadowDom focus"); }); @@ -323,7 +319,6 @@ describe("Accessibility: Testing focus", () => { }); assert.strictEqual(rangeSlider.isFocused(), true, "Range Slider component is focused"); - assert.strictEqual(rangeSlider.getProperty("focused"), true, "Range Slider is focused"); assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderSelection.getAttribute("class"), "Range Slider progress tracker has the shadowDom focus"); }); @@ -365,10 +360,8 @@ describe("Accessibility: Testing focus", () => { }); assert.strictEqual(currentRangeSlider.isFocused(), false, "First RangeSlider component is now not focused"); - assert.strictEqual(currentRangeSlider.getProperty("focused"), false, "First RangeSlider state is not focused"); assert.strictEqual(nextRangeSlider.isFocused(), true, "Next RangeSlider is focused"); - assert.strictEqual(nextRangeSlider.getProperty("focused"), true, "Next RangeSlider's state is focused"); assert.strictEqual($(innerFocusedElement).getAttribute("class"), rangeSliderSelection.getAttribute("class"), "Next Range Slider second handle has the shadowDom focus"); }); @@ -384,10 +377,8 @@ describe("Accessibility: Testing focus", () => { }); assert.strictEqual(currentRangeSlider.isFocused(), false, "First RangeSlider component is now not focused"); - assert.strictEqual(currentRangeSlider.getProperty("focused"), false, "First RangeSlider state is not focused"); assert.strictEqual(previousRangeSlider.isFocused(), true, "Slider component is focused"); - assert.strictEqual(previousRangeSlider.getProperty("focused"), true, "Slider is focused"); assert.strictEqual($(innerFocusedElement).getAttribute("class"), previousRangeSliderEndHandle.getAttribute("class"), "Previous Range Slider second handle now has the shadowDom focus"); }); @@ -423,7 +414,6 @@ describe("Accessibility: Testing focus", () => { browser.keys(["Shift", "Tab"]); assert.strictEqual(rangeSlider.isFocused(), false, "First RangeSlider component is now not focused"); - assert.strictEqual(rangeSlider.getProperty("focused"), false, "First RangeSlider state is not focused"); }); it("When one handle come across the other and the values are swapped the focus must be switched between the handles", () => { diff --git a/packages/main/test/specs/Slider.spec.js b/packages/main/test/specs/Slider.spec.js index 699a8c24d314..c4dcdaa7f010 100644 --- a/packages/main/test/specs/Slider.spec.js +++ b/packages/main/test/specs/Slider.spec.js @@ -186,7 +186,7 @@ describe("Testing events", () => { }); describe("Accessibility: Testing focus", () => { - it("Click anywhere in the Slider should focus the Slider's handle and set the 'focused' property to true", () => { + it("Click anywhere in the Slider should focus the Slider's handle", () => { browser.url("http://localhost:8080/test-resources/pages/Slider.html"); const slider = browser.$("#basic-slider"); @@ -199,7 +199,6 @@ describe("Accessibility: Testing focus", () => { }); assert.strictEqual(slider.isFocused(), true, "Slider component is focused"); - assert.strictEqual(slider.getProperty("focused"), true, "Slider state is focused"); assert.strictEqual($(innerFocusedElement).getAttribute("class"), sliderHandle.getAttribute("class"), "Slider handle has the shadowDom focus"); }); @@ -214,7 +213,6 @@ describe("Accessibility: Testing focus", () => { }); assert.strictEqual(slider.isFocused(), true, "Slider component is focused"); - assert.strictEqual(slider.getProperty("focused"), true, "Slider is focused"); assert.strictEqual($(innerFocusedElement).getAttribute("class"), sliderHandle.getAttribute("class"), "Slider handle has the shadowDom focus"); }); @@ -229,7 +227,6 @@ describe("Accessibility: Testing focus", () => { }); assert.strictEqual(slider.isFocused(), true, "Slider component is focused"); - assert.strictEqual(slider.getProperty("focused"), true, "Slider is focused"); assert.strictEqual($(innerFocusedElement).getAttribute("class"), sliderHandle.getAttribute("class"), "Slider handle has the shadowDom focus"); }); }); From 1154dc5af8c3096e43c268227d9d42736c34d22e Mon Sep 17 00:00:00 2001 From: ndeshev Date: Mon, 11 Jan 2021 19:50:20 +0200 Subject: [PATCH 15/20] Fix merge error --- packages/main/src/RangeSlider.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index 525655c42bb4..8b9d1787de74 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -102,10 +102,6 @@ class RangeSlider extends SliderBase { this.i18nBundle = getI18nBundle("@ui5/webcomponents"); } - onEnterDOM() { - ResizeHandler.register(this, this._resizeHandler); - } - get tooltipStartValue() { const stepPrecision = this.constructor._getDecimalPrecisionOfNumber(this._effectiveStep); return this.startValue.toFixed(stepPrecision); From 171dcf543269b505d605ff277bc8491bceced6b8 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 12 Jan 2021 16:40:17 +0200 Subject: [PATCH 16/20] Fix click in a range selection close to handle bug, revert back the transparent background of the handles --- packages/main/src/RangeSlider.js | 2 +- packages/main/src/themes/SliderBase.css | 9 +++++++++ packages/main/src/themes/base/SliderBase-parameters.css | 2 ++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index 8b9d1787de74..8294ce8c19e0 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -433,7 +433,7 @@ class RangeSlider extends SliderBase { } // Flag if press is in the current select range - const isNewValueInCurrentRange = value > this._startValueAtBeginningOfAction && value < this._endValueAtBeginningOfAction; + const isNewValueInCurrentRange = value >= this._startValueAtBeginningOfAction && value <= this._endValueAtBeginningOfAction; this._setIsPressInCurrentRange(!(this._getAffectedValue() || this._handeIsPressed) ? isNewValueInCurrentRange : false); } diff --git a/packages/main/src/themes/SliderBase.css b/packages/main/src/themes/SliderBase.css index 05f1d6ea501c..7a4d4d641b68 100644 --- a/packages/main/src/themes/SliderBase.css +++ b/packages/main/src/themes/SliderBase.css @@ -74,6 +74,10 @@ width: var(--_ui5_slider_handle_width); } +.ui5-slider-handle--start, .ui5-slider-handle--end { + background: var(--_ui5_range_slider_handle_background); +} + .ui5-slider-handle:focus { outline: var(--_ui5_slider_handle_outline); outline-offset: var(--_ui5_slider_handle_outline_offset); @@ -89,6 +93,11 @@ border-color: var(--_ui5_slider_handle_hover_border); } +.ui5-slider-root:hover .ui5-slider-handle--start, .ui5-slider-root:hover .ui5-slider-handle--end, +.ui5-slider-root:active .ui5-slider-handle--start, .ui5-slider-root:active .ui5-slider-handle--end, +.ui5-slider-handle--start:active, .ui5-slider-handle--end:active { + background: var(--_ui5_range_slider_handle_hover_background); +} .ui5-slider-tooltip { text-align: center; diff --git a/packages/main/src/themes/base/SliderBase-parameters.css b/packages/main/src/themes/base/SliderBase-parameters.css index 46a2d131e2f0..b69926826e99 100644 --- a/packages/main/src/themes/base/SliderBase-parameters.css +++ b/packages/main/src/themes/base/SliderBase-parameters.css @@ -9,12 +9,14 @@ --_ui5_slider_handle_width: 1.625rem; --_ui5_slider_handle_border: solid 0.125rem var(--sapField_BorderColor); --_ui5_slider_handle_background: var(--sapButton_Background); + --_ui5_range_slider_handle_background: rgba(var(--sapButton_Background), 0.25); --_ui5_slider_handle_top: -0.825rem; --_ui5_slider_handle_margin_left: -0.9725rem; --_ui5_slider_handle_hover_background: var(--sapButton_Hover_Background); --_ui5_slider_handle_hover_border: var(--sapButton_Hover_BorderColor); --_ui5_slider_handle_outline: 1px dotted var(--sapContent_FocusColor); --_ui5_slider_handle_outline_offset: 0.075rem; + --_ui5_range_slider_handle_hover_background: rgba(var(--sapButton_Background), 0.25); --_ui5_slider_progress_outline: 0.0625rem dotted var(--sapContent_FocusColor); --_ui5_slider_progress_outline_offset: 0.525rem; --_ui5_slider_tickmark_color: #89919a; From 601e06bfd4255c056e3e4a3fcd9d7c0a9cece868 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Mon, 18 Jan 2021 16:25:57 +0200 Subject: [PATCH 17/20] Fix code reviews proposals --- packages/main/src/RangeSlider.js | 35 +++++++++++++++++++------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index 8294ce8c19e0..3770ad6529e5 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -95,6 +95,13 @@ class RangeSlider extends SliderBase { return RangeSliderTemplate; } + static get VALUES() { + return { + start: "startValue", + end: "endValue", + }; + } + constructor() { super(); this._stateStorage.startValue = null; @@ -171,7 +178,7 @@ class RangeSlider extends SliderBase { * @private */ _onkeyup(event) { - SliderBase.prototype._onkeyup.call(this, event); + super._onkeyup(event); this._swapValues(); this._setAffectedValue(null); @@ -225,11 +232,11 @@ class RangeSlider extends SliderBase { */ _setAffectedValueByFocusedElement() { if (this.shadowRoot.activeElement === this._startHandle) { - this._setAffectedValue("startValue"); + this._setAffectedValue(this.VALUES.start); } if (this.shadowRoot.activeElement === this._endHandle) { - this._setAffectedValue("endValue"); + this._setAffectedValue(this.VALUES.end); } if (this.shadowRoot.activeElement === this._progressBar) { @@ -424,12 +431,12 @@ class RangeSlider extends SliderBase { // Return that handle that is closer to the press point if (inHandleEndDom || value > this.endValue) { - this._setAffectedValue("endValue"); + this._setAffectedValue(this.VALUES.end); } // If one of the handle is pressed return that one if (inHandleStartDom || value < this.startValue) { - this._setAffectedValue("startValue"); + this._setAffectedValue(this.VALUES.start); } // Flag if press is in the current select range @@ -506,11 +513,11 @@ class RangeSlider extends SliderBase { this._progressBar.focus(); } - if ((affectedValue === "startValue" && !isReversed) || (affectedValue === "endValue" && isReversed)) { + if ((affectedValue === this.VALUES.start && !isReversed) || (affectedValue === this.VALUES.end && isReversed)) { this._startHandle.focus(); } - if ((affectedValue === "endValue" && !isReversed) || (affectedValue === "startValue" && isReversed)) { + if ((affectedValue === this.VALUES.end && !isReversed) || (affectedValue === this.VALUES.start && isReversed)) { this._endHandle.focus(); } } @@ -603,10 +610,10 @@ class RangeSlider extends SliderBase { // The value according to which we update the UI can be either the startValue // or the endValue property. It is determined in _getClosestHandle() // depending on to which handle is closer the user interaction. - if (affectedValue === "startValue") { + if (affectedValue === this.VALUES.start) { this._selectedRange = (prevEndValue - newValue) / (max - min); this._firstHandlePositionFromStart = ((newValue - min) / (max - min)) * 100; - } else if (affectedValue === "endValue") { + } else if (affectedValue === this.VALUES.end) { this._selectedRange = ((newValue - prevStartValue)) / (max - min); this._secondHandlePositionFromStart = (newValue - min) / (max - min) * 100; } else { @@ -632,7 +639,7 @@ class RangeSlider extends SliderBase { _swapValues() { const affectedValue = this._getAffectedValue(); - if (affectedValue === "startValue" && this.startValue > this.endValue) { + if (affectedValue === this.VALUES.start && this.startValue > this.endValue) { const prevEndValue = this.endValue; this.endValue = this.startValue; this.startValue = prevEndValue; @@ -641,7 +648,7 @@ class RangeSlider extends SliderBase { this.focusInnerElement(); } - if (affectedValue === "endValue" && this.endValue < this.startValue) { + if (affectedValue === this.VALUES.end && this.endValue < this.startValue) { const prevStartValue = this.startValue; this.startValue = this.endValue; this.endValue = prevStartValue; @@ -668,15 +675,15 @@ class RangeSlider extends SliderBase { } get _startHandle() { - return this.shadowRoot.querySelector(".ui5-slider-handle--start"); + return this.getDomRef().querySelector(".ui5-slider-handle--start"); } get _endHandle() { - return this.shadowRoot.querySelector(".ui5-slider-handle--end"); + return this.getDomRef().querySelector(".ui5-slider-handle--end"); } get _progressBar() { - return this.shadowRoot.querySelector(".ui5-slider-progress"); + return this.getDomRef().querySelector(".ui5-slider-progress"); } get tabIndexProgress() { From f2a61783b722f300bc80d0872414e8adb4d03cab Mon Sep 17 00:00:00 2001 From: ndeshev Date: Mon, 18 Jan 2021 18:53:09 +0200 Subject: [PATCH 18/20] Fix calling of the static 'VALUES' getter --- packages/main/src/RangeSlider.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index 3770ad6529e5..6074aa0c6e48 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -232,11 +232,11 @@ class RangeSlider extends SliderBase { */ _setAffectedValueByFocusedElement() { if (this.shadowRoot.activeElement === this._startHandle) { - this._setAffectedValue(this.VALUES.start); + this._setAffectedValue(RangeSlider.VALUES.start); } if (this.shadowRoot.activeElement === this._endHandle) { - this._setAffectedValue(this.VALUES.end); + this._setAffectedValue(RangeSlider.VALUES.end); } if (this.shadowRoot.activeElement === this._progressBar) { @@ -431,12 +431,12 @@ class RangeSlider extends SliderBase { // Return that handle that is closer to the press point if (inHandleEndDom || value > this.endValue) { - this._setAffectedValue(this.VALUES.end); + this._setAffectedValue(RangeSlider.VALUES.end); } // If one of the handle is pressed return that one if (inHandleStartDom || value < this.startValue) { - this._setAffectedValue(this.VALUES.start); + this._setAffectedValue(RangeSlider.VALUES.start); } // Flag if press is in the current select range @@ -513,11 +513,11 @@ class RangeSlider extends SliderBase { this._progressBar.focus(); } - if ((affectedValue === this.VALUES.start && !isReversed) || (affectedValue === this.VALUES.end && isReversed)) { + if ((affectedValue === RangeSlider.VALUES.start && !isReversed) || (affectedValue === RangeSlider.VALUES.end && isReversed)) { this._startHandle.focus(); } - if ((affectedValue === this.VALUES.end && !isReversed) || (affectedValue === this.VALUES.start && isReversed)) { + if ((affectedValue === RangeSlider.VALUES.end && !isReversed) || (affectedValue === RangeSlider.VALUES.start && isReversed)) { this._endHandle.focus(); } } @@ -610,10 +610,10 @@ class RangeSlider extends SliderBase { // The value according to which we update the UI can be either the startValue // or the endValue property. It is determined in _getClosestHandle() // depending on to which handle is closer the user interaction. - if (affectedValue === this.VALUES.start) { + if (affectedValue === RangeSlider.VALUES.start) { this._selectedRange = (prevEndValue - newValue) / (max - min); this._firstHandlePositionFromStart = ((newValue - min) / (max - min)) * 100; - } else if (affectedValue === this.VALUES.end) { + } else if (affectedValue === RangeSlider.VALUES.end) { this._selectedRange = ((newValue - prevStartValue)) / (max - min); this._secondHandlePositionFromStart = (newValue - min) / (max - min) * 100; } else { @@ -639,7 +639,7 @@ class RangeSlider extends SliderBase { _swapValues() { const affectedValue = this._getAffectedValue(); - if (affectedValue === this.VALUES.start && this.startValue > this.endValue) { + if (affectedValue === RangeSlider.VALUES.start && this.startValue > this.endValue) { const prevEndValue = this.endValue; this.endValue = this.startValue; this.startValue = prevEndValue; @@ -648,7 +648,7 @@ class RangeSlider extends SliderBase { this.focusInnerElement(); } - if (affectedValue === this.VALUES.end && this.endValue < this.startValue) { + if (affectedValue === RangeSlider.VALUES.end && this.endValue < this.startValue) { const prevStartValue = this.startValue; this.startValue = this.endValue; this.endValue = prevStartValue; From 4947b1fbe812bbc5275e0d028145c2a2ded0bc76 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Wed, 20 Jan 2021 08:35:06 +0200 Subject: [PATCH 19/20] Remove getters for private props --- packages/main/src/RangeSlider.js | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index 6074aa0c6e48..cc030a96bf95 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -195,7 +195,7 @@ class RangeSlider extends SliderBase { const min = this._effectiveMin; const max = this._effectiveMax; - const affectedValue = this._getAffectedValue(); + const affectedValue = this._valueAffected; // If home/end key is pressed and no single handle is focused the active element // is the range selection - update both start and end values. Otherwise, if 'home' @@ -304,7 +304,7 @@ class RangeSlider extends SliderBase { } // Update Slider UI and internal state - this.update(this._getAffectedValue(), newValue, null); + this.update(this._valueAffected, newValue, null); } @@ -364,7 +364,7 @@ class RangeSlider extends SliderBase { */ _updateValueOnHandleDrag(event) { const newValue = this.constructor.getValueFromInteraction(event, this._effectiveStep, this._effectiveMin, this._effectiveMax, this.getBoundingClientRect(), this.directionStart); - this.update(this._getAffectedValue(), newValue, null); + this.update(this._valueAffected, newValue, null); } /** @@ -441,7 +441,7 @@ class RangeSlider extends SliderBase { // Flag if press is in the current select range const isNewValueInCurrentRange = value >= this._startValueAtBeginningOfAction && value <= this._endValueAtBeginningOfAction; - this._setIsPressInCurrentRange(!(this._getAffectedValue() || this._handeIsPressed) ? isNewValueInCurrentRange : false); + this._setIsPressInCurrentRange(!(this._valueAffected || this._handeIsPressed) ? isNewValueInCurrentRange : false); } /** @@ -462,10 +462,6 @@ class RangeSlider extends SliderBase { } } - _getAffectedValue() { - return this._valueAffected; - } - /** * Flag if press action is made on the currently selected range of values * @@ -476,10 +472,6 @@ class RangeSlider extends SliderBase { this._isPressInCurrentRange = isPressInCurrentRange; } - _isPressInCurrentRange() { - return this._isPressInCurrentRange; - } - /** * Manage the focus between the focusable inner elements within the component. * @@ -507,7 +499,7 @@ class RangeSlider extends SliderBase { */ focusInnerElement() { const isReversed = this._areValuesReversed(); - const affectedValue = this._getAffectedValue(); + const affectedValue = this._valueAffected; if (this._isPressInCurrentRange || !affectedValue) { this._progressBar.focus(); @@ -605,7 +597,7 @@ class RangeSlider extends SliderBase { const min = this._effectiveMin; const prevStartValue = this.getStoredPropertyState("startValue"); const prevEndValue = this.getStoredPropertyState("endValue"); - const affectedValue = this._getAffectedValue(); + const affectedValue = this._valueAffected; // The value according to which we update the UI can be either the startValue // or the endValue property. It is determined in _getClosestHandle() @@ -637,7 +629,7 @@ class RangeSlider extends SliderBase { * @private */ _swapValues() { - const affectedValue = this._getAffectedValue(); + const affectedValue = this._valueAffected; if (affectedValue === RangeSlider.VALUES.start && this.startValue > this.endValue) { const prevEndValue = this.endValue; From e6e073d2559d6e8e5818adf7341d0bb6e6fa1f7a Mon Sep 17 00:00:00 2001 From: ndeshev Date: Wed, 20 Jan 2021 11:22:02 +0200 Subject: [PATCH 20/20] Fix the bug in _setAfffectedValueByFocusedElement() introduced in the previous commit --- packages/main/src/RangeSlider.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index cc030a96bf95..886e3b00aa13 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -243,7 +243,7 @@ class RangeSlider extends SliderBase { this._setAffectedValue(null); } - this._setIsPressInCurrentRange(!this._getAffectedValue); + this._setIsPressInCurrentRange(!this._valueAffected); } /**