From dfcf104dc28f4564b1c1e8bf0acbef573405ec8b Mon Sep 17 00:00:00 2001 From: ndeshev Date: Tue, 15 Dec 2020 20:26:08 +0200 Subject: [PATCH 01/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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 5418658abdf466e5f4cafe51a9c9b0c7197986fe Mon Sep 17 00:00:00 2001 From: ndeshev Date: Mon, 4 Jan 2021 18:27:15 +0200 Subject: [PATCH 10/14] Fix Range Slider's resize handler instantiation and failing test caused by it. add focusInnerElement() as a hook in SliderBase.js --- packages/main/src/RangeSlider.js | 7 +++++-- packages/main/src/Slider.js | 8 ++++---- packages/main/src/SliderBase.js | 10 ++++++++-- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/packages/main/src/RangeSlider.js b/packages/main/src/RangeSlider.js index c3815f472dc9..83e221f8aca5 100644 --- a/packages/main/src/RangeSlider.js +++ b/packages/main/src/RangeSlider.js @@ -1,8 +1,7 @@ import Float from "@ui5/webcomponents-base/dist/types/Float.js"; import { fetchI18nBundle, getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js"; +import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js"; import SliderBase from "./SliderBase.js"; - -// Template import RangeSliderTemplate from "./generated/templates/RangeSliderTemplate.lit.js"; /** @@ -99,6 +98,10 @@ 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); diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index 1abec00d8407..b7047fb14140 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -114,6 +114,10 @@ class Slider extends SliderBase { this._updateHandleAndProgress(this.value); } + focusInnerElement() { + this._sliderHandle.focus(); + } + /** * Called when the user starts interacting with the slider * @@ -142,10 +146,6 @@ 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. diff --git a/packages/main/src/SliderBase.js b/packages/main/src/SliderBase.js index 476688a9681b..1f9b2c19737f 100644 --- a/packages/main/src/SliderBase.js +++ b/packages/main/src/SliderBase.js @@ -355,9 +355,15 @@ class SliderBase extends UI5Element { * @private */ _preventFocusOut() { - this._focusInnerElement(); + this.focusInnerElement(); } + /** + * This method is reserved for derived classes for managing the focus between the component's inner elements + * @protected + */ + focusInnerElement() {} + /** * Handle the responsiveness of the Slider's UI elements when resizing * @@ -435,7 +441,7 @@ class SliderBase extends UI5Element { if (!focusedElement || focusedElement !== event.target) { this._preserveFocus(true); - this._focusInnerElement(); + this.focusInnerElement(); } } From 6f56f99ffeb84afb0340f198a8894bce66a06ed2 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Sat, 9 Jan 2021 18:25:22 +0200 Subject: [PATCH 11/14] Improve code based on the code reviews -- onEnterDom() moved from Slider.js to SliderBase.js -- The Slider's handle is now focused via UI5Element getFocusDomRef() -- The private 'focus' property is removed as it is no longer used -- Keyboard interactions with inactive Slider (with step 0) are prevented --- packages/main/src/Slider.hbs | 8 +++++++- packages/main/src/Slider.js | 13 ------------- packages/main/src/SliderBase.js | 19 +++++++++---------- 3 files changed, 16 insertions(+), 24 deletions(-) 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 b7047fb14140..8daeca90f0a9 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -1,7 +1,6 @@ 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 @@ -88,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 @@ -114,10 +108,6 @@ class Slider extends SliderBase { this._updateHandleAndProgress(this.value); } - focusInnerElement() { - this._sliderHandle.focus(); - } - /** * Called when the user starts interacting with the slider * @@ -152,8 +142,6 @@ class Slider extends SliderBase { if (this._getInitialValue("value") === null) { this._setInitialValue("value", this.value); } - - this.focused = true; } _onfocusout(event) { @@ -166,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); } 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 From cf7d29851faf297638c64bf79b191e5c94668baa Mon Sep 17 00:00:00 2001 From: ndeshev Date: Sat, 9 Jan 2021 18:40:53 +0200 Subject: [PATCH 12/14] Fix tests according to the latest changes --- packages/main/src/Slider.js | 5 +++++ packages/main/test/specs/Slider.spec.js | 3 --- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index 8daeca90f0a9..6a5796da2784 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -252,6 +252,11 @@ class Slider extends SliderBase { }; } + get _sliderHandle() { + return this.shadowRoot.querySelector(".ui5-slider-handle"); + + } + get labelItems() { return this._labelItems; } diff --git a/packages/main/test/specs/Slider.spec.js b/packages/main/test/specs/Slider.spec.js index 699a8c24d314..8e5f34724f3f 100644 --- a/packages/main/test/specs/Slider.spec.js +++ b/packages/main/test/specs/Slider.spec.js @@ -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 61d91871afd4cdd4c3c9bd851fb16eb1ffcfa85a Mon Sep 17 00:00:00 2001 From: ndeshev Date: Sat, 9 Jan 2021 18:40:53 +0200 Subject: [PATCH 13/14] Fix tests according to the latest changes --- packages/main/src/Slider.js | 4 ++++ packages/main/test/specs/Slider.spec.js | 3 --- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/main/src/Slider.js b/packages/main/src/Slider.js index 8daeca90f0a9..57b725cd4806 100644 --- a/packages/main/src/Slider.js +++ b/packages/main/src/Slider.js @@ -252,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/test/specs/Slider.spec.js b/packages/main/test/specs/Slider.spec.js index 699a8c24d314..8e5f34724f3f 100644 --- a/packages/main/test/specs/Slider.spec.js +++ b/packages/main/test/specs/Slider.spec.js @@ -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 bbccda0f3565516a3608dc075e0aef6996fa4320 Mon Sep 17 00:00:00 2001 From: ndeshev Date: Sat, 9 Jan 2021 18:43:54 +0200 Subject: [PATCH 14/14] Minor linting fixes --- packages/main/test/specs/Slider.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/main/test/specs/Slider.spec.js b/packages/main/test/specs/Slider.spec.js index 8e5f34724f3f..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");