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

Allow preventing scroll when an element is focused #5

Merged
merged 4 commits into from
Feb 20, 2024

Conversation

krksgbr
Copy link
Contributor

@krksgbr krksgbr commented Feb 20, 2024

I've added a data-focus-preventScroll attribute to enable specifying elements that should not initiate scrolling when they receive focus.

I opted for this instead of dispatching a custom event that can be prevented, because it seemed simpler. What do you think?

@krksgbr krksgbr requested a review from knuton February 20, 2024 12:36
@krksgbr
Copy link
Contributor Author

krksgbr commented Feb 20, 2024

Seems like cypress runs the test on an outdated version of webkit.
The preventScroll focus option has been supported for a while:

https://github.com/WebKit/WebKit/blame/68c99280815be0b90f3541350037619b33b0dd06/Source/WebCore/dom/FocusOptions.h#L45

I'm not sure if we can configure a newer webkit version.
A quick alternative would be to disable the test in webkit:

  if (Cypress.browser.name === 'webkit') {
    return
  }

https://docs.cypress.io/api/cypress-api/browser

@knuton
Copy link
Member

knuton commented Feb 20, 2024

The failing test says AssertionError: expected 0 to not equal 0, I think that implies that the browser does not scroll for the second test condition, where scrolling is expected?

@krksgbr
Copy link
Contributor Author

krksgbr commented Feb 20, 2024

The failing test says AssertionError: expected 0 to not equal 0, I think that implies that the browser does not scroll for the second test condition, where scrolling is expected?

In the meantime I've found out that in the case of webkit, the reported scrollX values are wrong. The tests run in an iframe, but cypress reports the scroll of the parent window. I'm exploring a workaround.

The window's scroll position doesn't change when the tests are run inside WebKit.
Use the document's scroll values to work around the problem.
Copy link
Member

@knuton knuton left a comment

Choose a reason for hiding this comment

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

OK, let's go with this. It may be simpler than triggering an event.

Can you add to the README?

We could consider allowing to mark an entire subtree as no-scroll, but I am not sure that would be needed. We could expand to that if the need becomes apparent.

index.js Outdated
@@ -258,7 +258,8 @@ function applyFocus(direction, origin, target) {
if (isGroup(target)) {
dispatchGroupFocus(direction, origin, target)
} else if ("focus" in target && typeof target.focus === "function") {
target.focus()
const preventScroll = target.hasAttribute("data-focus-preventScroll")
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer sticking to kebab-case.

Suggested change
const preventScroll = target.hasAttribute("data-focus-preventScroll")
const preventScroll = target.hasAttribute("data-focus-prevent-scroll")

@knuton knuton merged commit 7996fb7 into dividat:main Feb 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants