Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WRR-1063: Add optional options.preventScroll parameter to focus function to prevent scroll by focus #3277

Merged
merged 7 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/spotlight/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@ The following is a curated list of changes in the Enact spotlight module, newest

## [unreleased]

### Added
MikyungKim marked this conversation as resolved.
Show resolved Hide resolved

- `spotlight` an optional `options.preventScroll` parameter to `focus` function to prevent scrolling by focus

### Changed

- Renamed `spotlight` an optional `containerOption` parameter of `focus` function to `options` to avoid misunderstandings
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this changelog necessary? Developers doesn't specify the parameter's name when they call Spotlight.focus.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought the same manner at first time but we already mentioned containerOption twice in changelogs. I wanted to remove this log honestly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, removed. At least both of us do not want it. ;)


### Fixed

- `spotlight` to not require `less` dependency
Expand Down
27 changes: 15 additions & 12 deletions packages/spotlight/src/spotlight.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@
}
}

function focusElement (elem, containerIds, fromPointer) {
function focusElement (elem, containerIds, fromPointer, preventScroll) {
if (!elem) {
return false;
}
Expand All @@ -214,7 +214,7 @@
return true;
}

const focusOptions = isWithinOverflowContainer(elem, containerIds) ? {preventScroll: true} : null;
const focusOptions = preventScroll || isWithinOverflowContainer(elem, containerIds) ? {preventScroll: true} : null;

let silentFocus = function () {
elem.focus(focusOptions);
Expand Down Expand Up @@ -750,16 +750,19 @@
* @param {String|Node} [elem] The spotlight ID or selector for either a spottable
* component or a spotlight container, or spottable node. If not supplied, the default
* container will be focused.
* @param {Object} [containerOption] The object including `enterTo` and `toOuterContainer`.
* It works when the first parameter `elem` is either a spotlight container ID or a spotlight container node.
* @param {('last-focused'|'default-element'|'topmost')} [containerOption.enterTo] Specifies preferred
* @param {Object} [options] The object including `enterTo`, `toOuterContainer`, and `preventScroll`.
* `enterTo` and `toOuterContainer` work when the first parameter `elem` is either
* a spotlight container ID or a spotlight container node.
* @param {('last-focused'|'default-element'|'topmost')} [options.enterTo] Specifies preferred
* `enterTo` configuration.
* @param {Boolean} [containerOption.toOuterContainer] If the proper target is not found, search one
* @param {Boolean} [options.toOuterContainer] If the proper target is not found, search one
* recursively to outer container.
* @param {Boolean} [options.preventScroll] Prevents the focused element from an automatic scrolling
* into view after focusing the element.
* @returns {Boolean} `true` if focus successful, `false` if not.
* @public
*/
focus: function (elem, containerOption = {}) {
focus: function (elem, options = {}) {
let target = elem;
let wasContainerId = false;
let currentContainerNode = null;
Expand All @@ -768,7 +771,7 @@
target = getTargetByContainer();
} else if (typeof elem === 'string') {
if (getContainerConfig(elem)) {
target = getTargetByContainer(elem, containerOption.enterTo);
target = getTargetByContainer(elem, options.enterTo);
wasContainerId = true;
currentContainerNode = getContainerNode(elem);
} else if (/^[\w\d-]+$/.test(elem)) {
Expand All @@ -778,14 +781,14 @@
target = getTargetBySelector(elem);
}
} else if (isContainer(elem)) {
target = getTargetByContainer(getContainerId(elem), containerOption.enterTo);
target = getTargetByContainer(getContainerId(elem), options.enterTo);
currentContainerNode = elem;
}

const nextContainerIds = getContainersForNode(target);
const nextContainerId = last(nextContainerIds);
if (isNavigable(target, nextContainerId, true)) {
const focused = focusElement(target, nextContainerIds);
const focused = focusElement(target, nextContainerIds, false, options.preventScroll);

if (!focused && wasContainerId) {
setLastContainer(elem);
Expand All @@ -798,11 +801,11 @@
setLastContainer(elem);
}

if (containerOption.toOuterContainer && currentContainerNode) {
if (options.toOuterContainer && currentContainerNode) {
const outerContainer = getContainersForNode(currentContainerNode.parentElement).pop();

if (outerContainer) {
return this.focus(outerContainer, containerOption);
return this.focus(outerContainer, options);

Check warning on line 808 in packages/spotlight/src/spotlight.js

View check run for this annotation

Codecov / codecov/patch

packages/spotlight/src/spotlight.js#L808

Added line #L808 was not covered by tests
}
}

Expand Down
10 changes: 10 additions & 0 deletions packages/spotlight/src/tests/spotlight-specs.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,16 @@ describe('Spotlight', () => {
expect(fn).toBeCalled();
}
));

test('should pass preventScroll option', testScenario(
scenarios.complexTree,
(root) => {
const fn = mockFocus(root.querySelector('[data-spotlight-id="s1"]'));
Spotlight.focus('s1', {preventScroll: true});

expect(fn).toHaveBeenCalledWith({preventScroll: true});
}
));
});

describe('#move', () => {
Expand Down
1 change: 0 additions & 1 deletion packages/ui/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1413,7 +1413,6 @@ libraries.

- Renamed `ui/Group` prop `select` to `childSelect` and added prop `select` to support selection types


## [1.0.0-alpha.2] - 2016-10-21

This version includes a lot of refactoring from the previous release. Developers need to switch to the new enact-dev command-line tool.
Expand Down