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

factor ComboBoxListBox out of ComboBox #445

Closed
pixelzoom opened this issue Jan 15, 2019 · 10 comments
Closed

factor ComboBoxListBox out of ComboBox #445

pixelzoom opened this issue Jan 15, 2019 · 10 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 15, 2019

In #430, I was unable to factor "list box" out of ComboBox. After hours of working on this, I bailed. According to #314 (comment), the code is "not in an acceptable state". The a11y code is too intertwined with the implementation, not adequately documented, and I had no idea what I might be breaking.

Highly recommended to complete this refactor before further a11y work in #314.

FYI, the name ComboBoxListBox was chosen by consensus in #440.

Assigning to @ariel-phet to prioritize and assign.

@ariel-phet
Copy link

@zepumph can you give me some insight as to an estimate of how much time it will take to get this code into an acceptable state?

In discussing with @pixelzoom one option may well be to remove the current a11y implementation and start from scratch. He was not necessarily advocating for that approach, but said that it might be easier depending on where you feel the a11y design started and where your understanding of it is now. My understanding is the design is solid, but the implementation currently leaves a bit to be desired.

If this is a more appropriate question for @jessegreenberg please assign to him.

Either way, once you have an estimate, also let me know if there is time to work on this in the near future, and then assign back to me. Considering this UI element is fairly ubiquitous to sims, it seems fairly high priority to have it in good shape.

pixelzoom added a commit that referenced this issue Jan 15, 2019
@zepumph
Copy link
Member

zepumph commented Jan 15, 2019

I would say my estimate would be 2-3 days of work. Much of this would involve reworking the a11y implementation, and to me that feels fine. Again what is currently in place was a prototype, and should go through some refining no matter what.

There are 12 TODOs relating to the a11y code that will be worked on as part of this, and likely others too.

Finally it would be good to note that making ComboBox accessible and ready for production is in line with a11y goals too, because Molarity is one of the next sims to have description work done with it. I hope to work on ComboBox heavily in the next 2 weeks or so.

zepumph added a commit that referenced this issue Jan 15, 2019
@pixelzoom
Copy link
Contributor Author

I'd be happy to help with this. Since "what is currently in place was a prototype", my recommendation would be to first create a solid implementation, then add fresh a11y instrumentation to it.

@ariel-phet
Copy link

@zepumph sounds good....and glad to know this is all in line with current a11y goals.

Please schedule time to work with @pixelzoom when this work rises to the top of your work pile. If you are open to it, I would go with his suggestion of adding fresh a11y instrumentation.

pixelzoom added a commit that referenced this issue Jan 17, 2019
@pixelzoom
Copy link
Contributor Author

@zepumph I'm going to take a quick first pass at ComboBoxListBox.

pixelzoom added a commit that referenced this issue Jan 17, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit that referenced this issue Jan 17, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit that referenced this issue Jan 17, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 17, 2019

@zepumph I factored out ComboBoxListBox, it went pretty well. I was able to keep the existing a11y code, and I think everything is behaving correctly. Have a look, then let's collaborate on further work.

@pixelzoom
Copy link
Contributor Author

@zepumph After you've had a look at ComboBoxListBox, I think the next step would be to evaluate whether there's any code in ComboBox that belongs in either ComboBoxListBox or ComboBoxButton.

pixelzoom added a commit that referenced this issue Jan 19, 2019
pixelzoom added a commit that referenced this issue Jan 19, 2019
pixelzoom added a commit that referenced this issue Jan 20, 2019
pixelzoom added a commit that referenced this issue Jan 20, 2019
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 20, 2019

I noticed that KEY_ENTER selected items in the listbox, but does not pop down the listbox. In the above commit, I added a TODO to investigate.

When KEY_ENTER is pressed, I see hideList is called, but then showList is immediately called.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 20, 2019

I finally understand that setting focus should only be done in a11y-specific listeners that handle keystrokes. Fixed applied in a484df8 so that focus highlights don't appear when using the mouse.

@pixelzoom
Copy link
Contributor Author

@zepumph and I spent several hours cleaning up TODO items for this and other ComboBox issues. All TODO items related to this issue have been addressed or moved to other issues. So I'm comfortable with closing this issue. @zepumph if you agree, please close.

@pixelzoom pixelzoom removed their assignment Jan 23, 2019
@zepumph zepumph closed this as completed Jan 23, 2019
pixelzoom added a commit that referenced this issue Jan 24, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants