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

Change item list highlight color to white #51447

Merged
merged 2 commits into from
Sep 8, 2021

Conversation

eltank
Copy link
Contributor

@eltank eltank commented Sep 7, 2021

Summary

Interface "Change item list highlight color to white"

Purpose of change

Fixes #51443
Improve accessibility and UI consistency

Describe the solution

change a few lines in game::list_items for the color change
add a wmove call to position the cursor on the selected item for accessibility

Describe alternatives you've considered

Testing

Before:
image

After:
image

Additional context

I'm sure someone will complain because it's changing, but white-on-blue is what we use to highlight the "selected item" in the inventory UI and AIM, so this improves consistency.

@actual-nh actual-nh added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Accessibility Issues regarding accessibility Info / User Interface Game - player communication, menus, etc. labels Sep 7, 2021
@andrei8l
Copy link
Contributor

andrei8l commented Sep 7, 2021

Favorite items are also colored white

@dseguin
Copy link
Member

dseguin commented Sep 7, 2021

It looks like other elements on the panel are also highlighted in white ("Items", total count, etc). Would it be possible to highlight the selected item in yellow instead?

Edit: Just to clarify, because originally the user had issues distinguishing between different elements that used the same highlight.

@actual-nh
Copy link
Contributor

It looks like other elements on the panel are also highlighted in white ("Items", total count, etc). Would it be possible to highlight the selected item in yellow instead?

Edit: Just to clarify, because originally the user had issues distinguishing between different elements that used the same highlight.

Or other things such as those other elements + favorite items could be recolored (thus keeping things matching to the inventory and AIM UIs).

@andrei8l
Copy link
Contributor

andrei8l commented Sep 7, 2021

Or other things such as those other elements + favorite items could be recolored (thus keeping things matching to the inventory and AIM UIs).

Favorite items are colored white in inventory and AIM as well. This accessibility problem is supposedly solved by wmove() (see #39638) but my experience with that hasn't been great.

@eltank
Copy link
Contributor Author

eltank commented Sep 7, 2021

After taking a closer look at the inventory UI, I noticed that the selected item is white-on-blue. I changed the item list UI to match. This makes the selected item look different from favorite items.

@eltank
Copy link
Contributor Author

eltank commented Sep 7, 2021

I added a wmove call but I don't know if I'm doing it right. @Qrox, could you take a look?

@eltank eltank marked this pull request as draft September 7, 2021 03:41
@eltank
Copy link
Contributor Author

eltank commented Sep 7, 2021

Changed the wmove code with advice from @dseguin and https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/ACCESSIBILITY.md
Hopefully it works now.

@eltank eltank marked this pull request as ready for review September 7, 2021 04:13
@Qrox
Copy link
Contributor

Qrox commented Sep 7, 2021

I added a wmove call but I don't know if I'm doing it right. @Qrox, could you take a look?

It looks good to me, but you might want to ask someone who uses screen reader to test it.

@eltank
Copy link
Contributor Author

eltank commented Sep 7, 2021

I would test it myself, but I couldn't find any instructions on how to set up a screen reader for CDDA.
At this point the easiest way to verify might be to just get this merged and released into an experimental build, then ask for feedback from players using screen readers.

@actual-nh actual-nh added the PUBLIC TEST Come and try this! label Sep 8, 2021
@I-am-Erk I-am-Erk merged commit 5612551 into CleverRaven:master Sep 8, 2021
@eltank eltank deleted the itemlist_white branch September 8, 2021 05:56
@anothersimulacrum
Copy link
Member

To test wmoves, make a curses build, and change curs_set( 0 ) to curs_set( 2 ) https://linux.die.net/man/3/curs_set.

@eltank
Copy link
Contributor Author

eltank commented Sep 10, 2021

Hmm, so unfortunately the "info panel" display moves the cursor away from the item list, but it's still next to the title of the selected item (at the end). Not sure what a screen reader would do with that.

image

@Qrox
Copy link
Contributor

Qrox commented Sep 11, 2021

Maybe switch the refreshing order of w_item and w_item_info? If that does not work, call wmove after refreshing w_item_info and refreshing w_item again?

@eltank
Copy link
Contributor Author

eltank commented Sep 11, 2021

I could fix it in this instance, but this is a systemic issue. Now that I have a build that shows me the cursor, I can see that nearly all of the menus in CDDA put the cursor someplace nonsensical. Is it even helpful to position the cursor more appropriately?

Inventory - at the bottom
image

Advanced inventory - on the minimap
image

Main menu, Action menu - bottom option
image

The crafting and construction menus are the only ones that put the cursor on the selected item
image

And how do people read the message log? In the main game window the cursor is on the "@" symbol.

In order to improve accessibility I would need someone to tell me how to set up a screen reader for CDDA so that I could hear what they hear. I tried playing the game using Orca on linux with a curses build and the experience was absolutely horrible.

@andrei8l
Copy link
Contributor

andrei8l commented Sep 11, 2021

Those menus haven't been updated for screen reader support, at least according to the linked issue. For this case, you can just move the wmove() and wnoutrefresh() calls to the end of the on_redraw callback (below if (filter_type {...}). nope... sorry, wrong again

In my experience the screen reader didn't stop at window boundaries though so it wasn't very useful at all. That's possibly just a configuration issue though.

@eltank
Copy link
Contributor Author

eltank commented Sep 11, 2021

[We should probably be discussing this in an Issue like #26885 rather than in a merged PR thread but oh well]

When I was playing with Orca I also noticed that it reads "past" the end of what it should have read, and can't handle wrapped text at all, so while placing the cursor correctly will help, it really doesn't help that much overall.
Probably the best thing we can do for accessibility is to put TTS in the game engine. There seemed to be some willingness to add espeak-ng support from the core maintainers. I'm willing to write code that uses the library, but I don't have the necessary skills to actually add the library as a dependency. I do know a couple of contributors who do have those skills, so I'll try asking them when they're less busy.

Venera3 pushed a commit to Venera3/Cataclysm-DDA that referenced this pull request Sep 21, 2021
* Change item list highlight color to white

* Position the cursor on the selected item in the item list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Issues regarding accessibility <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. PUBLIC TEST Come and try this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility issue due to inconsistent UI in all-items panel
7 participants