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
[VUFIND-1654] Fix inconsistent cursor behavior with virtual keyboard. #3248
Conversation
(I should also note that, if it turns out that there really is a reason to do the focus under some circumstances, maybe skipping it during the first page initialization will be enough to fix the problem...) |
On problem with removing this is that after clicking the reset button one can't instantly type but has to manually focus the input box again. I'm not sure if there is another problem. But I didn't notice any. Unfortunately, I can't recall why exactly I added it. It was in this commit af20f74. It probably was supposed to fix the problem above. If there is no other reason then moving the line into |
Thanks, @ThoWagen, I can't think of a better explanation, and your proposed solution takes care of the reset button problem. @sturkel89, could you play with this a bit and see if you can find any new bad behaviors we're not accounting for? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the solution that's been put into place is that there is no longer a flashing cursor either before or after the searched text once you are viewing the results list.
- PHYSICAL KEYBOARD EXPERIENCE
If you are using a physical keyboard, you now need to click your mouse or otherwise activate the cursor in the desired location. If you don't, your typing does not appear. This seems to me to be a reasonable arrangement.
- VIRTUAL KEYBOARD EXPERIENCE
If you start typing on the virtual keyboard after typing a search and viewing results, your typed text is appended to the end of the existing text in the search box. You are n ot required to select a place to start typing; however, since there is no flashing cursor, so there is no indication to the user of what will happen.
In bootstrap3 and sandal, if the search string is more than a few words, the entered characters are hidden behind the reset button and the keyboard button, so the user cannot see what is happening.
If the browser window is narrow enough, the search box expands to fill the width of the window in all three themes. This provides visibility into what is happening, but is probably a less common view for most users.
RECOMMENDATION
I think it is a good change that there is no visible cursor on the results screen.
Is it possible to require the user to click in the search box and thereby create a flashing cursor before any kind of text entry is possible? I think that would be a reasonable solution and would offer consistency.
Once the user clicks in the search box, the text should remain right-justified at the point of the cursor so that the user can see what they're typing.
I'm assuming that PR 3247 will also help with this situation, as it will ensure that the end of the entered text string will always be visible and not hidden behind the reset and keyboard buttons.
Thanks, @sturkel89! The behavior in release 9.1 is that the search box does not auto-focus with the cursor after a user performs a search, so changing that behavior here actually restores us to the previous norm. I agree that this is desirable. Regarding the virtual keyboard behavior, since the virtual keyboard is solely linked to the search box, I wonder if we should auto-focus the search box when the virtual keyboard is visible to provide a clearer user experience. I think this would satisfy @sturkel89's suggestion in a logical way, but I'm not familiar enough with the virtual keyboard code to know if this is easy or potentially problematic. I'll wait and see what @ThoWagen thinks about this. If he likes the idea, we can decide whether to do the work here or open a separate PR. Maybe it would make sense to merge this first, then combine it with #3247, and then make further adjustments there, just so we can look at all the enhancements in a single place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving this branch so we can move on to #3250, Thanks!
This PR is intended to fix the problem described in VUFIND-1654, as reported by @sturkel89.
I suspect that the solution here is too simplistic, and that this focus call was here for a reason. But when I remove it, the problem goes away, and I can't see any other obvious issues, so maybe the fix is this simple. @ThoWagen, can you shed any light on this?
TODO: