Allow dropdown inputs to be manually enabled for DnD questions for accessibility - #2159
Allow dropdown inputs to be manually enabled for DnD questions for accessibility #2159sjd210 wants to merge 53 commits into
Conversation
This reverts commit 602c7f7.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2159 +/- ##
==========================================
+ Coverage 43.20% 43.56% +0.35%
==========================================
Files 601 602 +1
Lines 25728 25796 +68
Branches 8566 8594 +28
==========================================
+ Hits 11116 11238 +122
+ Misses 14555 14501 -54
Partials 57 57 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nput-setting [VRT] Update baselines for feature/non-dragging-input-setting
…nput-setting [VRT] Update baselines for feature/non-dragging-input-setting
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…nput-setting [VRT] Update baselines for feature/non-dragging-input-setting
jacbn
left a comment
jacbn
left a comment
There was a problem hiding this comment.
This is looking really good! I have one comment here about keeping hooks inside /services but beyond that my only consideration here is where and how we display the toggle.
As we've discussed, perhaps the most important thing for accessibility across the site is that we are consistent in how we surface accessibility features, so that using a new question type does not feel completely alien to e.g. a screenreader user.
Having a toggle at the top (which may be skipped if the user jumps to a specific section via a hash anchor) for just this question type does feel a little specific, though I can see it having value, particularly for tablets. To this end, I'm very happy for this feature to exist in the accessibility settings (and as you mention, it can remain on the page for non-logged in users), but if our plans for accessibility on other question types do not include such a toggle, I think I'd prefer we improve the accessibility on Cloze by allowing click-to-select and click-to-drop, so that we remain consistent in that all questions can be answered accessibly without requiring a toggle elsewhere on the page.
I'd love to be able to ask an accessibility panel about how they feel about which method is most accessible, as I can only base this off being consistent with other questions... :/
There is no convenient place to put the toggle for xs screens, and they are already currently bound to dropdowns for DnD questions so there shouldn't be any harm in maintaining that as a hard override
…nput-setting [VRT] Update baselines for feature/non-dragging-input-setting
…nput-setting [VRT] Update baselines for feature/non-dragging-input-setting
|
The main changes since last review:
See commit messages for other small things. |
jacbn
left a comment
jacbn
left a comment
There was a problem hiding this comment.
I think this still needs some further refinement – I've listed a few points below. For the last one, as we both disagree with content's decision(s) on how the toggle should work / where it should be, maybe we need to better convince them otherwise? :/
| <StyledToggle | ||
| checked={dragAndDropEnabled} | ||
| falseLabel="Dropdown" | ||
| trueLabel="Drag and drop" | ||
| onChange={(e) => {toggleDragAndDropEnabled(); e.stopPropagation();}} | ||
| /> |
There was a problem hiding this comment.
We ought to consider improving the styling for this in dark mode – both are accessibility features and having both on is currently extremely illegible!
1: Input mode toggle
2: Drop zone button replacement
There was a problem hiding this comment.
For 1, we have an (incomplete) spectrum of misc orange colours that I had already been thinking of adding this to - but it isn't inverted as part of the dark theme. The colours are barely used, but for future cases like this where they would be used we probably should incorporate them into the colour shifting so we don't get problems like this. I'll work on that.
For 2, giving them backgrounds at all was intended to fix an issue with the dropdown items overlapping with images. Of course this does still work with a darker background rather than white, but it no longer blends into the background of most images. This is probably fine, but just to note.
| if (!isEqual(portalFuncs, newPortalFuncs)) { | ||
| setPortalFuncs(newPortalFuncs); | ||
| } | ||
| }, [newPortalFuncs, portalFuncs]); |
There was a problem hiding this comment.
This doesn't seem to have fixed the portals issue – I still get missing inputs after changing from Dropdown => DnD for the first time since page load:
On Ada, it seems to keep happening until I toggle twice...
There was a problem hiding this comment.
This is a problem with me poorly cleaning up the messy code I had after my load of portal testing 🤦
The other half of this should be reconstructing the portalFunc callback when the toggle is used by adding dragAndDropEnabled as a dependency - which is what actually triggers the useEffect.
NOW it should hopefully be fixed...
| </DropdownItem> | ||
| {dropdownItems.map((item, i) => { | ||
| return <DropdownItem key={i} | ||
| className={!nonSelectedItemIds.includes(item.id) ? "invalid" : ""} |
There was a problem hiding this comment.
A subtle issue, but switching from DnD <=> drop zones clears the nonSelectedItemIds, meaning that items that have been used appear to be selectable again despite not doing anything when clicked. For example, in the following image, 3N should be greyed out, as it is used prior:
| aria-describedby="non-dragging-movement-helptext" | ||
| removeVerticalOffset | ||
| /></b> | ||
| <p id="non-dragging-helptext">{`Enabling this will allow you to use alternative input methods that don't require dragging for certain question types (e.g. drag-and-drop).`}</p> |
There was a problem hiding this comment.
I do find this a little confusing as is.
"Enabling this will allow you to use..." makes it sound like you will be given the option to use alternative input methods for DnD questions, presumably somewhere on the question page. But if you haven't selected "Warn about inaccessible content" too then you aren't given any options, and the questions are just forcibly set to using dropdowns with no indication that anything has changed. I was expecting the toggle to exist in all situations, but accessible from within the banner if said banner exists (such that all accessibility-related content is in the same place), or outside above the (entire) question otherwise. It could even still have a warning banner in the case where "Warn about inaccessible content" is off, for consistency's sake, but I'm not sure what the text would say in that case.
Speaking of things we disagree with in your latest comment, I disagree with the solution to use an in-question toggle at all. I truly believe the chances of needing to switch an accessibility setting once you have set them initially is incredibly negligible. Why do content insist on the option there on every question?? It's messy for us having to consider situations like this and just clutters the accessibility tree... ugh.
There was a problem hiding this comment.
On the latter point, I think they see this as both an accessibility setting AND a convenience one. Like I remember them raising that for DnD questions taller than the user's screen, it would be helpful to be able to enable dropdowns for that question in particular - but that they might still want dragging for a more reasonably-sized question. That does sound more like a problem with the question content being too tall, but I can at least see where the idea is coming from.
This incorporates the current partial orange palette, $color-misc-pale-yellow (as 100) and $color-wildcard-orange (as 500), and is built to correspond to the hue shifts of bootstrap's alert colours. Most colours in the spectrum as-yet unused, but they should enable future development.
4 participants
Adds an accessibility setting to "Enable non-dragging alternative inputs". For now this only affects cloze/drag-and-drop questions, but is defined broadly enough that we can use the same setting for other inputs in the future if they apply (e.g. depending on how we implement reorder question accessibility).
Also adds a
Dropdown/Drag and dropoverride toggle to all question and quiz pages containing these input types in the top right, equivalent to the context switcher on concept pages. The toggle is defaulted toDropdownif the user has enabled the accessibility setting or is on a mobile device, andDrag and dropotherwise. If manually toggled, this overrides all other settings and the toggled input mode is enabled instead (e.g. allowing drag and drop at xs screens, if wanted).I'm not 100% confident on things like:
Thoughts welcome!
Also requires https://github.com/isaacphysics/isaac-sops-config/pull/22 for new accessibility setting to work.Merged!Known problem: If a question is using dropdowns on first page load, then the first time drag and drop is switched to the dropzones will not load properly. This is a pre-existing issue (try some incremental page width changes on a live question) but is surfaced a lot more heavily here. This is sure to be some portalling shenanigans, and I don't think we should merge this without fixing it, but the rest is ready to review now.