Focus search input when modal shown #115
Conversation
This is a small hack for now. We should find a cleaner way to solve this issue. It works fow now :) Closes #78
|
Part of me feels like we should honor the site/theme keyboard shortcut, even though it's in conflict with our UI. This is the main problem with relying on keyboard shortcuts, it's always an arms race of sorts to ensure your code captures the event first. The approach with firing a timer off probably works for most cases, but it will effectively duplicate the key press event. If the theme/site handler for the existing key press has side effects other than focus, like showing up of a separate search modal or something, then the reader will get that UI and our search modal UI. That is, the use of a timer doesn't intercept the event, but duplicates it. I know you can manipulate previously registered event handlers, however I've never done this for third party code. Normally, you know the exact function you are trying to deregister. If you can find the existing handler, it's possible to wrap that handler and intercept the keypress event for just |
I'm not sure what this means. Can you expand on this?
Yeah, I've already hit this with Material for MkDocs, yeah. I'm not sure what to do in those cases.
I researched about this and I wasn't able to de-register event handlers from others. I'm tracking this case and the one from the previous paragraph in #25 I know the hack from this PR is not good, but I don't have anything better at the moment and I thought that at least it was better than the current behavior, but I know this is not the solution 🤷🏼 |
Just that we shouldn't try to replace the override keyboard shortcut. These shortcuts may not be there for any particular reason, but unfortunately the shortcuts might also be important for the site.
Ah yes, that has a UI there too. There really isn't a great answer here, as we don't control the website we're injecting the features into. I don't think this is the primary method we want for triggering this UI long term anyways though, so maybe this is okay. This, among other reasons like screen reader overlap, is why I'm generally -1 on implementing keyboard shortcuts (and I've been known to use qutebrowser!)
Yeah, I can only say it's possible when you have the instance of the function handy. Otherwise, I don't know how feasible deregistering is.
I might lean towards not doing anything magical or unexpected here. If the user wants our search/flyout/etc, they should disable whatever is interfering with the hotkeys. Long term, we should avoid relying on shortcuts to trigger our UI, but I understand this is just a short term solution to avoid UI that we haven't added yet. We could make a secondary keyboard shortcut available perhaps? ie |
Agreed. I imagine we can probably just document "How to disable the default search on Material for MkDocs" and "How to disable the default search on Sphinx" and other doctools giving people a chunk of code or similar and rely on that.
Regarding using or not using keyboard shortcuts at all, we have this conversation opened: #80. I was on the fence, but considering that GitHub makes extensive usage of them, I'm kind of convinced they are not bad.
Yes, even having a UI element this may be required. Well, at least right now that we are using an
Yes. I did some tests with |
|
Are we OK merging this half-baked solution for now? At least it solves the problem in our own theme. We can continue with the proper solution in the other issues that I linked. |
👍 This seems like a great place to start for now. If we do have to hack things in later to support themes/tools, I'd consider that work polish level adjustments for later in our roadmap. I bet we'll find better solutions in the meantime though.
Aye, though I'm generally -1, I still feel shortcuts are okay as an addition to addons. I just don't like focusing on this as a primary part of our UX as it's much harder to discover and use as a feature, especially for less technical users. As an alternative UX, it's usually okay though, as long as adaptive technologies and normal browser use aren't negatively affected.
Yup, modifiers make this more complicated. Keyboard support is a rabbit hole and there is a lot that goes into doing keyboard support well.
There are definitely some downsides to this approach, and I feel there is probably a more standard fix for the problem. The focus issue is almost certainly either an async code issue or a matter of CSS transitions throwing the timing off. I'd try to rule out async troubles first, as it seems the most likely. I believe we need to be using the Otherwise, I would have assumed that the Element.updated event is what we would have wanted to sync the event up correctly: https://lit.dev/docs/v1/components/lifecycle/#updated I'd give those a try first if you haven't already. |
Excellent! This was it 🚀 . In fact, in the documentation link you shared, there is the exact same case I need here 😅 . It works perfect now without any type of hack.
Man, I don't know if this is what you are thinking here but maybe the "keyboard shortcuts" is an addon on itself that people can enable/disable 🤯 . I'm now convinced this is the way to go here. I will open an issue for this. |
Boom! 💣 I implemented this idea in #118 |
Sphinx also catches It seems there is an option to disable it, |
|
This PR is ready to be merged 👍🏼 |
|
Excellent, I'm glad this approach worked out. I think your idea to split out keyboard shortcuts as it's own addon is an interesting one, definitely worth testing that more. But I like that we have a way to surface configuration of hotkeys already 👍 |
This is a small hack for now.
We should find a cleaner way to solve this issue.
It works fow now :)
Closes #78