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

React Events: add onFocusVisibleChange to Focus #15516

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

necolas
Copy link
Contributor

@necolas necolas commented Apr 26, 2019

Called when focus visibility changes. Focus is only considered visible if a
focus event occurs after keyboard navigation. This provides a way for people to
provide visual focus styles for keyboard accessible UIs without those styles
appearing if focus is triggered by mouse, touch, pen.

Still working on incorporating a couple more details around when focus is considered visible.

Ref #15257

@necolas necolas requested a review from trueadm April 26, 2019 21:45
@necolas necolas force-pushed the react-events/focus-visible branch 2 times, most recently from efb1149 to dcee5f8 Compare April 26, 2019 21:53
nativeEvent.key === 'Tab' &&
!(nativeEvent.metaKey || nativeEvent.altKey || nativeEvent.ctrlKey)
) {
isGlobalFocusVisible = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's probably not ideal to have global focus state being checked / modified in every instance. Should this be handled by a module outside of Flare (using native APIs) that we query for global focus visibility info? Any other ideas?

Copy link
Contributor

@trueadm trueadm Apr 26, 2019

Choose a reason for hiding this comment

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

We could add a context.isEventComponentWithinResponderScope or context.isResponderTopLevel API that checks if a parent event component in the tree has the same responder and bail-out of this logic?

@sizebot
Copy link

sizebot commented Apr 26, 2019

Details of bundled changes.

Comparing: cc5a493...f2776df

react-events

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-events.production.min.js 0.0% 🔺+0.2% 682 B 682 B 431 B 432 B UMD_PROD
react-events.production.min.js 0.0% -0.3% 512 B 512 B 354 B 353 B NODE_PROD
react-events-Press.development.js 0.0% -0.0% 19.61 KB 19.61 KB 4.91 KB 4.9 KB UMD_DEV
react-events-Press.production.min.js 0.0% -0.0% 7.45 KB 7.45 KB 2.74 KB 2.74 KB UMD_PROD
react-events-Press.production.min.js 0.0% -0.0% 7.29 KB 7.29 KB 2.68 KB 2.68 KB NODE_PROD
react-events-Hover.development.js 0.0% -0.0% 9.31 KB 9.31 KB 2.32 KB 2.32 KB NODE_DEV
react-events-Hover.production.min.js 0.0% -0.1% 3.73 KB 3.73 KB 1.4 KB 1.4 KB NODE_PROD
react-events-Focus.development.js +57.2% +35.9% 4.24 KB 6.67 KB 1.34 KB 1.82 KB UMD_DEV
react-events-Focus.production.min.js 🔺+61.3% 🔺+40.6% 1.77 KB 2.86 KB 825 B 1.13 KB UMD_PROD
react-events-Focus.development.js +59.6% +36.7% 4.07 KB 6.5 KB 1.29 KB 1.76 KB NODE_DEV
react-events-Focus.production.min.js 🔺+67.8% 🔺+45.0% 1.6 KB 2.69 KB 755 B 1.07 KB NODE_PROD
ReactEventsFocus-dev.js +61.9% +38.7% 3.99 KB 6.47 KB 1.27 KB 1.75 KB FB_WWW_DEV
ReactEventsFocus-prod.js 🔺+75.5% 🔺+47.1% 3.05 KB 5.36 KB 935 B 1.34 KB FB_WWW_PROD
react-events-FocusScope.production.min.js 0.0% -0.1% 2.03 KB 2.03 KB 1007 B 1006 B UMD_PROD
react-events-FocusScope.development.js 0.0% -0.1% 4.9 KB 4.9 KB 1.53 KB 1.53 KB NODE_DEV
react-events-FocusScope.production.min.js 0.0% -0.1% 1.85 KB 1.85 KB 939 B 938 B NODE_PROD

Generated by 🚫 dangerJS

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

This is good for now but we need to optimize this better in a follow up.

Called when focus visibility changes. Focus is only considered visible if a
focus event occurs after keyboard navigation. This provides a way for people to
provide visual focus styles for keyboard accessible UIs without those styles
appearing if focus is triggered by mouse, touch, pen.
@necolas
Copy link
Contributor Author

necolas commented Apr 29, 2019

I noticed a couple of odd things happening while working on–and unrelated to changes in–this branch. First, the onRootEvent handler for Focus seems to be called twice for each keydown and keyup event. Second, in Safari pressing any of the Shift, Ctrl, Opt, Cmd keys while focused is causing mousemove events to be dispatched to Focus's onRootEvent handler. Third, the Hover component's behaviour has regressed and now it "flashes" the hover state on/off as you move the pointer through the boundaries of descendant elements.

https://fburl.com/anpad6mb

@necolas necolas merged commit 2cca187 into facebook:master Apr 29, 2019
@trueadm
Copy link
Contributor

trueadm commented Apr 29, 2019

@necolas This wasn't merged before the other PR that should have addressed the double event issue (the PR for using proper keys for events). I wonder if that fixes this issue. The other bugs sounds like yet another Safari thing we need to handle as an edge case. :/

@necolas necolas deleted the react-events/focus-visible branch May 3, 2019 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants