-
Notifications
You must be signed in to change notification settings - Fork 355
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
Autocomplete: reorganize modules and update to v2.1.8 #3064
Conversation
@crhallberg, after looking at @ThoWagen's work on #2952, where he significantly refactored the search box reset button to incorporate it with his virtual keyboard support, I begin to wonder why we need to focus the search box in order to make the reset button work. I'm pretty sure it's that added focus call that introduced the problem that @EreMaijala first observed. It's entirely possible that the autocomplete improvements made here are worthwhile and helpful, but I wonder if we should also consider introducing a simplified version of searchbox_controls.js from #2952 into this PR (or a new PR) in order to begin taking advantage of that refactoring, which seems to keep things working while eliminating the extra focus call. What does everyone think? I'd also be interested to hear @xmorave2's opinion, since he might have more insight into whether the focus is needed for something I'm overlooking. |
@demiankatz I agree that would be easier, but my understanding was that the focusing is not to activate the clear button, but as an autofocus functionality. I’ll give it a second look though, you may be right. |
@crhallberg, I don't think autofocus was intended -- I believe we had previously discussed the negative accessibility implications of autofocus and decided against it, but I just let the change through without realizing the consequences. |
I think you were right about the autofocus. I've addressed it in the latest commit. |
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.
Thanks, @crhallberg. There are a few issues noted below, but overall I think this is a step in the right direction! However, this also seems like more refactoring and changes than is strictly necessary to fix the problem at hand. I'm going to open a separate PR to just extract the bare minimum changes that I think are necessary to solve our issues in 9.1, and then we can target the rest of this to 10.0. Any objections?
Also, I'm encountering a test failure:
This test is passing on dev, so something has somehow gotten broken here. |
Okay, now that #3080 is merged, we need to resolve conflicts here when time permits. For the moment, finishing up the 9.1-related pull requests is a higher priority, but hopefully we can clean this up early in the 10.0 development process. |
@crhallberg, can you take another look at this when time permits? Is it worth trying to resolve conflicts here, or have things moved so much that we're better off closing and taking a fresh approach? I honestly can't remember what we were trying to accomplish here (apart from fixing the bug cited in the description, which is already fixed now). |
I merged in dev but that caused some of the de-jQuery work to be reverted. We can address that after merge. |
Thanks, @crhallberg. It looks like there's some unaddressed feedback from a previous review, if you have a chance to look at that too. Also, I'm pretty sure this PR needs to be renamed, because the autocomplete opening problem was solved elsewhere. Is this applying more generalized fixes/improvements now? |
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.
Thanks, @crhallberg -- I think we're getting there, but if I'm remembering correctly how this works, there may still be some issues related to the virtual keyboard implementation.
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.
Looks like some recent fixes to the virtual keyboard have introduced conflicts here... and it's probably not worth trying to resolve them until after #3250 is finished as well. After that, we should make sure that all of those functional improvements are not lost in the refactoring here.
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.
Now that #3250 has been merged, there are more conflicts to resolve. Can you sort it out when time permits, @crhallberg? Hopefully this gets the virtual keyboard logic into a stable place and we won't have to reconcile any further changes.
I've resolved the conflict but I'm seeing two issues that I'm investigating:
EDIT: I patched these errors but I'm unsure of the true cause. My top culprit is the virtual keyboard itself trying to sync the input based on the settings we're using. |
This is behaving all the ways I would expect - I cannot seem to break it. @ThoWagen, I would appreciate a look from you to double-check my refactoring. @sturkel89, if you have time, I wouldn't mind you trying to break it as well. Thank you! |
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.
Besides two small comments, I don't see any problems with this. I could not break the behavior as well. The virtual keyboard, the reset button and autocomplete are working well together.
The idea of _handleInputChange
was to have one method that can handle all of the events and behavior independent of what caused the change. But I'm not convinced myself that this is a better approach than this.
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.
Thanks, @ThoWagen, I agree that these things need to be fixed.
@crhallberg, can you make the necessary adjustments and then request a review from @sturkel89 to get some final hands-on testing of everything?
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.
Thanks, @crhallberg, I think this looks good now! All that's left to do is to mint the new release at the autocomplete.js repo and then update package.json here. Can you do that?
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.
Thanks, @crhallberg, looking good!
Separated from #2979.
TODO