-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Move focus to trigger instead of simply blur()ing #419
Move focus to trigger instead of simply blur()ing #419
Conversation
Looking at the travis failure above...I almost suspect it's a problem with the test rather than with the code change? Or, to put another way: is the function in real world ever meant to execute without a trigger being present (e.g. triggered programmatically)? If so, I guess the code needs to change subtly just to test for the presence of |
Hey @patrickhlauke, thanks a lot for sending this pull request. When testing on iOS, I noticed that focus is not changing even on the user taps somewhere else. See this video comparing before and after this patch: http://cl.ly/kfSv #418 seems to fix the problem you mentioned and don't throw users back to the start of the page: http://cl.ly/kgAH Because of that, I'll close this PR for now, but feel free to open another one if needed. Thanks again! |
looking at the first video, that's a standard iOS behavior: focus, or rather |
see https://youtu.be/Ekq81Z1tqUo that illustrates the problem still being present. for iOS, there's no nice way to suppress the quirky iOS behavior (it's consistent with how iOS behaves... blur - the JS event - is similarly not fired unless tapping away to another focusable element, see https://patrickhlauke.github.io/touch/tests/results/#mobile-tablet-touchscreen-events). there's no clean way to suppress this iOS behavior AND to keep sane keyboard focus, as far as i'm aware. i'd personally say having the focus (and tooltip) remain visible in iOS is a small price to pay in order for these controls to not completely mess up keyboard users. unless you implement some other more neutral additional "target" (like an extra neutral element, like a |
Would any of this be an issue if the executeCommand didn't require the selection of text to work? The fact you are having to mess with focus state to copy something to clipboard feels pretty wonky. |
No @gauntface, this only happens because we need to perform a selection for execCommand to work. @patrickhlauke I think we'll need to update the site as well to prevent this from happening: |
"this" being the tooltip not clearing after focus is moved away from the buttons (and the tooltip showing again once the same button receives focus again)? without looking at the code, i'd guess that adding a blur listener that clears the tooltip should do the trick |
blur() results in a loss/reset of keyboard focus, meaning keyboard users usually get thrown right back to the start of the page. Instead, this moves focus back to the trigger (which had the focus when the trigger was activated). As with the proposed fix in #418 this obviously results in the focus styles (like the default outline, unless suppressed) being applied to the trigger (see the related PR for rationale and future fix using `:focus-ring`)
just force-pushed a tiny tweak (change to the preceding |
regarding the sticky tooltips - #420 |
Thanks @patrickhlauke! All merged and published! |
Good stuff :) I'll double-check tomorrow that everything's working as expected, but thanks for being amenable to the change in iOS behavior! (there are ways to work around it, but they're ugly and involve hacking specifically around iOS' quirks with additional JS and empty noop handlers to force event bubbling - see twbs/bootstrap#22426) |
Interesting, thanks for sharing! I appreciate you taking some precious time to work on this ;) |
blur() results in a loss/reset of keyboard focus, meaning keyboard users usually get thrown right back to the start of the page. Instead, this moves focus back to the trigger (which had the focus when the trigger was activated). As with the proposed fix in zenorocha#418 this obviously results in the focus styles (like the default outline, unless suppressed) being applied to the trigger (see the related PR for rationale and future fix using `:focus-ring`)
)" This reverts commit 2006ea7.
blur()
results in a loss/reset of keyboard focus, meaning keyboard users usually get thrown right back to the start of the page. Instead, this moves focus back to the trigger (which had the focus when the trigger was activated).As with the proposed fix in #418 this obviously results in the focus styles (like the default outline, unless suppressed) being applied to the trigger (see the related PR for rationale and future fix using
:focus-ring
)